Fix support for `install_rpath` #724

dnicolodi · 2025-02-13T16:58:36Z

No description provided.

dnicolodi · 2025-02-13T22:50:09Z

tests/packages/sharedlib-in-package/mypkg/meson.build

@@ -32,7 +32,7 @@ py.extension_module(
    include_directories: 'sub',
    install: true,
    subdir: 'mypkg',
-    install_rpath: '$ORIGIN',
+    install_rpath: build_machine.system() == 'darwin' ? '@loader_path' : '$ORIGIN',


So... This test was working before just because of a bug: we did no strip the RPATH pointing to the build directory added by meson when the package does not contain libraries relocated to .<package-name>.mesonpy.libs. Now we do and the test fails. The fix above is required because AFAICT meson does not substitures @loader_path for $ORIGIN on macOS. However, it is only half the fix.

The test installs one library in site-packages/mypkg/ and one in site-packages/mypkg/sub/. An RPATH to both of these directories would need to be added. However, meson does not provide a mechanism to set install_rpath to a list of strings.

I don't know how to fix this other than extending meson to accept a list for install_rpath or to simply change the test.

@rgommers You added the test. Which use case did you have in mind?

The test installs one library in site-packages/mypkg/ and one in site-packages/mypkg/sub/. An RPATH to both of these directories would need to be added. However, meson does not provide a mechanism to set install_rpath to a list of strings.

This was helpful to smoke out the details of Windows vs. Cygwin, namely this sentence in the docs:

The Windows DLL search path includes the path to the object attempting to load the DLL: it needs to be augmented only when the Python extension modules and the DLLs they require are installed in separate directories.

Now that it's done though, the test case isn't essential. I suggest that we remove the shared library right next to the extension, and keep the one in sub.

The fix above is required because AFAICT meson does not substitures @loader_path for $ORIGIN on macOS.

Hmm, something seems wrong there, $ORIGIN really should be made to work without this hack. At least SciPy releases depend on this already, and it's conceptually right. I don't fully understand this yet, but it seems if there's something to fix, it should be done in Meson in a backwards-compatible way.

When building a wheel, install_rpath is handled by meson-python, thus we can easily replace $ORIGIN with @loader_path. However, I think it is better to keep handling of install_rpath consistent between meson and meson-python. I haven't tested it, but the documentation does not mention it, and reading the code I didn't find anything in meson that does the translation, therefore I didn't implement it for meson-python either.

What do you mean when you say that SciPy depends on this? install_rpath did not do anything when building a wheel before this PR.

Could we land these sharedlib-in-package test package changes separately? This PR is very tricky, and making major changes to both tests and implementation at the same time makes it quite easy for regressions to slip in. None of the tests are actually wrong, but if cleaning up this test a bit helps then sure let's do it.

The tests are actually wrong and they work just because meson-python fails to clean up the RPATH entries added by meson pointing to the build directory. This PR fixes this and the tests start to fail.

I am not yet convinced that the tests are wrong. As we already determined in the discussion, there are two separate things that need to be achieved:

Keep the RPATH entries added by the package builder

Remove RPATH entries added by Meson and which it would have removed in the install step

The tests are for case (1). You can't argue that tests for (1) are wrong because you're trying to fix case (2).

Empirically, I know install_rpath: '$ORIGIN' was necessary and worked for SciPy, also for redistributable wheels tested on different machines where the original build dir doesn't exist.

I was suggesting to break out these changes to the tests so they can be reviewed/understood separately and merged. The test changes here may well be correct, but it's still unclear to me - and given that this PR is in a broken state currently for conda-forge, it makes it hard for me to test. If this is a separate test-only PR, testing will be easier.

they work just because meson-python fails to clean up the RPATH entries added by meson pointing to the build directory

It just occurred to me after writing this comment that this is verifiable, by building the test package as a wheel, then installing it and manually checking that the imports work (the build dir will no longer exist).

It just occurred to me after writing this comment that this is verifiable, by building the test package as a wheel, then installing it and manually checking that the imports work (the build dir will no longer exist).

The build RPATH entries added by Meson are relative to $ORIGIN and do not contain any absolute reference to the build directory. I think I've stated this multiple times now, and there is a long comment in one of the commits explaining this. Therefore, the imports fail only if the layout in the build directory is different from the layout in the in the install location. This is not the case for the current tests.

The tests are for case (1). You can't argue that tests for (1) are wrong because you're trying to fix case (2).

This PR touches different test packages. Some check for (1) and others check for (1) and (2), but succeed by accident. Some use install_rpath as they should (althooug, prior to this PR it has no effect). Some other rely on the bogus meson-python behavior. The tests that use install_rpath are anyhow relying on the bogus meson-python behavior because they use RPATH entries relative to $ORIGIN, which has no meaning on macOS and prior to this patch there is not provision in meson-python to translate $ORIGIN to @loader_path on macOS.

As I explained, this PR assumes that all RPATH entries already present before applying install_rpath that are relative to $ORIGIN (or @loader_path on macOS) are Meson build RPATH entries and must be removed. This is the reason why RPATH entries added with linker arguments (either via $LDFLAGS or via linker arguemts in meson.build and that are relative to $ORIGIN (or @loader_path on macOS) are removed. This is what needs fixing. I have a patch ready. The correct fix depends on mesonbuild/meson#14819

I know install_rpath: '$ORIGIN' was necessary and worked for SciPy

I don't know why you feel it is necessary to state this. I am nowhere questioning that adding RPATH entries is ncessary. However, before this PR, there is nothing in meson-python that applies install_rpath: '$ORIGIN' thus I cannot believe it makes a difference, and indeed, I don't find any install_rpath in the whole SciPy source code.

dnicolodi · 2025-02-15T11:28:02Z

This should fix all identified issue and it is ready for review. The failed tests are due to mesonbuild/meson#14254

rgommers · 2025-02-17T23:36:55Z

@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?

dnicolodi · 2025-02-18T16:50:54Z

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.

voidtrance · 2025-05-09T14:24:51Z

Hi, what is the status of this PR? Is install_rpath handled properly now? For reference: #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.

dnicolodi · 2025-05-25T12:48:10Z

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 · 2025-06-14T17:16:05Z

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

Always strip RPATH pointing to the build directory automatically added by meson at build time to all artifacts linking to a shared library built as part of the project. Before this was done only when the project contained a shared library relocated to .<project-name>.mesonpy.libs. Add the RPATH entry specified in the meson.build definition via the install_rpath argument to all artifacts. This automatically remaps the $ORIGIN anchor to @loader_path on macOS. This requires Meson 1.6.0 or later. Deduplicate RPATH entries. Fixes mesonbuild#711.

Rework the dependencies between the Python extension module and the package libraries so that they can be loaded with a single additional RPATH entry for each. The previous dependencies required two RPATH entries to be added to the extension module to find the two libraries installed in two different paths, however setting two RPATH entries is not possible via install_rpath. Meson version 1.6.0 or later is required for insrall_rpath to be present in meson introspection data. Drop test checking correct handling of RPATH set via linker arguments: modern Meson highly discourages doing this. Reorganizes the test package to a flatter layout that helps visualizing all the parts involved in the test.

rgommers · 2025-07-15T08:38:53Z

@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 · 2025-07-15T09:28:02Z

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 · 2025-07-15T09:28:26Z

tests/packages/link-against-local-lib/meson.build

@@ -9,7 +9,7 @@ if meson.get_compiler('c').get_id() in ['msvc', 'clang-cl', 'intel-cl']
    link_args = ['-DEXAMPLE_DLL_IMPORTS']
 else
    lib_compile_args = []
-    link_args = ['-Wl,-rpath,custom-rpath']


This test case needs to stay, it's important that this works.

I see a comment in the commit message:

Drop test checking correct handling of RPATH set via linker arguments: modern Meson highly discourages doing this.

Do you have a reference for this? I think that if this is really the case right now, that is misinformed - this is pretty essential for some distros.

I see the failures in gh-768, looking into them now.

Okay, the warning message is slightly misleading. It comes from here:

https://github.com/mesonbuild/meson/blob/0376d655694747ba36aa2bfd94e5bce2d2f1efa8/mesonbuild/build.py#L1156-L1162

The point seems to be to not use -Wl,-rpath inside a meson.build file. So the test needs changing to use LDFLAGS to avoid the warning.

If we do that in a separate PR, it will unblock gh-768 it looks like. Want me to do this?

This test case needs to stay, it's important that this works.

This is working. I'll drop this commit from the PR.

dnicolodi · 2025-07-15T17:39:02Z

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).

eli-schwartz · 2025-07-15T19:51:37Z

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?

rgommers · 2025-07-15T20:23:51Z

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.

eli-schwartz · 2025-07-15T21:38:02Z

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.

dnicolodi · 2025-07-15T22:18:26Z

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 · 2025-07-15T22:30:21Z

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.

rgommers · 2025-07-16T14:34:38Z

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.

dnicolodi force-pushed the install-rpath branch 9 times, most recently from cfad83c to 4f361a9 Compare February 13, 2025 22:31

dnicolodi commented Feb 13, 2025

View reviewed changes

dnicolodi force-pushed the install-rpath branch 14 times, most recently from 3c1b103 to 9951f6f Compare February 15, 2025 11:19

dnicolodi force-pushed the install-rpath branch from 9951f6f to 8420a34 Compare March 29, 2025 17:51

rgommers added the maintenance Regular code improvements that are not new features nor end-user-visible bugs label May 7, 2025

dnicolodi force-pushed the install-rpath branch from 8420a34 to 6d008e1 Compare May 25, 2025 12:46

dnicolodi added enhancement New feature or request and removed maintenance Regular code improvements that are not new features nor end-user-visible bugs labels May 25, 2025

dnicolodi added this to the v0.19.0 milestone May 25, 2025

dnicolodi mentioned this pull request Jun 14, 2025

TST: enable --fatal-meson-warnings for all tests #768

Open

dnicolodi added 2 commits June 14, 2025 19:16

MAINT: refactoring

2b992af

dnicolodi force-pushed the install-rpath branch from 6d008e1 to a67f31c Compare June 14, 2025 17:17

dnicolodi force-pushed the install-rpath branch from a67f31c to 4b4f9d5 Compare July 1, 2025 15:04

rgommers reviewed Jul 15, 2025

View reviewed changes

Fix support for install_rpath #724

Are you sure you want to change the base?

Fix support for install_rpath #724

Uh oh!

Conversation

dnicolodi commented Feb 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnicolodi Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnicolodi commented Feb 15, 2025

Uh oh!

rgommers commented Feb 17, 2025

Uh oh!

dnicolodi commented Feb 18, 2025

Uh oh!

voidtrance commented May 9, 2025

Uh oh!

dnicolodi commented May 25, 2025

Uh oh!

dnicolodi commented Jun 14, 2025

Uh oh!

rgommers commented Jul 15, 2025

Uh oh!

rgommers commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnicolodi commented Jul 15, 2025

Uh oh!

eli-schwartz commented Jul 15, 2025

Uh oh!

rgommers commented Jul 15, 2025

Uh oh!

eli-schwartz commented Jul 15, 2025

Uh oh!

dnicolodi commented Jul 15, 2025

Uh oh!

dnicolodi commented Jul 15, 2025

Uh oh!

rgommers commented Jul 16, 2025

Uh oh!

Uh oh!

Fix support for `install_rpath` #724

Fix support for `install_rpath` #724

dnicolodi Feb 14, 2025 •

edited

Loading