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

@@ -517,7 +542,7 @@ def get_path_names():
    return _SCHEME_KEYS


-def get_paths(scheme=_get_default_scheme(), vars=None, expand=True):
+def get_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

@@ -731,7 +756,7 @@ def _main():
        return
    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

+    if scheme not in _INSTALL_SCHEMES:
+        raise KeyError(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

+    if scheme not in _INSTALL_SCHEMES:
+        raise KeyError(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-actions bot 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 a0d613f Compare March 29, 2021 20:17

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

github-actions bot 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:master Apr 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_scheme versionadded vs versionchanged #100206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

uranusjr commented Feb 25, 2021 •

edited by bedevere-bot

Loading

github-actions bot commented Mar 28, 2021

uranusjr commented Mar 29, 2021 •

edited

Loading

pfmoore left a comment

gaborbernat Apr 7, 2021

uranusjr Apr 7, 2021

pfmoore Apr 7, 2021

gaborbernat Apr 7, 2021

uranusjr Apr 7, 2021

hroncok left a comment

hroncok Apr 14, 2021

uranusjr Apr 14, 2021

zooba Apr 19, 2021

uranusjr Apr 19, 2021

uranusjr commented Apr 14, 2021

frenzymadness commented Apr 15, 2021

uranusjr commented Apr 15, 2021

encukou commented Apr 16, 2021

uranusjr commented Apr 17, 2021 •

edited

Loading

hroncok commented Apr 19, 2021

uranusjr commented Apr 19, 2021

zooba left a comment

zooba Apr 19, 2021

uranusjr commented Apr 27, 2021

pfmoore commented Apr 27, 2021

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

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

Conversation

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

github-actions bot commented Mar 28, 2021

uranusjr commented Mar 29, 2021 • edited Loading

pfmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hroncok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uranusjr commented Apr 14, 2021

frenzymadness commented Apr 15, 2021

uranusjr commented Apr 15, 2021

encukou commented Apr 16, 2021

uranusjr commented Apr 17, 2021 • edited Loading

hroncok commented Apr 19, 2021

uranusjr commented Apr 19, 2021

zooba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uranusjr commented Apr 27, 2021

pfmoore commented Apr 27, 2021

uranusjr commented Feb 25, 2021 •

edited by bedevere-bot

Loading

uranusjr commented Mar 29, 2021 •

edited

Loading

uranusjr commented Apr 17, 2021 •

edited

Loading