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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Doc/distutils/apiref.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ the full reference.
.. versionchanged:: 3.8

On Unix, C extensions are no longer linked to libpython except on
Android.
Android and Cygwin.


.. class:: Distribution
Expand Down
17 changes: 9 additions & 8 deletions Doc/whatsnew/3.8.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ environment variable, can be set using the new ``./configure --with-trace-refs``
build option.
(Contributed by Victor Stinner in :issue:`36465`.)

On Unix, C extensions are no longer linked to libpython except on Android.
On Unix, C extensions are no longer linked to libpython except on Android
and Cygwin.
It is now possible
for a statically linked Python to load a C extension built using a shared
library Python.
Expand All @@ -163,8 +164,8 @@ previous command fails (replace ``X.Y`` with the Python version).

On the other hand, ``pkg-config python3.8 --libs`` no longer contains
``-lpython3.8``. C extensions must not be linked to libpython (except on
Android, case handled by the script); this change is backward incompatible on
purpose.
Android and Cygwin, whose cases are handled by the script);
this change is backward incompatible on purpose.
(Contributed by Victor Stinner in :issue:`36721`.)

f-strings now support = for quick and easy debugging
Expand Down Expand Up @@ -1061,12 +1062,12 @@ Changes in the C API
instead.
(Contributed by Victor Stinner in :issue:`36728`.)

* On Unix, C extensions are no longer linked to libpython except on
Android. When Python is embedded, ``libpython`` must not be loaded with
* On Unix, C extensions are no longer linked to libpython except on Android
and Cygwin. When Python is embedded, ``libpython`` must not be loaded with
``RTLD_LOCAL``, but ``RTLD_GLOBAL`` instead. Previously, using
``RTLD_LOCAL``, it was already not possible to load C extensions which were
not linked to ``libpython``, like C extensions of the standard library built
by the ``*shared*`` section of ``Modules/Setup``.
``RTLD_LOCAL``, it was already not possible to load C extensions which
were not linked to ``libpython``, like C extensions of the standard
library built by the ``*shared*`` section of ``Modules/Setup``.

* Use of ``#`` variants of formats in parsing or building value (e.g.
:c:func:`PyArg_ParseTuple`, :c:func:`Py_BuildValue`, :c:func:`PyObject_CallFunction`,
Expand Down
36 changes: 24 additions & 12 deletions Lib/distutils/command/build_ext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,20 +714,32 @@ def get_libraries(self, ext):
# don't extend ext.libraries, it may be shared with other
# extensions, it is a reference to the original list
return ext.libraries + [pythonlib]
# On Android only the main executable and LD_PRELOADs are considered
# to be RTLD_GLOBAL, all the dependencies of the main executable
# 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).
else:
# On Android only the main executable and LD_PRELOADs are considered
# to be RTLD_GLOBAL, all the dependencies of the main executable
# 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.
Comment on lines +720 to +725
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).

from distutils.sysconfig import get_config_var
link_libpython = False
if get_config_var('Py_ENABLE_SHARED'):
# Either a native build on an Android device or the
# cross-compilation of Python.
if (hasattr(sys, 'getandroidapilevel') or
('_PYTHON_HOST_PLATFORM' in os.environ and
get_config_var('ANDROID_API_LEVEL') != 0)):
ldversion = get_config_var('LDVERSION')
return ext.libraries + ['python' + ldversion]
# A native build on an Android device or on Cygwin
if hasattr(sys, 'getandroidapilevel'):
link_libpython = True
elif sys.platform == 'cygwin':
link_libpython = True
elif '_PYTHON_HOST_PLATFORM' in os.environ:
# We are cross-compiling for one of the relevant platforms
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...


if link_libpython:
ldversion = get_config_var('LDVERSION')
return ext.libraries + ['python' + ldversion]

return ext.libraries
3 changes: 2 additions & 1 deletion Misc/NEWS.d/3.8.0a4.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,8 @@ Remove the stale scriptsinstall Makefile target.
.. nonce: ACQkiC
.. section: Build

On Unix, C extensions are no longer linked to libpython except on Android.
On Unix, C extensions are no longer linked to libpython except on Android
and Cygwin.

It is now possible for a statically linked Python to load a C extension
built using a shared library Python.
Expand Down
4 changes: 2 additions & 2 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15129,9 +15129,9 @@ LDVERSION='$(VERSION)$(ABIFLAGS)'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $LDVERSION" >&5
$as_echo "$LDVERSION" >&6; }

# On Android the shared libraries must be linked with libpython.
# On Android and Cygwin the shared libraries must be linked with libpython.

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

LIBPYTHON=''
else
LIBPYTHON="-lpython${VERSION}${ABIFLAGS}"
Expand Down
4 changes: 2 additions & 2 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4620,9 +4620,9 @@ AC_MSG_CHECKING(LDVERSION)
LDVERSION='$(VERSION)$(ABIFLAGS)'
AC_MSG_RESULT($LDVERSION)

# On Android the shared libraries must be linked with libpython.
# On Android and Cygwin the shared libraries must be linked with libpython.
AC_SUBST(LIBPYTHON)
if test -z "$ANDROID_API_LEVEL"; then
if test -z "$ANDROID_API_LEVEL" -o "$MACHDEP" != "cygwin"; then
LIBPYTHON=''
else
LIBPYTHON="-lpython${VERSION}${ABIFLAGS}"
Expand Down