meson-python icon indicating copy to clipboard operation
meson-python copied to clipboard

Fix support for `install_rpath`

Open dnicolodi opened this issue 9 months ago • 3 comments

dnicolodi avatar Feb 13 '25 16:02 dnicolodi

This should fix all identified issue and it is ready for review. The failed tests are due to https://github.com/mesonbuild/meson/issues/14254

dnicolodi avatar Feb 15 '25 11:02 dnicolodi

@dnicolodi sorry I'm struggling to find the time to think about this more thoroughly right now. This is fixing a pre-existing bug that's been there for a long time, right? And nothing is urgent (I can work around gh-711 for the time being). If so, wouldn't it be better to defer this PR until after 0.18.0?

rgommers avatar Feb 17 '25 23:02 rgommers

I don't see a problem with postponing this. On the other hand, I don't see any reason why we need to release 0.18.0 now. We can merge this for the next release or we can release 0.18.0 once this is merged. It is up to you.

dnicolodi avatar Feb 18 '25 16:02 dnicolodi

Hi, what is the status of this PR? Is install_rpath handled properly now? For reference: https://github.com/mesonbuild/meson-python/discussions/663.

In that discussion, the dependency for fixing install_rpath seems to have been merged into mesonbuild. However, as of meson 1.7.0 and meson-python 0.17.1, it still does not seem to work. Meson is now complaining that using link_flags will become a hard error in future releases, so even that workaround won't work then.

voidtrance avatar May 09 '25 14:05 voidtrance

what is the status of this PR?

It is not merged yet. What else would you like to know?

Is install_rpath handled properly now?

This PR is not merged yet, thus, no, it is not handled in the latest release of meson-python.

dnicolodi avatar May 25 '25 12:05 dnicolodi

@rgommers I'm inclined to merge this soon. Do you still want to have a better look at it before?

dnicolodi avatar Jun 14 '25 17:06 dnicolodi

@rgommers I'm inclined to merge this soon. Do you still want to have a better look at it before?

Sorry for the delay. Agreed, we should merge this, doing some final testing now - but I expect to find nothing and hit the green button. Going through my whole backlog now.

rgommers avatar Jul 15 '25 08:07 rgommers

doing some final testing now - but I expect to find nothing

Okay, that was a bit too optimistic.

Started testing with scipy 1.15.1, as the last released version to use an internal shared library. In all cases, the intro-install_plan.json entry contains "install_rpath": "$ORIGIN".

Linux, meson 1.7.1, meson-python main branch:

$ python -m build -wnx -Cbuild-dir=build
$ cd dist
$ unzip scipy-1.15.1-cp313-cp313-linux_x86_64.whl
$ readelf -d scipy/special/_ufuncs.cpython-313-x86_64-linux-gnu.so | rg "rpath|runpath"
 0x000000000000000f (RPATH)              Library rpath: [/home/rgommers/mambaforge/envs/scipy-dev/lib:$ORIGIN/]

Linux, meson 1.7.1, meson-python from this PR:

$ python -m build -wnx -Cbuild-dir=build
$ cd dist
$ unzip scipy-1.15.1-cp313-cp313-linux_x86_64.whl
$ readelf -d scipy/special/_ufuncs.cpython-313-x86_64-linux-gnu.so | rg "rpath|runpath"
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN]

LDFLAGS contains -Wl,-rpath:

$ echo $LDFLAGS   # from conda-forge compiler activation
-Wl,-O2 -Wl,--sort-common -Wl,--as-needed -Wl,-z,relro -Wl,-z,now -Wl,--disable-new-dtags -Wl,--gc-sections -Wl,--allow-shlib-undefined -Wl,-rpath,/home/rgommers/mambaforge/envs/scipy-dev/lib -Wl,-rpath-link,/home/rgommers/mambaforge/envs/scipy-dev/lib -L/home/rgommers/mambaforge/envs/scipy-dev/lib

So the behavior on main is correct and this PR breaks usage of -Wl,-rpath. As can be seen further from ldd output; no libraries are found from the conda-forge environment's libdir:

$ ldd scipy/special/_ufuncs.cpython-313-x86_64-linux-gnu.so 
        linux-vdso.so.1 (0x00007d47fdcea000)
        libsf_error_state.so => /home/rgommers/code/bldscipy/dist/scipy/special/libsf_error_state.so (0x00007d47fdbf0000)
        libopenblas.so.0 => not found
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007d47fd800000)
        libm.so.6 => /usr/lib/libm.so.6 (0x00007d47fdad0000)
        libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007d47fdaa2000)
        libc.so.6 => /usr/lib/libc.so.6 (0x00007d47fd60e000)
        /usr/lib64/ld-linux-x86-64.so.2 (0x00007d47fdcec000)

We do have a test case for -Wl,-rpath, but you removed it in this PR. That has to be incorrect; Conda, Nix, Spack at least rely on setting RPATHs explicitly, and those have to be preserved.

rgommers avatar Jul 15 '25 09:07 rgommers

So the behavior on main is correct and this PR breaks usage of -Wl,-rpath.

This PR does not (and cannot) distinguish RPATH entries based on how they are added. The old code didn't remove some Meson build RPATH entries and this has been fixed in this PR. In particular, if SciPy uses $ORINGIN unconditionally on all platforms, it is SciPy that is broken (and working by accident because of a bug in meson-python fixed by this PR): $ORIGIN does not have any meaning on macOS (see the changes required to the tests implemented in this PR).

dnicolodi avatar Jul 15 '25 17:07 dnicolodi

IIRC meson treats install_rpath as additive, not a replacement. The other angle is that it removes build_rpath -- and tracks various internally added rpaths needed for meson test and meson devenv as build rpaths.

So maybe the solution here is to treat the introspection files as incomplete and extend it with even more info?

eli-schwartz avatar Jul 15 '25 19:07 eli-schwartz

In particular, if SciPy uses $ORIGIN unconditionally on all platforms,

I didn't even get that far, haven't tested on macOS yet. What I pointed out is that this PR seems to break adding an RPATH with LDFLAGS=-Wl,-rpath,/some/path, which will break conda-forge et al.

IIRC meson treats install_rpath as additive, not a replacement

Yes, it is additive indeed.

So maybe the solution here is to treat the introspection files as incomplete and extend it with even more info?

If there is no other way to know what the build-rpath(s) is/are, yes maybe. I'm not yet fully convinced that we cannot know - we know in meson-python what the build directory is after all, so we may be able to compute this somehow from that build dir + install_rpath? Even if there are multiple, there is no way -Wl,-rpath should be pointing to within the build dir.

rgommers avatar Jul 15 '25 20:07 rgommers

If there is no other way to know what the build-rpath(s) is/are, yes maybe. I'm not yet fully convinced that we cannot know - we know in meson-python what the build directory is after all, so we may be able to compute this somehow from that build dir + install_rpath? Even if there are multiple, there is no way -Wl,-rpath should be pointing to within the build dir.

That might work, sure. Though FWIW internally meson does have target.rpath_dirs_to_remove inside the backend build data, and that's what meson install uses.

eli-schwartz avatar Jul 15 '25 21:07 eli-schwartz

install_rpath is treated as additive in this PR. I've just refreshed myself on how the implementation works, and what breaks is that in this PR it is assumed that any RPATH that is anchored to $ORIGIN is a build RPATH added by meson. This heuristic is clearly wrong. However, the build RPATHs do not refer the build directory but use $ORIGIN, thus the knowledge of the build directory is not useful to detect the build RPATHs. I don't see a solution other than augmenting the introspection metadata with target.rpath_dirs_to_remove.

dnicolodi avatar Jul 15 '25 22:07 dnicolodi

What I pointed out is that this PR seems to break adding an RPATH with LDFLAGS=-Wl,-rpath,/some/path, which will break conda-forge et al.

This is true only of the added RATH is anchored to $ORIGIN.

dnicolodi avatar Jul 15 '25 22:07 dnicolodi

This is true only of the added RATH is anchored to $ORIGIN.

That is not the case. I just checked more extension modules, ones that don't use install_rpath: at all - and the -Wl,-rpath,/some/abspath/ path really does go missing with this branch. There's only an empty RUNPATH entry in the built wheel for regular extension modules.

For the default conda-forge compilers and scipy-dev environment, with LDFLAGS containing -Wl,-rpath-link,/home/rgommers/mambaforge/envs/scipy-dev/lib, we get for extension modules in a built wheel:

On main:

  • No install_rpath: -> Library rpath: [/home/rgommers/mambaforge/envs/scipy-dev/lib]
  • With install_rpath: '$ORIGIN' -> Library rpath: [/home/rgommers/mambaforge/envs/scipy-dev/lib:$ORIGIN/]

With this PR:

  • No install_rpath: -> Library runpath: []
  • With install_rpath: '$ORIGIN' -> Library runpath: [$ORIGIN]

This should be reproducible for any package in a conda-forge environment with the compilers package installed.


Did a bit of digging, linking to the implementation in Meson for future reference:

https://github.com/mesonbuild/meson/blob/0376d655694747ba36aa2bfd94e5bce2d2f1efa8/mesonbuild/backend/backends.py#L791

What it seems to do is captured by this comment:

# For all link args that are absolute paths to a library file, add RPATH args

I don't see a solution other than augmenting the introspection metadata with target.rpath_dirs_to_remove.

That sounds right. If we see one or more absolute paths for rpath/runpath entries, we can't otherwise know if they were added by Meson itself yes or no.

rgommers avatar Jul 16 '25 14:07 rgommers