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

WIP: Generalize the rpath API for relocatable Python installations #4890

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

geofft
Copy link

@geofft geofft commented Feb 3, 2025

Relocatable Python installations like python-build-standalone have a similar need for an rpath as the macOS framework builds in Xcode. Furthermore, the platform on which you build your code is not really a function of the code itself. This change generalizes the recent add_python_framework_link_args() from #4833 to
add_python_link_args(), which also does the right thing on python-build-standalone, and encourages everyone to set up a build.rs file that invokes it, whether or not they need it on their current machine.


The motivation here is #4813 and astral-sh/uv#11006: currently, cargo test doesn't work if your test cases call into libpython and your libpython isn't installed systemwide.

I'm opening this now to get feedback on the general approach of encouraging everyone to have a build.rs file regardless of whether they currently need it. This needs some more docs changes to convey that. If this sounds good, I also plan to contribute a change to maturin new to create this file.

At the moment the heuristic for whether your Python installation needs the rpath setting is whether there's a sysconfig.get_var("PYTHON_BUILD_STANDALONE") == "1". We're planning on trying to get a mode in upstream CPython to build a relocatable installation (mostly to build bin/python itself with $ORIGIN-relative rpaths, but also to get libpython to detect some things about its own path better), at which point it would make sense to add an official sysconfig var to designate relocatability. Until then I think a heuristic is the best we can do. I suppose we can unconditionally emit an rpath line but having an rpath for system directories like /usr/lib is pretty weird.

Test case: download some recent python-build-standalone build, add its bin directory to the front of your PATH, and try to run cargo test on, say

 #[test]
fn test_something() {
    Python::with_gil(|_py| ());   
}

This currently fails, but works with this patch. (It does not work with a stock python-build-standalone build if you set PYO3_PYTHON instead of adding to your PATH, which is a bug; it does work with a Python installed by uv, which does some post-install tweaks.)

Relocatable Python installations like python-build-standalone have a
similar need for an rpath as the macOS framework builds in Xcode.
Furthermore, the platform on which you build your code is not really a
function of the code itself. This change generalizes the recent
`add_python_framework_link_args()` from PyO3#4833 to
`add_python_link_args()`, which also does the right thing on
python-build-standalone, and encourages everyone to set up a build.rs
file that invokes it, whether or not they need it on their current
machine.
@geofft
Copy link
Author

geofft commented Feb 3, 2025

The reason I think this is the right approach is that it's needed for macOS anyway. We could do something else on the python-build-standalone or uv side, but it still seems worthwhile to have everyone include the build.rs file so that their code builds on macOS.

A couple of alternatives (that wouldn't address the macOS need):

  1. We could get people to statically link libpython. But we're not necessarily guaranteed that the Python installation someone is trying to use has a static libpython (e.g., the "install-only" packages from python-build-standalone, which uv uses, don't have it; Fedora and Nix don't seem to have it in any available package at all), and using it is cumbersome in ways that require more heuristics (e.g., on a random Ubuntu machine, I seem to need to compile with -no-pie -Wl,--push-state -Wl,-Bstatic -lpython3.8 -lz -lexpat -pthread -lutil -Wl,--pop-state -ldl -lm—it fails if I leave off -no-pie, don't link zlib or expat, or try to use libdl.a or libm.a instead of their dynamic libraries). So this feels much less elegant than I originally would have thought it would be.
  2. You can set the soname of the Python library to its full path with e.g. patchelf --set-soname /wherever/your/lib/libpython3.13.so /wherever/your/lib/libpython3.13.so, and then that gets baked in when you link against it. But this has to be done post-install, meaning that it's not a great solution for python-build-standalone; we'd have to ask users to run a script after extracting the tarball, once we know the path where they've extracted it. (This would be straightforward to do in uv, though, and it already does the equivalent thing for macOS in Patch embedded install path for Python dylib on macOS during python install astral-sh/uv#10629. On the other hand, that bug is I think about the same problem as this one and would likely be resolved by this PR, meaning uv could back out that post-install patching if this lands.)

Asking every user to create a build.rs is a little annoying but it seems to be the best path here. (And maybe if someone figures out a good design for rust-lang/cargo#9554, we can go back to not requiring a build.rs.)

@ngoldbaum
Copy link
Contributor

ngoldbaum commented Feb 5, 2025

I'm opening this now to get feedback on the general approach of encouraging everyone to have a build.rs file regardless of whether they currently need it. This needs some more docs changes to convey that. If this sounds good, I also plan to contribute a change to maturin new to create this file.

I'm not sure there's appetite for this, since it's a big migration. Could there be a way to opt out for packages using the abi3 feature? Then at least there's an escape hatch for people that don't have a lot of stomach for managing packaging.

While spinning up on PyO3 I did notice that needing to opt in to depending on pyo3-build-config
and then add the resolve-config feature to get conditional compilation guards based on Python version was definitely a bit of a complexity bump. It would be nice if we could allow users to avoid it in the common case when they're just experimenting with a local Python executable. But maybe that's impossible with a python-build-standalone Python?

It'd be nice to add some tests using setup-uv along with this PR, could do it in a followup too.

@ngoldbaum
Copy link
Contributor

using setup-uv

Or I guess, using python-build-standalone if there's already a fix for this issue in uv.

@geofft
Copy link
Author

geofft commented Feb 5, 2025

I'm not sure there's appetite for this, since it's a big migration. Could there be a way to opt out for packages using the abi3 feature? Then at least there's an escape hatch for people that don't have a lot of stomach for managing packaging.

As I understand it, abi3 is only usable by extension modules loaded into Python, and not usable by Rust code loading libpython (because how do you know which libpython are you going to use), right? Which means that cargo test invoking Python is expected not to work for people using abi3?

If that's already the case, then yes, this is unnecessary for abi3 users. (Mostly my thinking was that it's good for people to be able to write tests; if they already can't write tests, then that argument doesn't apply.)

As I understand it, the rpath is not required by extension modules, regardless of whether they use abi3, regardless of platform (in particular, I believe the existing add_python_framework_link_args() is not needed for extension modules on macOS if you have no test cases).

I can double-check all these claims.

While spinning up on PyO3 I did notice that needing to opt in to depending on pyo3-build-config
and then add the resolve-config feature to get conditional compilation guards based on Python version was definitely a bit of a complexity bump.

To be clear I'm also relatively new to PyO3, but, I think if we tell people "in order to use pyo3, you need a build.rs file that does pyo3_build_config::do_all_the_things_i_need();" at step 1, then things like these conditional compilation guards will already be there when they need them, right? (And the rpath stuff will be there when they write tests.) Or in other words—is the complexity bump just the fact that you have to think about this? I think the actual mechanics of the change are just writing a three-line build.rs + adding a build-dependency on pyo3-build-config at the same version, right?

So I'm thinking that where the docs say, "to get started with pyo3, add this line to your Cargo.toml", they'll now say, "add these three lines to your Cargo.toml and create this three-line build.rs," and then people won't have to think about that as their project gets more complex / as they find some more unusual build scenario.

I'm happy to create that do_all_the_things_i_need() function (with a better name of course) that both does add_python_link_args() and use_pyo3_cfgs().

@ngoldbaum
Copy link
Contributor

As I understand it, abi3 is only usable by extension modules loaded into Python, and not usable by Rust code loading libpython (because how do you know which libpython are you going to use), right? Which means that cargo test invoking Python is expected not to work for people using abi3?

I think people tend to use e.g. pytest to manage tests and not use cargo test at all.

See https://pyo3.rs/v0.14.4/faq.html#i-cant-run-cargo-test-im-having-linker-issues-like-symbol-not-found-or-undefined-reference-to-_pyexc_systemerror and
#340

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.

2 participants