bpo-43312: Functions returning default and preferred sysconfig schemes

[uranusjr]

https://bugs.python.org/issue43312

[github-actions]

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

[uranusjr]

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]

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?

[uranusjr]

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.

[pfmoore]

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

[gaborbernat]

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

[uranusjr]

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]

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

[uranusjr]

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.

[zooba]

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]

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.

[uranusjr]

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

[frenzymadness]

The implementation looks good to me.

[uranusjr]

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]

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]

Documentation entry added 👍

---

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

[hroncok]

Curiosity question: Should distutils use this?

[uranusjr]

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]

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]

Another nudge 🙂

[pfmoore]

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.