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

Fix support for install_rpath #724

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dnicolodi
Copy link
Member

No description provided.

@dnicolodi dnicolodi force-pushed the install-rpath branch 9 times, most recently from cfad83c to 4f361a9 Compare February 13, 2025 22:31
@@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dnicolodi dnicolodi Feb 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dnicolodi dnicolodi Feb 14, 2025

Choose a reason for hiding this comment

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

I suggest that we remove the shared library right next to the extension, and keep the one in sub.

It is a bit more convoluted to do, but to keep the test coverage on Windows, it is possible to make one library depend on the other and set install_rpath on the libraries accordingly. I'll look into how complex this gets.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

https://github.com/scipy/scipy/blob/e70ed9c96bdbc75aaf0fb5060b3cdaa8b0f6abdc/scipy/special/meson.build#L87

It certainly was necessary to use install_rpath: '$ORIGIN'. See also https://labs.quansight.org/blog/libsf-error-state (search for $ORIGIN).

You're saying that $ORIGIN doesn't work on macOS I think - how sure are you of that? It does not sound correct to me, IIRC @loader_path and @rpath are different, and the latter should be understanding $ORIGIN.

However, I think it is better to keep handling of install_rpath consistent between meson and meson-python.

Yes, agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just looking at that commit. I don't understand how that may work: install_rpath is handled by meson when meson install is called and meson-python does not do that...

Thinking about it, it works for the same reason that the test in meson-python worked before and it needs to be adjusted now: meson-python did not remove the rpath entries pointing to the build directory added by meson unless the package contains at least one library relocated to .<package-name>.mesonpy.libs. Therefore the SciPy modules never had their RPATH fixed and contained the build RPATH entry pointing to $ORIGIN or @loader_path depending on the platform. This PR corrects this and exposes the problem.

You're saying that $ORIGIN doesn't work on macOS I think - how sure are you of that? It does not sound correct to me, IIRC @loader_path and @rpath are different, and the latter should be understanding $ORIGIN.

I don't understand. @rpath is not recognized as a special root handle by the dynamic linker. It can be used in the identification name of a dynamic shared library. It is not another field in the search path where $ORIGIN can be substituted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the best way to deal with $ORIGIN vs @loader_path is to do the substitution in meson-python and emit a warning about it. This would avoid breaking SciPy and possibly other packages while bringing users to do the right thing.

@dnicolodi dnicolodi force-pushed the install-rpath branch 13 times, most recently from 01a6879 to 3c1b103 Compare February 15, 2025 11:10
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.

Reorganizes the test package to a flatter layout that helps
visualizing all the parts involved in the test.
@dnicolodi
Copy link
Member Author

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

@rgommers
Copy link
Contributor

@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
Copy link
Member Author

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.

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