bpo-43312: Functions returning default and preferred sysconfig schemes#24644

uranusjr · 2021-02-25T06:46:19Z

https://bugs.python.org/issue43312

github-actions · 2021-03-28T00:12:38Z

This PR is stale because it has been open for 30 days with no activity.

uranusjr · 2021-03-29T21:36:56Z

Uh, how do I remove the stale label?

pfmoore

I can't say whether the choices in _get_preferred_schemes are correct, but they seem reasonable. Unless there's anything we can check them against, I'd say we should go with them.

gaborbernat · 2021-04-07T08:35:10Z

Lib/sysconfig.py



-defget_paths(scheme=_get_default_scheme(), vars=None, expand=True):
+defget_paths(scheme=get_default_scheme(), vars=None, expand=True):


Do we need to call this function every time? Wouldn't be better to call it just once and save it into a private global variable, and use that as the default value for all these methods?

That’s a separate issue already presented in the previous implementation. I’m not sure this is the best plac eto discuss the possible implications in e.g. multiprocessing settings (I don’t even have enough expertise to comment on it).
As it currently stands, the function is only called twice (get_paths and get_path) on import time, so the performance difference is pretty close to none anyway.

Agreed, let's keep the change to the minimal required.

gaborbernat · 2021-04-07T08:42:33Z

Lib/sysconfig.py

 print('Platform: "%s"'%get_platform())
 print('Python version: "%s"'%get_python_version())
-print('Current installation scheme: "%s"'%_get_default_scheme())
+print('Current installation scheme: "%s"'%get_default_scheme())


Would be a good opportunity to migrate this to f-strings?

Probably, I’ll combine this to other needed changes, if there are any.

hroncok

I ❤️ it!

Several non-blocking questions attached.

However, I have a very strong suggestion: Please add tests.

hroncok · 2021-04-14T13:43:41Z

Lib/sysconfig.py

+ifschemenotin_INSTALL_SCHEMES:
+raiseKeyError(key)


Will the above lookup not raise already? Should this be done before calling _get_preferred_schemes?

The main intention is to guard against incorrect implementation, since _get_preferred_schemes() is expected to be modified by redistributors and implementers. Say an implementer makes a typo:
def_get_preferred_schemes(): return{"prefix": "posix_prefix", "home": "posix_home", "user": "posix_uesr", # Oops! }
And when the value is later used:
scheme=sysconfig.get_preferred_scheme("user") ... # Much later...paths=sysconfig.get_paths(scheme)
This check would catch that posix_uesr is not actually a valid scheme and error early, instead of later when the user actually tries to use the (invalid) scheme.

Can we raise a ValueError with a better message then? "Key '{}' returned scheme '{}' which is not valid on this platform" (or something more correct)

I couldn’t think of a better message and implemented yours for now, any ideas are very welcomed. Ideally I want to point users to the implementer/redistributor instead of reporting the error to CPython, but that’s probably not really feasible.

Lib/sysconfig.py

This reduces logic duplication with a slight performance tradeoff.

uranusjr · 2021-04-14T16:57:38Z

I’ve changed get_default_scheme() to use get_preferred_scheme(), and a couple of simple tests for the new functions.

frenzymadness · 2021-04-15T10:25:39Z

The implementation looks good to me.

uranusjr · 2021-04-15T12:04:23Z

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

encukou · 2021-04-16T12:04:11Z

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

At least a docstring would be great, yes.

uranusjr · 2021-04-17T05:45:59Z

Documentation entry added 👍

Edit: Plus converting %-formatting to f-strings.

Drive-by refactoring.

hroncok · 2021-04-19T15:06:37Z

Curiosity question: Should distutils use this?

uranusjr · 2021-04-19T16:52:52Z

Ideally yes, but distutils’s implementation around this is structured very differently, and tangled with a lot of things inside distutils.command.install, I’m not sure if it’s really worthwhile (or even doable without risking behavioural changes) 😞

zooba

Would like to see that error message improved, but the rest seems fine to me. I'm just taking on trust that it's sufficient for distros to override what they need, because I don't know that side of it well enough.

zooba · 2021-04-19T20:29:52Z

Lib/sysconfig.py

+ifschemenotin_INSTALL_SCHEMES:
+raiseKeyError(key)


Can we raise a ValueError with a better message then? "Key '{}' returned scheme '{}' which is not valid on this platform" (or something more correct)

uranusjr · 2021-04-27T08:26:29Z

Another nudge 🙂

pfmoore · 2021-04-27T08:45:24Z

I believe all comments have been addressed, and I see no remaining concerns raised and general approval of the change, so I'm going to merge this based on my approval above.

the-knights-who-say-ni added the CLA signed label Feb 25, 2021

bedevere-bot added the awaiting review label Feb 25, 2021

github-actionsbot added the stale Stale PR or inactive for long period of time. label Mar 28, 2021

Make default scheme function public
a0d613f

uranusjr force-pushed the sysconfig-preferred-scheme branch from acadc40 to a0d613fCompare March 29, 2021 20:17

uranusjr marked this pull request as ready for review March 29, 2021 21:36

github-actionsbot removed the stale Stale PR or inactive for long period of time. label Mar 30, 2021

pfmoore approved these changes Apr 6, 2021
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 6, 2021

gaborbernat reviewed Apr 7, 2021
View reviewed changes

hroncok reviewed Apr 14, 2021
View reviewed changes

uranusjr added 2 commits April 15, 2021 00:09

Make get_default_scheme call get_preferred_scheme
1f15613
This reduces logic duplication with a slight performance tradeoff.

Add tests for scheme-related sysconfig functions
122bac5

Add documentation entry for Python redistributors
01ffb68

Convert string formatting to use f-strings
fd46323
Drive-by refactoring.

zooba reviewed Apr 19, 2021
View reviewed changes

Better error message on invalid scheme
1425a92

pfmoore merged commit d925133 into python:masterApr 27, 2021

bedevere-bot removed the awaiting merge label Apr 27, 2021

uranusjr deleted the sysconfig-preferred-scheme branch April 27, 2021 08:46

carlbordum mentioned this pull request May 27, 2022
bpo-37592: sysconfig no longer relies on sys.version #14776
Closed

mattip mentioned this pull request Nov 16, 2022
sysconfig should not rely on sys.version #81773
Open

FFY00 mentioned this pull request Dec 13, 2022
sysconfig.get_default_schemeversionadded vs versionchanged#100206
Closed



		defget_paths(scheme=_get_default_scheme(), vars=None, expand=True):
		defget_paths(scheme=get_default_scheme(), vars=None, expand=True):

		ifschemenotin_INSTALL_SCHEMES:
		raiseKeyError(key)

		ifschemenotin_INSTALL_SCHEMES:
		raiseKeyError(key)

Uh oh!

bpo-43312: Functions returning default and preferred sysconfig schemes#24644

bpo-43312: Functions returning default and preferred sysconfig schemes #24644

Uh oh!

Conversation

uranusjr commented Feb 25, 2021• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsbot commented Mar 28, 2021

Uh oh!

uranusjr commented Mar 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfmoore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uranusjr commented Apr 14, 2021

Uh oh!

frenzymadness commented Apr 15, 2021

Uh oh!

uranusjr commented Apr 15, 2021

Uh oh!

encukou commented Apr 16, 2021

Uh oh!

uranusjr commented Apr 17, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hroncok commented Apr 19, 2021

Uh oh!

uranusjr commented Apr 19, 2021

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uranusjr commented Apr 27, 2021

Uh oh!

pfmoore commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

uranusjr commented Feb 25, 2021•
edited by bedevere-bot
Loading

uranusjr commented Mar 29, 2021•
edited
Loading

uranusjr commented Apr 17, 2021•
edited
Loading