Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-99942: correct the pkg-config/python-config flags for cygwin/android#100967
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
…/androidOn shared build configurations, Cygwin and Android need to link tolibpython. See bpo-21536.This was corrected in distutils with an explicit check to only link whenlibpython is built shared, but implemented in configure.acunconditionally. The correct approach is to follow distutils, whichincludes a comment regarding why:- on Android, a shared libpython is RTLD_LOCAL, and thus available only to the main executable, not exported to loaded modules, but a static libpython *is* the main executable and thus exported- on Cygwin, symbols in shared libraries must be resolved at link timeIt's actually not clear to me what to do for Cygwin. Cygwin doesn'tactually build static libpython successfully. If it did, then extensionsprobably need to be linked to the import library for the executable,rather than the full static library. So omitting this here is probablycorrect, but incomplete. Either way, it won't matter until other fixesare made.
bb0fede tof40ce64CompareFFY00 approved these changesFeb 16, 2023
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.
Thanks@eli-schwartz.
Thanks for merging. BTW I notice the commit message got truncated during the merge... I spent quite some time writing up the context for it, because I expect it to be crucial knowledge for things like bisecting and git blame as it's not exactly obvious why these conditionals matter. What's the preferred CPython way for me to handle this? Should I add comments to configure.ac instead of writing commit messages? I was a bit afraid of making it overly long, since configure.ac serves many purposes and this is a minor one, and unlike distutils there is no dedicated file for the topic of handling platform flags. Still, if commit messages are discouraged in favor of inline code comments I can do that... |
We can easily find the PR from the commit, but having at least a small comment in The only option available to us is squash merge, so I need to copy the message to the commit body manually. I'll try to be more careful in the future. |
Thanks for clarifying. Also interesting 👀 because that seems like it would be deadly to the ability to cleanly bisect when merging complex PRs that cannot be untangled into multiple discrete PRs, but have multiple distinct commits to them. Although perhaps encouraging separate PRs renders that concern mostly irrelevant. ... I am surprised to learn that it's that difficult to get a good default commit message. Github offers 3 options for this in the settings page available to repository admins:
It sounds like the CPython repository is manually configured to option 1? I wonder why. |
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull requestSep 8, 2023
Disagreement between distutils and sysconfig have been resolved forPython 3.12, seepython/cpython#100356 andpython/cpython#100967This is the other half of the fix formesonbuild#7702.
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull requestSep 8, 2023
Disagreement between distutils and sysconfig have been resolved forPython 3.12, seepython/cpython#100356 andpython/cpython#100967This is the other half of the fix formesonbuild#7702.
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull requestSep 8, 2023
Disagreement between distutils and sysconfig have been resolved forPython 3.12, seepython/cpython#100356 andpython/cpython#100967This is the other half of the fix formesonbuild#7702.
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull requestSep 9, 2023
Disagreement between distutils and sysconfig have been resolved forPython 3.12, seepython/cpython#100356 andpython/cpython#100967This is the other half of the fix formesonbuild#7702.
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestSep 22, 2023
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix formesonbuild#7702
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestOct 2, 2023
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix formesonbuild#7702
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestOct 3, 2023
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix formesonbuild#7702(cherry picked from commit2d6c109)
robtaylor pushed a commit to robtaylor/meson that referenced this pull requestOct 14, 2023
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix formesonbuild#7702
nirbheek pushed a commit to mesonbuild/meson that referenced this pull requestOct 17, 2023
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix for#7702
Volker-Weissmann pushed a commit to Volker-Weissmann/meson that referenced this pull requestMay 1, 2025
…nt pythonOn python >=3.8, this information is expected to be encoded in thesysconfig vars.In distutils, it is always necessary to link to libpython on Windows;for posix platforms, it depends on the value of LIBPYTHON (which is thelibrary to link to, possibly the empty string) as generated byconfigure.ac and embedded into python.pc and python-config.sh, and thencoded a second time in the distutils python sources.There are a couple of caveats which have ramifications for Cygwin andAndroid:- python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython- python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter doThe disagreement is resolved in favor of distutils' behavior in allcases, and python.pc is correct for our purposes on python 3.12; see:python/cpython#100356python/cpython#100967Although it was not backported to older releases, Cygwin at least hasalways patched in a fix for python.pc, which behavior is now declaredcanonical. We can reliably assume it is always correct.This is the other half of the fix formesonbuild#7702
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
On shared build configurations, Cygwin and Android need to link to libpython. Seebpo-21536.
This was corrected in distutils with an explicit check to only link when libpython is built shared, but implemented in configure.ac unconditionally. The correct approach is to follow distutils, which includes a comment regarding why:
on Android, a shared libpython is RTLD_LOCAL, and thus available only to the main executable, not exported to loaded modules, but a static libpythonis the main executable and thus exported
on Cygwin, symbols in shared libraries must be resolved at link time
It's actually not clear to me what to do for Cygwin. Cygwin doesn't actually build static libpython successfully. If it did, then extensions probably need to be linked to the import library for the executable, rather than the full static library. So omitting this here is probably correct, but incomplete. Either way, it won't matter until other fixes are made.