-
Notifications
You must be signed in to change notification settings - Fork 77
ENH: do not run 'meson install' to produce a wheel #340
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
Conversation
There is no need to have the files copied in the installation directory: they are read from the source and build trees. This removes the concept of a Project working directory containing the installation directory and the build directory (unless the user specifies one). A Project now only has a source directory and a build directory. Adapt the tests accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a code simplification, so useful in principle. My initial question here is why this doesn't break RPATH handling. Meson adds RPATH entries to binaries in the build dir, and strips those off again in meson install
. We do have a test for this in test_wheel.py
and apparently CI is happy with that. I'm not sure if the test captures everything we need here though - my understanding was that running meson install
was necessary. I'll try to test this.
After looking at this in more detail, I think our existing tests don't capture everything here.
|
I think that for the meson-python use case it is more relevant to know which rpath entries Meson adds, more than the ones it removes: we need to make sure that any rpath added by Meson to execute from the build directory does not make into the wheel, but we want to preserve any other rpath set by the user or required because of non-standard location of dependencies, although the latter types are problematic for redistributable wheels. |
Even more importantly, meson-python copies into the wheel the build artifacts from the build directory, not from the install directory, thus any transformation of the rpath applied by Meson at install time is not reflected into the files that are actually shipped. Nothing in this respect changes with this patch. |
Yes indeed, that sounds right to me. That's why I thought we needed the install step - to allow Meson to remove whatever it added (that's not something we really should have to keep track of). But I'm surprised that we're not taking the installed files to put into the wheel; that got changed somewhere along the way then I think, and apparently nobody noticed so it's probably fine. |
I've tried to see whether this changed recently, but unless I missed the pair of commits that fixed this and broke this again going through the git history, things have been this way since the beginning of times. |
If we determine that the rpaths pointing to the build directory (well, not really, the rpaths added by Meson are all relative, thus once installed in a wheel the rpaths point somewhere into Python's platlib) need to be removed, we can approach this in three ways: keep the Meson installation phase and pick the files that go into the wheel from the installed tree, use the machinery we already have to amend rpaths to remove them, ask Meson not to add them. The latter is the easiest solution, from the point of view of meson-python. But requires cooperation from the Meson side. By the way, not that it is indicative of much, but cmake has such an option. Keeping the install phase only to strip the rpaths seems a waste. Removing the rpaths ourselves requires knowing which rpaths to remove. Meson computes the rpaths to remove determining which rpaths were added explicitly by the user and removing any other ones present in the object files. This is done this way because rpaths are deduplicated when set, thus if the user set an rpath that coincides with one added by Meson, there is no other way to know if it should be kept at install time or not. I don't think we want to replicate the same logic in meson-python. Meson could provide this information in an introspection file. But if we need to add something to Meson, we could very well add an option to do not add the rpaths. I've also considered stripping all rpaths that point to a relative path inside platlib, because these make little sense in libraries and extension modules shipped in a Python wheel and we add an rpath pointing to the meson-python special location dedicated to shared libraries distributed with the wheel. However, there may be developers that like to distribute their libs next to the extensions modules and set rpath accordingly, and I don't think we should break this use case. @eli-schwartz What do you think? |
IIRC, meson adds both install rpaths and build rpaths (including automatically calculated ones). Then during install, it removes the latter. The idea here is to make sure that the built artifacts actually work when running locally from the build directory, for example when running tests. This is not quite such useful functionality when dealing with filesystem tree dependent projects such as python code, e.g. SciPy's
I think that "we want to install it ourselves, so it doesn't go through the rpath install-rewriter" is the first example of a case where someone has ever wanted to not have those rpaths. Which probably explains why no one implemented such a cmake-like option for meson (I'm aware of the cmake option, but was never really sure when I'd want to use it). "Lack of a compelling reason" is definitely something feasible to change. :) |
If we were to add such an option to Meson, would it be in the form of a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look, following this PR, and it doesn't seem like we are copying the files from the install directory for some reason, which a problem not only for the RPATH, but also for install_subdir
, because Meson does not include the exclusion options. I opened #344 documenting it.
Once Meson provides alternative mechanisms to handle these two situations, we can make this change, but at the moment, not running meson install
is not an option.
I am gonna temporarily close this PR, as given the current status quo does not allow us to make such change, but we should revisit when the issues that prevent us from making this change get resolved. |
There is no need to have the files copied in the installation directory: they are read from the source and build trees. This removes the concept of a Project working directory containing the installation directory and the build directory (unless the user specifies one). A Project now only has a source directory and a build directory. Adapt the tests accordingly.