gh-99942: correct the pkg-config/python-config flags for cygwin/android #100967
Conversation
This file contains 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
…/android On shared build configurations, Cygwin and Android need to link to libpython. See bpo-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 libpython *is* 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.
eli-schwartz
force-pushed
the
configure-libpython
branch
from
bb0fede
to
f40ce64
Compare
|
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 request
Sep 8, 2023
Disagreement between distutils and sysconfig have been resolved for Python 3.12, see python/cpython#100356 and python/cpython#100967 This is the other half of the fix for mesonbuild#7702.
dnicolodi
pushed a commit
to dnicolodi/meson
that referenced
this pull request
Sep 8, 2023
Disagreement between distutils and sysconfig have been resolved for Python 3.12, see python/cpython#100356 and python/cpython#100967 This is the other half of the fix for mesonbuild#7702.
dnicolodi
pushed a commit
to dnicolodi/meson
that referenced
this pull request
Sep 8, 2023
Disagreement between distutils and sysconfig have been resolved for Python 3.12, see python/cpython#100356 and python/cpython#100967 This is the other half of the fix for mesonbuild#7702.
dnicolodi
pushed a commit
to dnicolodi/meson
that referenced
this pull request
Sep 9, 2023
Disagreement between distutils and sysconfig have been resolved for Python 3.12, see python/cpython#100356 and python/cpython#100967 This is the other half of the fix for mesonbuild#7702.
eli-schwartz
added a commit
to eli-schwartz/meson
that referenced
this pull request
Sep 22, 2023
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig 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 the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - 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 do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for mesonbuild#7702
eli-schwartz
added a commit
to eli-schwartz/meson
that referenced
this pull request
Oct 2, 2023
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig 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 the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - 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 do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for mesonbuild#7702
eli-schwartz
added a commit
to eli-schwartz/meson
that referenced
this pull request
Oct 3, 2023
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig 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 the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - 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 do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for mesonbuild#7702 (cherry picked from commit 2d6c109)
robtaylor
pushed a commit
to robtaylor/meson
that referenced
this pull request
Oct 14, 2023
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig 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 the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - 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 do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for mesonbuild#7702
nirbheek
pushed a commit
to mesonbuild/meson
that referenced
this pull request
Oct 17, 2023
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig 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 the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - 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 do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for #7702
Sign up for free
to 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.
On shared build configurations, Cygwin and Android need to link to libpython. See bpo-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 libpython is 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.