meson: fix rpath fixup #35513
Merged
meson: fix rpath fixup #35513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GrahamcOfBorg
added
6.topic: GNOME
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
dezgeg
reviewed
Mar 4, 2018
| paths += ':' | ||
| paths += build_rpath | ||
| - if len(paths) < len(install_rpath): | ||
| - padding = 'X' * (len(install_rpath) - len(paths)) |
There was a problem hiding this comment.
Shouldn't hardcode /nix/store here, rather substituteAll the $NIX_STORE shell variable here.
There was a problem hiding this comment.
Do you mean substituteAlling builtins.storePath, or reading NIX_STORE environment variable at runtime?
There was a problem hiding this comment.
I meant the value of $NIX_STORE during build-time, but the builtins.storeDir Nix variable seems to work as well.
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
Merged
jtojnar
force-pushed
the
meson-rpath-fix
branch
from
3318bb1
to
df33000
Compare
|
Rebasing onto master as it will be merged with GNOME 3.28. |
|
Success on x86_64-darwin (full log) Attempted: meson The following builds were skipped because they don't evaluate on x86_64-darwin: deja-dup, epiphany, gnome3.gnome-shell, systemd Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: deja-dup, epiphany, gnome3.gnome-shell, meson, systemd Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: deja-dup, epiphany, gnome3.gnome-shell, meson, systemd Partial log (click to expand)
|
In common distributions, RPATH is only needed for internal libraries so meson removes everything else. With Nix, the locations of libraries are not as predictable, therefore we need to keep them in the RPATH. [1] Previously we have just kept the RPATH produced by the linker, patching meson not to remove it. This deprived us of potentially replacing it with install_rpath provided by project so we had to re-add it manually, and also introduced a vulnerability of keeping build paths in RPATH. This commit restores the clean-up but modifies it so the items starting with /nix/store are retained. This should be relatively safe since the store is immutable, however, there might be some unwanted retainment of build_rpath [2] if it contains paths from Nix store. [1]: NixOS#31222 (comment) [2]: http://mesonbuild.com/Release-notes-for-0-42-0.html#added-build_rpath-keyword-argument
jtojnar
force-pushed
the
meson-rpath-fix
branch
from
df33000
to
de75940
Compare
GrahamcOfBorg
added
10.rebuild-darwin: 0
10.rebuild-linux: 0
and removed
6.topic: GNOME
|
This belive this broke the systemd-cryptsetup-generator The binary is no longer able to find @jtojnar Any idea how to fix this? |
|
Meson fixes the rpath during |
|
The problem is the cyclic dependency this would introduce: It comes from here. |
|
Note that |
|
For now, you could re-add the removed |
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
In common distributions,
RPATHis only needed for internal libraries so meson removes everything else. With Nix, the locations of libraries are not as predictable, therefore we need to keep them in theRPATH. See #31222 (comment) for more details.Previously we have just kept the
RPATHproduced by the linker, patching meson not to remove it. This deprived us of potentially replacing it withinstall_rpathprovided by project so we had to re-add it manually, and also introduced a vulnerability of keeping build paths inRPATH.This patch restores the clean-up but modifies it so the items starting with
/nix/storeare retained.This should be relatively safe since the store is immutable, however, there might be some unwanted retainment of
build_rpathif it contains paths from Nix store.Closes: #31222
Things done
DT_NEEDEDitems.wlrootsthey use a different fixup, let them (@primeos) clean it up themselvesbuild-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)cc @brandonedens @rasendubi @yegortimoshenko