gh-99942: correct the pkg-config/python-config flags for cygwin/android by eli-schwartz · Pull Request #100967 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation4 Commits1 Checks0 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
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.
…/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 configure.ac would be good. I think the most future-proof way to do it would be to document the change on the issue, and have a link to that comment in configure.ac, and maybe also add the PR ID (with the GH-100967 format) in the comment.
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:
- PR title
- PR title and commit details
- PR title and PR description
- an alias "github default" which is currently option 2
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
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull request
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull request
dnicolodi pushed a commit to dnicolodi/meson that referenced this pull request
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull request
…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
…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
…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
…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
…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
Volker-Weissmann pushed a commit to Volker-Weissmann/meson that referenced this pull request
…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
Labels
The build process and cross-build