autoPatchelfHook: do not patch statically linked files #47222
Conversation
|
Success on x86_64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
What does "statically linked" mean exactly? I'm surprised ofborg says this isn't a mass rebuild. |
|
This is not a mass-rebuild because we only use autoPatchelfHook for programs that we do not compiled from source. Many of them also have an unfree license. Statically linked means it does not use a runtime linker (e.x.: ld-linux-x86-64.so.2). patchelf will fail on those files and it also does not make any sense to set the RPATH on statically linked files because there are not libraries loaded. |
There was a problem hiding this comment.
I'd move the detection to findElfs instead. In addition I think using file for detecting ELFs is not a very good idea in the first place and we should change the detection method in findElfs altogether, because some magic files tend to have quite expensive search operations.
|
What else should be used instead of |
|
@Mic92 there is a n |
|
@Ericson2314: Hm, looking at that one, it looks a bit too fuzzy to me, for example it will return success if you have a text file like this: So IMHO something like |
|
@Ericson2314: Replacing |
|
@aszlig proposed the following patch on irc: I will combine this with the stdenv's native isElf patch later. |
|
I don't disagree with improving |
|
patchelf does not work on windows/macOS so |
Mic92
force-pushed
the
auto-patchelf
branch
from
b45e1f6
to
4e65e73
Compare
|
Success on x86_64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
| findElfs "$prefix" | while read -r elffile; do | ||
| autoPatchelfFile "$elffile" | ||
| done | ||
| while IFS= read -r -d $'\0' file; do |
There was a problem hiding this comment.
taken from here: http://mywiki.wooledge.org/BashFAQ/020
There was a problem hiding this comment.
The page says it is required.
There was a problem hiding this comment.
Ah, you're right, otherwise it's trimmed by IFS.
| autoPatchelfFile "$elffile" | ||
| done | ||
| while IFS= read -r -d $'\0' file; do | ||
| if isELF "$file"; then |
There was a problem hiding this comment.
Maybe even:
isELF "$file" || continue
readelf -l "$file" | grep -q '^ *INTERP\>' || continue
autoPatchelfFile "$file"IMHO makes it less "pyramidic".
Also speed up quite significantly due less forking.
Mic92
force-pushed
the
auto-patchelf
branch
from
4e65e73
to
58a97df
Compare
|
regarding isELF, I opened: #47249 |
|
Success on x86_64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: autoPatchelfHook Partial log (click to expand)
|
|
Going to test (and if successful, merge) this later on all of the usages in nixpkgs, verifying whether their contents are still the same. |
|
fair enough. |
|
@Mic92: Status update: Something is still wrong here (I did the diffs with Some of these differences look a bit odd as well (might be Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: DYN (Shared object file)
Machine: Advanced Micro Devices X86-64
Version: 0x1
Entry point address: 0x44380
- Start of program headers: 7983104 (bytes into file)
+ Start of program headers: 64 (bytes into file)
Start of section headers: 7977872 (bytes into file)
Flags: 0x0
Size of this header: 64 (bytes)
Size of program headers: 56 (bytes)
- Number of program headers: 8
+ Number of program headers: 7
Size of section headers: 64 (bytes)
Number of section headers: 31
- Section header string table index: 28
+ Section header string table index: 30 |
|
Ah, damn... of course... we now check for |
If the ELF file is not an executable, we do not get a PT_INTERP section, because after all, it's a *shared* library. So instead of checking for PT_INTERP (to avoid statically linked executables) for all ELF files, we add another check to see if it's an executable and *only* skip it when it is and there's no PT_INTERP. Signed-off-by: aszlig <aszlig@nix.build>
The "maxx" package recursively runs isExecutable on a bunch of files and since the change to use "readelf" instead of "file" a lot of errors like this one are printed during build: readelf: Error: Not an ELF file - it has the wrong magic bytes at the start While the isExecutable was never meant to be used outside of the autoPatchelfHook, it's still a good idea to silence the errors because whenever readelf fails, it clearly indicates that the file in question is not a valid ELF file. Signed-off-by: aszlig <aszlig@nix.build>
The unfree variant of elasticsearch uses autoPatchelfHook and since we removed the dependency on file for the hook itself in 58a97df we no longer have zlib propagated. So we need to explicitly state that dependency here. Signed-off-by: aszlig <aszlig@nix.build> Cc: @apeschar, @basvandijk
|
Success on aarch64-linux (full log) Attempted: autoPatchelfHook The following builds were skipped because they don't evaluate on aarch64-linux: elasticsearch Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: autoPatchelfHook The following builds were skipped because they don't evaluate on x86_64-darwin: elasticsearch Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: autoPatchelfHook The following builds were skipped because they don't evaluate on x86_64-linux: elasticsearch Partial log (click to expand)
|
This includes the initialy commit was done by @Mic92 plus a few fixes from my side. So essentially this avoids patching statically linked executables and also speeds up searching for ELF files altogether. I've tested this by comparing the outputs of all the derivations which make use of this hook using the following Nix expression: let getPackagesForRev = rev: with import (builtins.fetchGit { url = ./.; inherit rev; }) { config.allowUnfree = true; }; [ cups-kyodialog3 elasticsearch franz gurobi javacard-devkit masterpdfeditor maxx oracle-instantclient powershell reaper teamviewer unixODBCDrivers.msodbcsql17 virtlyst wavebox zoom-us ]; pkgs = import <nixpkgs> {}; baseRev = "ef764eb0d8314b81a012dae04642b4766199956d"; in pkgs.runCommand "diff-contents" { chset = pkgs.lib.zipListsWith (old: new: pkgs.runCommand "diff" { inherit old new; nativeBuildInputs = [ pkgs.nukeReferences ]; } '' mkdir -p "''${NIX_STORE#/}" cp --no-preserve=all -r "$old" "''${NIX_STORE#/}" cp --no-preserve=all -r "$new" "''${NIX_STORE#/}" find "''${old#/}" "''${new#/}" \ \( -type f -exec nuke-refs {} + \) -o \( -type l -delete \) mkdir "$out" echo "$old" > "$out/old-path" echo "$new" > "$out/new-path" diff -Nur "''${old#/}" "''${new#/}" > "$out/diff" || : '') (getPackagesForRev baseRev) (getPackagesForRev ""); } '' err=0 for c in $chset; do if [ -s "$c/diff" ]; then echo "$(< "$c/old-path") -> $(< "$c/new-path")" \ "differs, report: $c/diff" >&2 err=1 fi done [ $err -eq 0 ] && touch "$out" '' With these changes there is only one derivation which has altered contents, which is "franz". However the reason why it has differing contents is not directly because of the autoPatchelfHook changes, but because the "env-vars" file from the builder is in "$out/opt/franz/env-vars" (Cc: @gnidorah) and we now have different contents for NIX_CFLAGS_COMPILE and other environment variables. I also tested this against a random static binary and the hook no longer tries to patch it. Merges: #47222
Motivation for this change
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)