Skip to content

bpo-21536: On Cygwin, C extensions must be linked with libpython #13549

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

Merged
merged 1 commit into from
May 24, 2019

Conversation

embray
Copy link
Contributor

@embray embray commented May 24, 2019

It is also possible to link against a library or executable with a
statically linked libpython, but not both with the same DLL. In fact
building a statically linked python is currently broken on Cygwin
for other (related) reasons.

The same problem applies to other POSIX-like layers over Windows
(MinGW, MSYS) but Python's build system does not seem to attempt
to support those platforms at the moment.

Follow-up to #12946

https://bugs.python.org/issue21536

@vstinner

general.

It is also possible to link against a library or executable with a
statically linked libpython, but not both with the same DLL.  In fact
building a statically linked python is currently broken on Cygwin
for other (related) reasons.

The same problem applies to other POSIX-like layers over Windows
(MinGW, MSYS) but Python's build system does not seem to attempt
to support those platforms at the moment.
if get_config_var('ANDROID_API_LEVEL') != 0:
link_libpython = True
elif get_config_var('MACHDEP') == 'cygwin':
link_libpython = True
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I just mostly "unwrapped" the single if condition above into multiple explicit branches, which I think makes the logic clearer, especially with more cases added on...

@vstinner vstinner merged commit c994c8f into python:master May 24, 2019
@vstinner
Copy link
Member

LGTM.I merged your PR, thanks!

@embray
Copy link
Contributor Author

embray commented May 24, 2019

Thank you, though perhaps it was too quick! There is a bug in my logic in configure.ac.

@vstinner
Copy link
Member

Thank you, though perhaps it was too quick! There is a bug in my logic in configure.ac.

Ah? I didn't notice. Write a second PR in that case ;-)


if test -z "$ANDROID_API_LEVEL"; then
if test -z "$ANDROID_API_LEVEL" -o "$MACHDEP" != "cygwin"; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be -a not -o technically, but it would probably be even clearer if the test weren't negated, and were instead

if test -n "$ANDROID_API_LEVEL" -o "$MACHDEP" = "cygwin";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"even clearer if the test weren't negated" yeah, please invert the test conditions to avoid double negation ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad we're on the same page. It's the double-negation that tripped me up in the first place, and probably you too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably you too.

yes

@vstinner
Copy link
Member

Hum, is if test -z "$ANDROID_API_LEVEL" -o "$MACHDEP" != "cygwin"; then the issue?

I'm not comfortable with double negation :-) Maybe write:

if test -n "$ANDROID_API_LEVEL" -o "$MACHDEP" = "cygwin"; then
   LIBPYTHON=...
else
  LIBPYTHON=
fi

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…honGH-13549)

It is also possible to link against a library or executable with a
statically linked libpython, but not both with the same DLL.  In fact
building a statically linked python is currently broken on Cygwin
for other (related) reasons.

The same problem applies to other POSIX-like layers over Windows
(MinGW, MSYS) but Python's build system does not seem to attempt
to support those platforms at the moment.
Comment on lines +720 to +725
# remain RTLD_LOCAL and so the shared libraries must be linked with
# libpython when python is built with a shared python library (issue
# bpo-21536).
# On Cygwin (and if required, other POSIX-like platforms based on
# Windows like MinGW) it is simply necessary that all symbols in
# shared libraries are resolved at link time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@embray trying to better understand this change...

On Android, before this PR, it is technically possible to refrain from linking to libpython if and only if python itself is configured as a static build.

This Cygwin change applies here also within if get_config_var('Py_ENABLE_SHARED'): -- does Cygwin only need libpython in shared builds, like Cygwin, or all the time, like MSVC?

The commit message glosses over it by pointing out that static Cygwin builds are broken anyway. I am simply curious what the most correct approach is (when implementing similar logic outside of distutils).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants