-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Inconsistent platform support for taking the loaded libpython
path into account in getpath
#127970
Comments
Signed-off-by: Filipe Laíns <[email protected]
Signed-off-by: Filipe Laíns <[email protected]
Signed-off-by: Filipe Laíns <[email protected]
Everything in the initial version of Chances are, whatever changes you make here should be treated as new features (i.e. not backported). Unless someone is reporting an issue that started with the original changeover to Other than that, go for it. Just remember that 100% of the quirks in this file are relied upon by someone, and any changes will break them. Make sure they're clearly documented (including whatsnew, porting guide, etc.) |
Thanks for the clarification! I assumed the semantic change was deliberate, as this comment from the new code explicitly calls for it: Lines 574 to 588 in b9a492b
And the PR also added Windows support for this: Lines 806 to 812 in b9a492b
Though, given the complexity of the issue, I imagine this might have been something you meant to do as a follow-up, but ended up slipping into that PR.
Considering this, yeah, we should probably treat this as a new feature.
Right. In this particular scenario, I think there'll be a fairly low negative impact on users, the opposite, this will remove the necessity for workarounds such as the one discussed in GH-66409. Users relying on the current code behaving differently than in the proposed change are relying on the stdlib from a different Python installation being used, which is already fragile and unstable on its own. |
It came from There's very little value in reading the diff from the old Footnotes
|
Implemented in GH-127972. |
Bug report
Bug description:
The current
getpath.py
code tries determiningbase_prefix
/base_exec_prefix
by searching the location of thelibpython
library loaded in the current process, falling back to the location of the Python interpreter executable.cpython/Modules/getpath.py
Lines 559 to 594 in 7900a85
Looking at the location of the
libpython
library in use first makes sense, as that is more reliable than looking at interpreter location — it works when embedding, where there isn't any interpreter executable, it works when the executable is not onbase_prefix
, etc. However, this is only currently supported on Windows and macOS framework builds.cpython/Modules/getpath.c
Lines 802 to 837 in 7b8bd3b
The spotty platform support stroke me as odd, especially on macOS, as I see no apparent reason for only supporting framework builds, so I looked traced back the origin of this code.
The macOS logic goes back to Python 2.0, having been introduced in 54ecc3d. At this time, we were determining
base_prefix
/base_exec_prefix
based on the Python interpreter location, which was problematic on OS X Frameworks, as the Python interpreter is provided via a launcher. The comment traces back to 55070f5 and is unrelated to the change made by that commit, it just highlights the special case for macOS framework builds.In GH-29041, which introduced
getpath.py
, rewriting the old path initialization C code in Python, the logic changed to purposefully search thelibpython
location before the executable location, also adding Windows support. I imagine the existing macOS code was kept as-is as a mistake, leaving it behind theWITH_NEXT_FRAMEWORK
flag, maybe under the assumption it was needed for some reason.Considering the clear intent in the code, I am treating this a bug.
cc @zooba
CPython versions tested on:
CPython main branch
Operating systems tested on:
No response
Linked PRs
The text was updated successfully, but these errors were encountered: