-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use posix_prefix for Apple's system Python #10256
Use posix_prefix for Apple's system Python #10256
Conversation
Thanks! I'd love to test :) haven't done this before though, how do I upgrade my pip to this branch? |
Try
|
Cool, I've installed it, tested by un/reinstalling the Meraki python package but it's still coming up:
I'm available if you want to do live testing via Skype/Zoom |
Hmm OkK, so we made progress (the warning is different) but are not quite there. Thanks, I'll continue investigating. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid mistake
Caused by a stupid mistake 🤦 Could you install the latest change? (same command) |
Nice catch, here's the output now:
|
Hmm, the output didn’t change? That doesn’t sound right. Could you try adding |
It looks like that worked! |
|
d9bf6e1
to
051de2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I have a very poor understanding of the logic here. I just happened to be looking at it a few hours ago for a completely unrelated problem. Hence my suggestions may be completely irrelevant: feel free to ignore.
@SylvainDe I’d still be happy to know what you think! Although it seems you didn’t post any comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry for the inconvenience, my initial comments were not sent for some reason)
@@ -43,8 +69,7 @@ def _infer_prefix() -> str: | |||
""" | |||
if _PREFERRED_SCHEME_API: | |||
return _PREFERRED_SCHEME_API("prefix") | |||
os_framework_global = is_osx_framework() and not running_under_virtualenv() | |||
if os_framework_global and "osx_framework_library" in _AVAILABLE_SCHEMES: | |||
if _should_use_osx_framework_prefix(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that this somehow hides the:
if val1 in _AVAILABLE_SCHEMES:
return val1
if val2 in _AVAILABLE_SCHEMES:
return val2
In fact, I'd even be tempted to rewrite the code in such a way that the check if performed in a single place.
def yield_candidates(): # or a better name :)
if is_osx_framework() and not running_under_virtualenv(): # or _should_use_osx_framework_prefix()
yield "osx_framework_library"
yield f"{sys.implementation.name}_{os.name}"
yield sys.implementation.name
yield f"{os.name}_prefix"
yield os.name # On Windows, prefx is just called "nt".
for val in yield_candidates:
if val in _AVAILABLE_SCHEMES:
return val
return "posix_prefix"
This is probably going out of scope for a code review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice to not always call _AVAILABLE_SCHEMES
is intentional; those that aren’t checked are from upstream CPython, and a sysconfig
without those values is seriously broken, so it’s better to panic-abort than silently trying something else IMO.
# Special case: When installing into a custom prefix, use posix_prefix | ||
# instead of osx_framework_library. See _should_use_osx_framework_prefix() | ||
# docstring for details. | ||
if prefix is not None and scheme_name == "osx_framework_library": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my understanding is correct, this can only correspond to the situation where we've gone into the scheme_name = _infer_prefix()
case and that particular function is only called from here.
Hence, I wonder if it'd make sense to have that particular piece of logic directly in the _infer_prefix
rather than as step performed afterward.
This could correspond to doing something like:
- adding a param to the _infer_prefix function about the prefix being provided or not
- writing
return "osx_framework_library" if provided_prefix is None else "posix_prefix"
in the _infer_prefix function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention behind the current approach is to make those_infer_*
functions act similarly to sysconfig.get_preferred_scheme
(because eventually we’re going to replace the ad-hoc inference with it). If we put the logic in _infer_prefix
, when we remove _infer_user
and _infer_home
, we’ll still be left with one short _infer_prefix
function. I don’t think either structure is better though, this is just how I mentally structure this logic (first I ask the macOS Python what its preferred scheme is, and if it says osx_framework_library
, oh I’m going to ignore it).
For #10151 (comment)
This adds additional logic to NOT use Apple’s
osx_framework_library
forpip install --prefix
, matching distutils’s behaviour. Using a framework scheme under a custom prefix doesn’t really work, so this makes sense.cc @stiewie33 for testing.