GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-99942: correct the pkg-config/python-config flags for cygwin/android by eli-schwartz · Pull Request #100967 · python/cpython

Conversation

eli-schwartz commented Jan 12, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Issue: Building and linking C extensions in a post-distutils world #99942

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added the awaiting review label

Jan 12, 2023

bedevere-bot mentioned this pull request

Jan 12, 2023

Building and linking C extensions in a post-distutils world #99942

Open


          pythongh-99942: correct the pkg-config/python-config flags for cygwin…

f40ce64

…/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

January 12, 2023 05:52

CAM-Gerlach added the build The build process and cross-build label

Jan 12, 2023

FFY00 approved these changes

Feb 16, 2023

View reviewed changes

FFY00 left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting review labels

Feb 16, 2023

FFY00 merged commit 226484e into python:main

Feb 16, 2023

bedevere-bot removed the awaiting merge label

Feb 16, 2023

eli-schwartz deleted the configure-libpython branch

February 16, 2023 18:56

eli-schwartz commented Feb 17, 2023

Copy link

Copy Markdown

Contributor Author

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...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FFY00 commented Feb 17, 2023

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eli-schwartz commented Feb 19, 2023

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dnicolodi pushed a commit to dnicolodi/meson that referenced this pull request

Sep 8, 2023


          python module: stop using distutils on sufficiently new python

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


          python module: stop using distutils on sufficiently new python

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


          python module: stop using distutils on sufficiently new python

dfef1a4

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


          python module: stop using distutils on sufficiently new python

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


          python module: stop using distutils "link to libpython" probe on rece…

3e20f08

…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


          python module: stop using distutils "link to libpython" probe on rece…

2d6c109

…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


          python module: stop using distutils "link to libpython" probe on rece…

3c493da

…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


          python module: stop using distutils "link to libpython" probe on rece…

6c60121

…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


          python module: stop using distutils "link to libpython" probe on rece…

3c3caf5

…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

May 1, 2025


          python module: stop using distutils "link to libpython" probe on rece…

857bf89

…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

Reviewers

FFY00

FFY00 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build

The build process and cross-build

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

eli-schwartz commented Jan 12, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FFY00 left a comment

Choose a reason for hiding this comment

Uh oh!

eli-schwartz commented Feb 17, 2023

Uh oh!

FFY00 commented Feb 17, 2023

Uh oh!

eli-schwartz commented Feb 19, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eli-schwartz commented Jan 12, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.