autoPatchelfHook: optimize performance; better error handling

[DavHau]

Motivation for this change

autoPatchelfHook has the following problems:

• quadratic complexity (very noticeable with larger amount of binaries)
• it can fail without stating any useful information about the problematic input. In fact the last error lines are very misleading since it looks like the last processed file could be the problem, while the error actually occured afterwards
• Finding and adding missing build inputs as a user is a slow process, since autoPatchelfHook fails immediately after hitting the first missing dependency and one has to run it over and over again, fixing only one input each time.
• invalid ELF files were causing autoPatchelfHook to crash.

Things done

• oraganize dependencies in an associative array for efficient lookups
• if patchelf fails, print an error message which states the exact file and command which has failed
• If dependencies are missing, first collect all missing dependencies, then fail with a message stating all missing dependencies
• believed valid ELF files are now first tested via a simple patchelf $file to make sure they are actually valid for patchelf.

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/mach-nix-create-python-environments-quick-and-easy/6858/79

[symphorien]

I'm not proficient enough in bash to review. All I can say is that if you feel brave enough, you can run shellcheck on the file :)

[DavHau]

Thanks for the hint.

I went through shellcheck now and got rid of the red ones and most of the orange ones. A few of them are definitely not satisfiable. But I think it is fine now. I tested autoPatchelfHook again and it worked.

BTW, wouldn't it be best to include shellcheck as a github check, since multiple people are referring to it?

Also I'm playing with the thought of re-implementing autoPatchelfHook some day in python and make use of multiprocessing (or async), since it is still quite slow after all.
Would that change we welcomed, or is introducing python as a depdendency a deal breaker?

[DavHau]

@Ericson2314 @SuperSandro2000
Anything left to do here?

[SuperSandro2000]

This rebuilds quite a lot. Can you please target staging?

[DavHau]

Targeted staging
@SuperSandro2000

[SuperSandro2000]

BTW, wouldn't it be best to include shellcheck as a github check, since multiple people are referring to it?

I would be definitely for it. Less work for me and the CI complains about a lot of stuff.

You could create an issue or if it is not to much work create a PR, too. It is important that only changed files are complained about.

Would that change we welcomed, or is introducing python as a depdendency a deal breaker?

I would prefer it if it would be kept bash only to reduce rebuilds and cyclic dependencies. Maybe you can utilize parallels or some bash multi thread tricks like storing pids and waiting until they are done. I can dig you up an examplke if you like.

[SuperSandro2000]

or better you suggest it here #104594 to be added to the list.

[DavHau]

Thanks for your comment. I added a proposal for shellcheck in the issue mentioned.
Keeping autoPatchelfHook in bash sounds reasonable considering the risk of circular dependencies.

The time might come where I'd like to optimize this further. But for now the most important issues are fixed I think.
Is there anything left to be done here?

Not sure if this helps, but this version of auto-patchelf.sh is used in mach-nix for some time now and I didn't receive any issues so far concerning .so dependencies.

[SuperSandro2000]

Is there anything left to be done here?

I am not sure and thats why I don't feel comfortable merging this.

[DavHau]

Github assigned 4 reviewers. One approved, one unresponsive and 2 not sure. No further comments given on what needs to be improved.
I'm kind of stuck here.
@FRidh, maybe you can help. I've seen you previously moved this to Needs Review in Staging.

[FRidh]

@DavHau like @symphorien I am not very competent with bash, so I can't review this. Otherwise, I do like the change!

Anyway, I will merge it. If it causes trouble it can always be reverted.

Also I'm playing with the thought of re-implementing autoPatchelfHook some day in python and make use of multiprocessing (or async), since it is still quite slow after all.
Would that change we welcomed, or is introducing python as a depdendency a deal breaker?

Some other scripts were converted from bash, but got some resistance and the feedback that they should be implemented in rust instead. I disagree with that it absolutely needs to be rust. You will get mixed responses.

Note you will always have a small part of bash considering the stdenv is in bash.

[eadwu]

This seems to break when ldd is run on non-native binaries (or at least the only change recently to patchelf I saw).

i.e. Native is x86_64
Breaks seem to occur with Intel 80386, version 1 (SYSV), ARM, EABI5 version 1 (GNU/Linux), which are all dynamically linked and ELF 32-bit LSB executable according to file.

[DavHau]

@eadwu, I'm not sure if I understand correctly. A binary which has been processes by autoPatchelfHook is afterwards broken, so that it cannot be read by ldd of another platform? Or is autoPatchelfHook failing during build?
Could you provide an example on how to reproduce this, or give some hints?

[eadwu]

nix-build -A vscode-extensions.ms-python.python.

If I remember correctly, it crashes in this step https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/setup-hooks/auto-patchelf.sh#L133

[numinit]

I can reproduce this crash with androidenv. Running with set -x, it seems like I get a failure when patching mksdcard:

++ isELF ./mksdcard
++ local fn=./mksdcard
++ local fd
++ local magic
++ exec
++ read -r -n 4 -u 11 magic
++ exec
++ '[' $'\177ELF' = $'\177ELF' ']'
++ return 0
+++ LANG=C
+++ readelf -l ./mksdcard
++ segmentHeaders='
Elf file type is EXEC (Executable file)
Entry point 0x8048640
There are 9 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x08048034 0x08048034 0x00120 0x00120 R E 0x4
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1
      [Requesting program interpreter: /lib/ld-linux.so.2]
  LOAD           0x000000 0x08048000 0x08048000 0x01ba0 0x01ba0 R E 0x1000
  LOAD           0x001f0c 0x0804af0c 0x0804af0c 0x0013c 0x40974 RW  0x1000
  DYNAMIC        0x001f20 0x0804af20 0x0804af20 0x000d0 0x000d0 RW  0x4
  NOTE           0x000168 0x08048168 0x08048168 0x00044 0x00044 R   0x4
  GNU_EH_FRAME   0x001934 0x08049934 0x08049934 0x00074 0x00074 R   0x4
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4
  GNU_RELRO      0x001f0c 0x0804af0c 0x0804af0c 0x000f4 0x000f4 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00
   01     .interp
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rel.dyn .rel.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
   03     .ctors .dtors .jcr .dynamic .got .got.plt .data .bss
   04     .dynamic
   05     .note.ABI-tag .note.gnu.build-id
   06     .eh_frame_hdr
   07
   08     .ctors .dtors .jcr .dynamic .got '
+++ echo '
Elf file type is EXEC (Executable file)
Entry point 0x8048640
There are 9 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x08048034 0x08048034 0x00120 0x00120 R E 0x4
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1
      [Requesting program interpreter: /lib/ld-linux.so.2]
  LOAD           0x000000 0x08048000 0x08048000 0x01ba0 0x01ba0 R E 0x1000
  LOAD           0x001f0c 0x0804af0c 0x0804af0c 0x0013c 0x40974 RW  0x1000
  DYNAMIC        0x001f20 0x0804af20 0x0804af20 0x000d0 0x000d0 RW  0x4
  NOTE           0x000168 0x08048168 0x08048168 0x00044 0x00044 R   0x4
  GNU_EH_FRAME   0x001934 0x08049934 0x08049934 0x00074 0x00074 R   0x4
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4
  GNU_RELRO      0x001f0c 0x0804af0c 0x0804af0c 0x000f4 0x000f4 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00
   01     .interp
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rel.dyn .rel.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
   03     .ctors .dtors .jcr .dynamic .got .got.plt .data .bss
   04     .dynamic
   05     .note.ABI-tag .note.gnu.build-id
   06     .eh_frame_hdr
   07
   08     .ctors .dtors .jcr .dynamic .got '
+++ grep '^Program Headers:'
++ '[' -n 'Program Headers:' ']'
++ isExecutable ./mksdcard
+++ LANG=C
+++ readelf -h -l ./mksdcard
+++ grep '^ *Type: *EXEC\>\|^ *INTERP\>'
++ isExeResult='  Type:                              EXEC (Executable file)
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1'
++ '[' -n '  Type:                              EXEC (Executable file)
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1' ']'
+++ echo '
Elf file type is EXEC (Executable file)
Entry point 0x8048640
There are 9 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x08048034 0x08048034 0x00120 0x00120 R E 0x4
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1
      [Requesting program interpreter: /lib/ld-linux.so.2]
  LOAD           0x000000 0x08048000 0x08048000 0x01ba0 0x01ba0 R E 0x1000
  LOAD           0x001f0c 0x0804af0c 0x0804af0c 0x0013c 0x40974 RW  0x1000
  DYNAMIC        0x001f20 0x0804af20 0x0804af20 0x000d0 0x000d0 RW  0x4
  NOTE           0x000168 0x08048168 0x08048168 0x00044 0x00044 R   0x4
  GNU_EH_FRAME   0x001934 0x08049934 0x08049934 0x00074 0x00074 R   0x4
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4
  GNU_RELRO      0x001f0c 0x0804af0c 0x0804af0c 0x000f4 0x000f4 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00
   01     .interp
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rel.dyn .rel.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
   03     .ctors .dtors .jcr .dynamic .got .got.plt .data .bss
   04     .dynamic
   05     .note.ABI-tag .note.gnu.build-id
   06     .eh_frame_hdr
   07
   08     .ctors .dtors .jcr .dynamic .got '
+++ grep '^ *INTERP\>'
++ '[' -n '  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1' ']'
++ patchelf ./mksdcard
++ autoPatchelfFile ./mksdcard
++ local dep rpath= toPatch=./mksdcard
++ local interpreter
++ interpreter=/nix/store/1yvpgm763b3hvg8q4fzpzmflr5674x4j-glibc-2.32-10/lib/ld-linux-x86-64.so.2
++ isExecutable ./mksdcard
+++ LANG=C
+++ readelf -h -l ./mksdcard
+++ grep '^ *Type: *EXEC\>\|^ *INTERP\>'
++ isExeResult='  Type:                              EXEC (Executable file)
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1'
++ '[' -n '  Type:                              EXEC (Executable file)
  INTERP         0x000154 0x08048154 0x08048154 0x00013 0x00013 R   0x1' ']'
++ runPatchelf --set-interpreter /nix/store/1yvpgm763b3hvg8q4fzpzmflr5674x4j-glibc-2.32-10/lib/ld-linux-x86-64.so.2 ./mksdcard
++ patchelf --set-interpreter /nix/store/1yvpgm763b3hvg8q4fzpzmflr5674x4j-glibc-2.32-10/lib/ld-linux-x86-64.so.2 ./mksdcard
++ '[' -n '' ']'
++ echo 'searching for dependencies of ./mksdcard'
searching for dependencies of ./mksdcard
++ runPatchelf --remove-rpath ./mksdcard
++ patchelf --remove-rpath ./mksdcard
++ local missing
+++ ldd ./mksdcard
+++ sed -n -e 's/^[\t ]*\([^ ]\+\) => not found.*/\1/p'
++ missing=
+ exitHandler
+ exitCode=1
+ set +e
+ '[' -n '' ']'
+ ((  1 != 0  ))
+ runHook failureHook
+ local hookName=failureHook
+ shift
+ local 'hooksSlice=failureHooks[@]'
+ local hook
+ for hook in "_callImplicitHook 0 $hookName" ${!hooksSlice+"${!hooksSlice}"}
+ _eval '_callImplicitHook 0 failureHook'
+ declare -F '_callImplicitHook 0 failureHook'
+ eval '_callImplicitHook 0 failureHook'
++ _callImplicitHook 0 failureHook
++ local def=0
++ local hookName=failureHook
++ declare -F failureHook
++ type -p failureHook
++ '[' -n '' ']'
++ return 0
+ return 0
+ '[' -n '' ']'
+ exit 1
error: --- Error --- nix-shell
builder for '/nix/store/bf1jj23rj4mgpc76lvqwmx52r0g3rkyl-androidsdk.drv' failed with exit code 1; last 10 log lines:
  ++ _callImplicitHook 0 failureHook
  ++ local def=0
  ++ local hookName=failureHook
  ++ declare -F failureHook
  ++ type -p failureHook
  ++ '[' -n '' ']'
  ++ return 0
  + return 0
  + '[' -n '' ']'

[numinit]

mksdcard (from an already patched version) is: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/2wrfwfdpklhaqhjxgq6yd257cagdxgph-glibc-2.32/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.24, BuildID[sha1]=119b9b83c17fa0228acfd582e951ea8b050e1229, not stripped

[Mic92]

So question is why this fails:

$ ldd ./mksdcard | sed -n -e 's/^[\t ]*\([^ ]\+\) => not found.*/\1/p'

[DavHau]

I found the problem.

local missing="$(...)"

seems to behave differently than

local missing
missing="$()"

[numinit]

@DavHau Yeah, Shellcheck will actually warn about that. Declaring a local and using $() in the same line will hide the return value.

[aszlig]

@FRidh: Can you please CC me next time before merging anything autopatchelf-related, since this has (apart from #106830) added a few merge conflicts of my WIP branches and I just found out about this.

[symphorien]

maybe add yourself as codeowner ?

[aszlig]

@symphorien: Thought about this already, but I still don't like the notion of "owner" in this regard (which to me would imply BDFL) and I only want to be notified. However, since the alternative would be to rely on everyone to do a git shortlog (or similar) and ofborg doesn't recognise setup hooks, I think I'm going for the codeowners file :-/