Bugfix: Generated binaries should not depend on devel package paths#14426
Bugfix: Generated binaries should not depend on devel package paths#14426hpkfft wants to merge 2 commits into
Conversation
|
The motivation here sounds quite wrong. If you're distributing libadd to hundreds of machines and the administrator of each machine can choose where to install it, because it's a relocatable package and uses The only correct option is to evaluate the pcfiledir pkg-config variable in the text used for rpath, with the rpath token |
|
In this scenario, there's only one administrator, who creates two disk images (one for the build machine and one for the deployment machines). The only difference between the two images is whether or not Sorry, I'm not really understanding your suggestion. The As the developer of The dependency code for pkgconfig already converts relative paths to absolute paths. |
|
With this PR: The executable |
No, if the pkg-config path is relocatable, then the library is intended to be moved around. Turning that into an absolute path would break lots of usages. Instead, your app should make sure that it lives where the library expects you to be (by looking at $ORIGIN). If you can't do that, then you need to use LD_LIBRARY_PATH. |
|
The meson code in According to the comments, it does this because, "We need the full path of the library to calculate RPATH values" and because, "De-dup of libraries is easier when we have absolute paths." If the library moves around, it seems to me that my patch doesn't make the problem any worse. Do you see a use case for which the absolute path with the Certainly, if the library is moved around from one machine to the next, then whoever is running the application would need to use I do not see how
The third-party math library has no idea that It seems common for third-party libraries to ship their product in two pieces, e.g., I would like to run on the build machine to have I think the same thing happens with Kubernetes deployments--a minimized container image without |
|
Rebased on top of master. |
|
Rebased on top of master so the CI checks pass. |
|
Rebased on top of master. |
|
Rebased. |
|
Rebased. |
|
Rebased. |
|
Rebased. |
d8871e0 to
c130c96
Compare
|
Rebased. |
d71ae0b to
36048e9
Compare
|
Summing up the various comments:
So I am going to merge this in a few days, but leaving time to comment. |
|
To be perfectly clear, no meson does not do that. (Unless you are running the binary uninstalled, inside the build directory, in which case meson adds many rpaths that don't make any sense at all, because it's more reliable than knowing which rpaths are required and which ones aren't.) Any rpaths that meson doesn't remove during
There has been a serious suggestion or three, to have |
eli-schwartz
left a comment
There was a problem hiding this comment.
General commentary on something that repeatedly bugs me across many PRs.
Right - more precisely, this patch only affects which absolute paths are created and not which files (whether uninstalled or installed) have absolute paths. Even for uninstalled binaries, I guess you could build one a machine that has the dev package, and run t on another machine that mounts the same home directory at the same location but only has the runtime package. So it boils to:
And anyway I'm moving it to 1.13 given the other comments. |
Yes, that is the one argument I personally find tempting. But it feels slightly weak if it's not really fixing anything by construction (albeit perhaps is more convenient for specific individuals), while adding even more filesystem stat calls and adding workarounds to the testsuite whose purpose we will never remember down the line. |
Uninstalled. Supercomputers have an interactive front-end node (a build machine) used for code development. Researchers develop and compile their code, If
It is fixing something by construction, as described above. It adds filesystem stat calls to |
That is exactly the problem. :( Making If we discard that from consideration then the uninstalled binaries aren't designed to run on many machines, just the one where the build is running. Even the rpaths we do add are a private implementation detail, solely so that But they are a private implementation detail, and we could someday change it if we felt there were compelling advantages for our supported use cases. I don't anticipate that happening, to be fair. There are absolutely some projects that only work inside of The rpaths are a security vulnerability by default because other machines may have /home/eschwartz/git/fooproject/builddir/src belong to someone else, who can sideload malware and have you run it with your unix permissions / ACLs / selinux contexts. I have no idea whether this matters to workflows that involve submitting jobs to a compute node! If the user accounts and uids and homedirs are guaranteed to be identical and consistent across nodes then likely it's fine.
? Again, it's not fixing it in the sense that meson is designed to support the use case of copying binaries onto another machine without installing them. Changing the style of RPATH entries doesn't make that part of the design. If it's not a designed scenario then we are in the realm of "maybe it coincidentally works, and we don't mind that it works and presumably won't go out of our way to break it with no cause".
Maybe, maybe not. :D "meson setup is slow" is a real problem that some people have. I could see this being a problem for example in projects that have many dependencies and are already feeling that "setup time would be good to optimize pretty please". And adding stat calls to every external library path in order to check if it can be resolved, will affect every dependency lookup for all users of meson. The vast majority of binaries won't have anything to resolve in the generated RPATH, so, running your |
|
AFAICT, os.path.realpath isn't even an interesting scenario for your use case, you only need os.path.normpath (that is, collapse As I already pointed out I don't like the use of pathlib.Path concrete methods, can we seize the opportunity in moving to os.path, to also skip symlink canonicalization and just flatten the use of It will look prettier, avoid encoding dependencies on pkgconfig/ directories, and as an abstract op the standard library implementation won't need to stat the filesystem. |
Yes, that's all I want to accomplish!
Yeah, I see your point. But |
d5791f3 to
b99452e
Compare
|
The Gentoo test failure appears unrelated: Please take another look at this PR. |
I think the best mental model is that of shared (for example, NFS-mounted) home directories, where you build on a large machine but then test on something that has the hardware you need. |
Prefix library paths specified by -L are converted to canonical paths
to improve the library runpath stored in the binary output.
For example, if `pkg-config --libs add` returns the following:
-L/usr/local/lib/pkgconfig/../../lib -ladd
we convert the path so that it is as if it had returned:
-L/usr/local/lib -ladd
Co-authored-by: Eli Schwartz <eschwartz93@gmail.com>
|
Rebased. |
Prefix library paths specified by -L are converted to canonical paths to improve the library runpath stored in the binary output. For example, if
pkg-config --libs addreturns the following:we convert the path so that it is as if it had returned:
Motivation
Consider a third-party math library that is distributed as two packages for installation:
libaddandlibadd-devel.The first installs the following files (under
/optor/usr/localas the system administrator chooses):The second, which is the development package, installs the following files:
The file
add.pcis as follows:On a build machine, the sysadmin has installed both packages. But the application that uses this math library will be deployed on hundreds of machines (virtual machines in the cloud, nodes on a supercomputer, docker containers, etc.). The system administrator only installs
libaddon those machines, notlibadd-devel, normeson,ninja, etc.The problem is that, without this patch, a
meson.buildfile containingwill create an application binary with the following library runpath:
and this won't run on the deployment nodes since the directory
/usr/local/lib/pkgconfigdoes not exist there.With this patch:
and all is well.