Skip to content
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

gh-111225: Link extension modules against libpython on Android #115780

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Feb 21, 2024

For the reasons explained in PEP 738, extension modules must be linked against libpython on Android.

Cygwin has the same requirement, and it looks like there were two existing attempts to implement this:

  • One of them, in makesetup, only applied to Cygwin.
  • The other, in configure.ac, applied to both Cygwin and Android, and worked by setting a LIBPYTHON variable. But this had no effect, because the variable wasn't actually being used anywhere.

I've removed the first attempt in favor of the second one, and fixed it as follows:

  • Renamed the variable to MODULE_LDFLAGS to more accurately reflect its purpose.
  • Edited makesetup to use the variable when linking extension modules.
  • Edited the Makefile so that extension modules depend on libpython on these platforms.
  • Restored -fPIC on Android. It was removed several years ago with a note that the toolchain used it automatically, but this is no longer the case. Omitting it causes all linker commands to fail with an error like relocation R_AARCH64_ADR_PREL_PG_HI21 cannot be used against symbol '_Py_FalseStruct'; recompile with -fPIC.

Before:

$ readelf -d _socket.cpython-313.so | grep -E 'NEEDED|SONAME' 
 0x0000000000000001 (NEEDED)             Shared library: [libm.so]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so]

After:

 0x0000000000000001 (NEEDED)             Shared library: [libm.so]
 0x0000000000000001 (NEEDED)             Shared library: [libpython3.13.so.1.0]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so]

erlend-aasland
erlend-aasland previously approved these changes Feb 21, 2024
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Looks good; thanks a lot for making this bug-fix in a separate PR.

@erlend-aasland

This comment was marked as resolved.

@erlend-aasland erlend-aasland dismissed their stale review February 21, 2024 21:18

macOS shared build broke

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Feb 21, 2024

Hm, seems the macOS build is broke for shared builds on main; so regard my previous comment (I marked it as resolved).

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Sorry for the back and forth; I had forgotten to clean the build folder. I redid my tests and I now can confirm that this breaks --enable-shared builds on macOS (main works fine).

@bedevere-app

This comment was marked as outdated.

@erlend-aasland
Copy link
Contributor

Sorry for the back and forth; I had forgotten to clean the build folder. I redid my tests and I now can confirm that this breaks --enable-shared builds on macOS (main works fine).

Aha, this PR does not include 074bbec; rebasing onto main fixed the problem. Looks like this is good to go again 😄

@erlend-aasland erlend-aasland enabled auto-merge (squash) February 21, 2024 22:41
@erlend-aasland erlend-aasland merged commit 7f5e3f0 into python:main Feb 21, 2024
36 of 37 checks passed
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…ython#115780)

Part of the work on PEP 738: Adding Android as a supported platform.

* Rename the LIBPYTHON variable to MODULE_LDFLAGS, to more accurately 
  reflect its purpose.
* Edit makesetup to use MODULE_LDFLAGS when linking extension modules.
* Edit the Makefile so that extension modules depend on libpython on 
  Android and Cygwin.
* Restore `-fPIC` on Android. It was removed several years ago with a 
  note that the toolchain used it automatically, but this is no longer
  the case. Omitting it causes all linker commands to fail with an error
  like `relocation R_AARCH64_ADR_PREL_PG_HI21 cannot be used against
  symbol '_Py_FalseStruct'; recompile with -fPIC`.
tacaswell added a commit to tacaswell/cpython that referenced this pull request Mar 13, 2024
Follow on to python#115780 .

Without this change numpy's build is broken.
befeleme pushed a commit to fedora-python/cpython that referenced this pull request Mar 20, 2024
Follow on to python#115780 .

Without this change numpy's build is broken.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ython#115780)

Part of the work on PEP 738: Adding Android as a supported platform.

* Rename the LIBPYTHON variable to MODULE_LDFLAGS, to more accurately 
  reflect its purpose.
* Edit makesetup to use MODULE_LDFLAGS when linking extension modules.
* Edit the Makefile so that extension modules depend on libpython on 
  Android and Cygwin.
* Restore `-fPIC` on Android. It was removed several years ago with a 
  note that the toolchain used it automatically, but this is no longer
  the case. Omitting it causes all linker commands to fail with an error
  like `relocation R_AARCH64_ADR_PREL_PG_HI21 cannot be used against
  symbol '_Py_FalseStruct'; recompile with -fPIC`.
@eli-schwartz
Copy link
Contributor

Renamed the variable to MODULE_LDFLAGS to more accurately reflect its purpose.

Renaming it breaks the de-facto API that the sysconfig module has provided for 5 years, and which downstream projects rely on.

Cygwin has the same requirement, and it looks like there were two existing attempts to implement this:

  • One of them, in makesetup, only applied to Cygwin.

  • The other, in configure.ac, applied to both Cygwin and Android, and worked by setting a LIBPYTHON variable. But this had no effect, because the variable wasn't actually being used anywhere.

That check was originally written to configure the python-config script -- I later updated it to also configure the python.pc file. distutils had its own, second copy of the configure.ac logic which was probably dropped during the migration away from internally using distutils. Not really the fault of the variable itself but what can you do. :)

@mhsmith
Copy link
Member Author

mhsmith commented Jul 30, 2024

It's unfortunate that sysconfig exposes all the Makefile variables in this way, because most of them were created with no intention of becoming a stable API. Can you give some examples of projects that rely on this variable?

Setuptools, at least, does not rely on it. It has its own logic for deciding which platforms need to link against libpython, and it already knows about Windows, Cygwin and Android.

@eli-schwartz
Copy link
Contributor

https://github.com/mesonbuild/meson/blob/fa4f2339465ce3d755e2df802ebd5aa962e2ad27/mesonbuild/scripts/python_info.py#L66-L79

Lacking any formalization of the information which might come via #107956, it is necessary to poke at raw Makefile variables -- in this case, the raw Makefile variable that corresponds to whether the "C-API extension modules" pkg-config file is configured to link to libpython. The setuptools logic is actually a copy-paste of the stdlib distutils logic, which was manually kept in sync with Makefile.pre.in up until it got deleted from the stdlib.

@rgommers
Copy link

rgommers commented Aug 2, 2024

This looks like it's going to break the build for NumPy on Android - the flag to link to libpython is going to silently disappear. And there is a fair amount of usage of NumPy on Android, judging by bug reports over the past few years. And we won't be able to work around this for NumPy 2.1.0 which is about to be released with Python 3.13 support, even if Meson adapts to this rename.

LIBPYTHON is the most obvious way to determine whether the linker flag is needed yes or no. It's one of the more important flags one can access through get_config_vars. A libpython key is also present in the current draft of PEP 739: https://peps.python.org/pep-0739/#libpython for that reason.

It's unfortunate that sysconfig exposes all the Makefile variables in this way, because most of them were created with no intention of becoming a stable API

I couldn't agree more, but unfortunately that's the situation we're in. It's best not to rename variables unless the actual content/semantics of the variable changes. In this case, reverting the rename looks like the right thing to do.

@mhsmith
Copy link
Member Author

mhsmith commented Aug 6, 2024

Thanks for the explanation.

This looks like it's going to break the build for NumPy on Android

Actually as @eli-schwartz says in #116745 (comment), it looks like it will break the build on every platform except Android, because the Meson code defaults to linking against libpython when the variable is missing from sysconfig.

So I agree the rename must be reverted. I've done that in #122764.

@eli-schwartz
Copy link
Contributor

Thanks for looking into this! :)

Fingers crossed that PEP 739 can be rolled out in the near future (hopefully as a beautiful gift included in 3.14).

@Sanny388cry
Copy link

mhsmith:android-link-modules

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

Successfully merging this pull request may close these issues.

Cross Compile for Android and issues with PyExc_OSError
5 participants