rewrite autoPatchelfHook in python #149731

layus · 2021-12-08T22:09:29Z

Motivation for this change

Things done

Reimplemented autoPatchelfHook in python for readability, proper cache dicts and speed!

/cc @aszlig sorry for pinging twice in a row. I would really appreciate your feedback on the implementation. For now it resides in a separate hook for ease of testing. It should replace the old one before merge.

In particular, the logic is now more clear, but maybe not the right one. And addAutoPatchelfSearchPath semantincs is broken, but works with all nixpkgs examples.

aszlig

Thanks a lot for the effort! This btw. would probably also make it less ugly to use it standalone, extend it or even move it out of tree one day.

I added a few comments here and there, some of them are rather nit-picky and opinionated so feel free to ignore them.

Speaking of nit-picky: I'd also generally prefer to be in line with PEP-8, eg. using snake case instead of camelCase names for functions and variables. The reason is that it's more consistent with other Python code in nixpkgs and also matches that of most Python FOSS libraries out there.

Most of the comments also assume the use of pathlib.Path and I also usually tend use type annotations.

Anyway, as mentioned, these are just personal opinions.

If it comes to functionality: Have you actually tested this against software using autoPatchelfHook and checked whether it produces different results? Especially when it comes to androidenv, since the latter uses some of the provided shell functions we had so far.

pkgs/build-support/setup-hooks/auto-patchelf.py

pkgs/build-support/setup-hooks/auto-patchelf-py.sh

pkgs/build-support/setup-hooks/auto-patchelf.py

layus · 2021-12-10T23:32:09Z

I fail to properly test this on androidenv. Accessing the 25.* tools version is not trivial.

nix-review is funny: "1059 packages updated"

Artturin · 2021-12-22T06:06:04Z

Most reverse deps failed to build
Example log of 1password
https://termbin.com/fvs5

@nix { "action": "setPhase", "phase": "unpackPhase" }
unpacking sources
unpacking source archive /nix/store/h5h0j1cmfbngd64wp25ngw65zf0c0r1m-source
source root is source
@nix { "action": "setPhase", "phase": "patchPhase" }
patching sources
@nix { "action": "setPhase", "phase": "configurePhase" }
configuring
no configure script, doing nothing
@nix { "action": "setPhase", "phase": "buildPhase" }
building
no Makefile, doing nothing
@nix { "action": "setPhase", "phase": "installPhase" }
installing
@nix { "action": "setPhase", "phase": "fixupPhase" }
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2
shrinking /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2/bin/op
strip is /nix/store/xiq6j4jsyj351p8q3yw9cg1hdqp9m685-gcc-wrapper-10.3.0/bin/strip
stripping (with command strip and flags -S) in /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2/bin 
patching script interpreter paths in /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2
checking for references to /build/ in /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2...
automatically fixing dependencies for ELF files
{'ignoreMissing': False,
 'libs': [PosixPath('/nix/store/cz8pz8h26z4xab7asqnpds3mq50kr2vy-auto-patchelf-py-hook/lib'),
          PosixPath('/nix/store/pjba17qx8vh7dln8m5vnkqdbbjahvg9b-patchelf-0.13/lib'),
          PosixPath('/nix/store/59jmzisg8fkm9c125fw384dqq1np602l-move-docs.sh/lib'),
          PosixPath('/nix/store/kxw6q8v6isaqjm702d71n2421cxamq68-make-symlinks-relative.sh/lib'),
          PosixPath('/nix/store/m54bmrhj6fqz8nds5zcj97w9s9bckc9v-compress-man-pages.sh/lib'),
          PosixPath('/nix/store/bkxq1nfi6grmww5756ynr1aph7w04lkk-strip.sh/lib'),
          PosixPath('/nix/store/bnj8d7mvbkg3vdb07yz74yhl3g107qq5-patch-shebangs.sh/lib'),
          PosixPath('/nix/store/cickvswrvann041nqxb0rxilc46svw1n-prune-libtool-files.sh/lib'),
          PosixPath('/nix/store/8zxndz5ag0p6s526c2xyllhk1nrn4c3i-audit-tmpdir.sh/lib'),
          PosixPath('/nix/store/c8n9kcdddp9np665xz6ri61b383nxvz8-move-systemd-user-units.sh/lib'),
          PosixPath('/nix/store/1i5y55x4b4m9qkx5dqbmr1r6bvrqbanw-multiple-outputs.sh/lib'),
          PosixPath('/nix/store/kd4xwxjpjxi71jkm6ka0np72if9rm3y0-move-sbin.sh/lib'),
          PosixPath('/nix/store/fyaryjvghbkpfnsyw97hb3lyb37s1pd6-move-lib64.sh/lib'),
          PosixPath('/nix/store/ngg1cv31c8c7bcm2n8ww4g06nq7s4zhm-set-source-date-epoch-to-latest.sh/lib'),
          PosixPath('/nix/store/wlwcf1nw2b21m4gghj70hbg1v7x53ld8-reproducible-builds.sh/lib'),
          PosixPath('/nix/store/xiq6j4jsyj351p8q3yw9cg1hdqp9m685-gcc-wrapper-10.3.0/lib'),
          PosixPath('/nix/store/v7w0pspwq8r2b7k2sndxq3db843z7xm5-binutils-wrapper-2.35.2/lib'),
          PosixPath('/nix/store/cz8pz8h26z4xab7asqnpds3mq50kr2vy-auto-patchelf-py-hook/lib'),
          PosixPath('/nix/store/pjba17qx8vh7dln8m5vnkqdbbjahvg9b-patchelf-0.13/lib'),
          PosixPath('/nix/store/59jmzisg8fkm9c125fw384dqq1np602l-move-docs.sh/lib'),
          PosixPath('/nix/store/kxw6q8v6isaqjm702d71n2421cxamq68-make-symlinks-relative.sh/lib'),
          PosixPath('/nix/store/m54bmrhj6fqz8nds5zcj97w9s9bckc9v-compress-man-pages.sh/lib'),
          PosixPath('/nix/store/bkxq1nfi6grmww5756ynr1aph7w04lkk-strip.sh/lib'),
          PosixPath('/nix/store/bnj8d7mvbkg3vdb07yz74yhl3g107qq5-patch-shebangs.sh/lib'),
          PosixPath('/nix/store/cickvswrvann041nqxb0rxilc46svw1n-prune-libtool-files.sh/lib'),
          PosixPath('/nix/store/8zxndz5ag0p6s526c2xyllhk1nrn4c3i-audit-tmpdir.sh/lib'),
          PosixPath('/nix/store/c8n9kcdddp9np665xz6ri61b383nxvz8-move-systemd-user-units.sh/lib'),
          PosixPath('/nix/store/1i5y55x4b4m9qkx5dqbmr1r6bvrqbanw-multiple-outputs.sh/lib'),
          PosixPath('/nix/store/kd4xwxjpjxi71jkm6ka0np72if9rm3y0-move-sbin.sh/lib'),
          PosixPath('/nix/store/fyaryjvghbkpfnsyw97hb3lyb37s1pd6-move-lib64.sh/lib'),
          PosixPath('/nix/store/ngg1cv31c8c7bcm2n8ww4g06nq7s4zhm-set-source-date-epoch-to-latest.sh/lib'),
          PosixPath('/nix/store/wlwcf1nw2b21m4gghj70hbg1v7x53ld8-reproducible-builds.sh/lib'),
          PosixPath('/nix/store/xiq6j4jsyj351p8q3yw9cg1hdqp9m685-gcc-wrapper-10.3.0/lib'),
          PosixPath('/nix/store/v7w0pspwq8r2b7k2sndxq3db843z7xm5-binutils-wrapper-2.35.2/lib')],
 'paths': [PosixPath('/nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2')],
 'recursive': True,
 'runtime_dependencies': []}
searching for dependencies of /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2/bin/op
setting interpreter of /nix/store/fm04jy2csjav9wvwzcfr53s6wfbdp5kq-1password-1.12.2/bin/op
Traceback (most recent call last):
  File "/nix/store/lycra8danr1rkw20xp2igfj51bggj9a1-auto-patchelf.py", line 319, in <module>
    main()
  File "/nix/store/lycra8danr1rkw20xp2igfj51bggj9a1-auto-patchelf.py", line 296, in main
    autoPatchelf(
  File "/nix/store/lycra8danr1rkw20xp2igfj51bggj9a1-auto-patchelf.py", line 250, in autoPatchelf
    autoPatchelfFile(path, runtime_deps)
  File "/nix/store/lycra8danr1rkw20xp2igfj51bggj9a1-auto-patchelf.py", line 202, in autoPatchelfFile
    for dep in dependencies:
  File "/nix/store/lycra8danr1rkw20xp2igfj51bggj9a1-auto-patchelf.py", line 43, in get_dependencies
    for section in elf.iter_sections():
  File "/nix/store/mg6w3fh347i2axcc56kv7aswfqfphqyd-python3-3.9.6-env/lib/python3.9/site-packages/elftools/elf/elffile.py", line 133, in iter_sections
    yield self.get_section(i)
  File "/nix/store/mg6w3fh347i2axcc56kv7aswfqfphqyd-python3-3.9.6-env/lib/python3.9/site-packages/elftools/elf/elffile.py", line 112, in get_section
    section_header = self._get_section_header(n)
  File "/nix/store/mg6w3fh347i2axcc56kv7aswfqfphqyd-python3-3.9.6-env/lib/python3.9/site-packages/elftools/elf/elffile.py", line 526, in _get_section_header
    return struct_parse(
  File "/nix/store/mg6w3fh347i2axcc56kv7aswfqfphqyd-python3-3.9.6-env/lib/python3.9/site-packages/elftools/common/utils.py", line 39, in struct_parse
    stream.seek(stream_pos)
ValueError: seek of closed file

Report
https://termbin.com/1gr5

layus · 2021-12-22T09:31:43Z

@Artturin yes, fixed, thanks. I used a generator that kept a reference to a stream that was closed before the actual use of the generator.

A this stage I am quite confident that we are reaching the end of WIP. nix-review manages to build everything, except for some rare cases like discordchatexporter-cli which fails now because I am more eager at finding the interpreter file. It did not fail before because the package contains no path to fix. I kept the failure as I think we should not litter the code with autoPatchelfHooks. That is also why I added a warning when nothing was patched. I would like to make it an error, but there are surprisingly quite a few packages that use autoPatchelfHook with nothing to patch. I think most of these use cases are invalid, but there are some valid corner cases.

layus · 2021-12-22T10:59:17Z

@aszlig, @Artturin Would you mind giving this one a second pass ? I think it matured well enough. At least it builds a lot a lot of things correctly in nix-review.

Attention points:

I have changed the semantics of runtimeDirs to stop appending /lib at the end. It seems more flexible that way. Backward compatibility is preserved in the bash wrapper.
I do not call patchelf for reading. It may cause some discrepancies with the old behavior but 1. I am confident enough that I replicated the exact same behavior and 2. it actually helped me untangle the exact semantics of the old hook.
I added a warning when the hook does nothing. Should I make that louder ? Maybe add more empty lines around :-).

Style, correctness, and error handling. (for example, when NIX_BINTOOLS is unset, you get an ugly error trace:

  Traceback (most recent call last):
    File "/nix/store/bjj245b9pa7d4n9bdn758ks4m04rp9gg-auto-patchelf.py", line 311, in <module>
      nix_support = Path(os.environ['NIX_BINTOOLS']) / 'nix-support'
    File "/nix/store/k0z9n599k02hab8qjjp3ljw065iwjcvg-python3-3.9.6/lib/python3.9/os.py", line 679, in __getitem__
      raise KeyError(key) from None
  KeyError: 'NIX_BINTOOLS'

Also, the phase start header is not printed, because it only occurs after that error. Should we move the header to the wrapper script ?

layus · 2021-12-22T11:00:42Z

P.S. I am not against type annotations, but I did not manage to get Tuple to work for example.

layus · 2021-12-22T11:24:18Z

And another fix 🎉. Does this ever end ?

aszlig

Mostly nitpicks from me, however I'm adding @svanderburg to the reviewers since he's built something similar for patching the Android SDK back then, which is why autoPatchelfHook can be used in a standalone way in order to support that use case. Since I'm not familiar with Android SDK packaging in nixpkgs, he's a better canidate for making sure that it still works after we merge this very pull request.

See also his blog post, particularly the Follow up section.

pkgs/build-support/setup-hooks/auto-patchelf.py

DavHau · 2022-02-03T18:56:20Z

For maximum performance it would be cool if patching the individual files would be handled by a thread pool or process pool. But I am happy to do this in a follow up PR.

FRidh · 2022-02-04T09:08:57Z

If we do run into issues with say the sdk and it is not easily fixable we can always revert.

layus · 2022-02-04T12:32:56Z

Thanks everyone for the review process. It has been a pleasant experience.

After being rewritten in NixOS#149731, this hook can fail on Mach-O binaries. Since patching ELF files on Darwin doesn't make much sense anyway, we'll mark this as Linux-exclusive.

With the re-implementation in Python merged[1], it no longer makes sense for me to track issues and pull requests. I did this originally because people were forgetting (rightfully so) to run tests against all that proprietary stuff we have in nixpkgs that is using autoPatchelfHook. We still can't test these automatically but with me no longer being the author of the code, I hereby drop my entry in CODEOWNERS and instead replace it with layus, who's the author of the rewrite. [1]: #149731 Signed-off-by: aszlig <aszlig@nix.build>

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 8, 2021

aszlig reviewed Dec 9, 2021

View reviewed changes

aszlig reviewed Dec 10, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

aszlig reviewed Dec 10, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Dec 11, 2021

layus requested review from aszlig and Artturin December 22, 2021 10:48

layus marked this pull request as ready for review December 22, 2021 11:00

layus requested a review from Ericson2314 as a code owner December 22, 2021 11:00

aszlig reviewed Dec 25, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

aszlig requested a review from svanderburg December 25, 2021 00:40

Mic92 reviewed Dec 26, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

Mic92 reviewed Dec 26, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

Mic92 reviewed Dec 26, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

Mic92 reviewed Dec 26, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

Mic92 reviewed Dec 26, 2021

View reviewed changes

pkgs/build-support/setup-hooks/auto-patchelf.py Outdated Show resolved Hide resolved

FRidh merged commit 7b9fd5d into NixOS:staging Feb 4, 2022

layus deleted the autoPatchelfHook-rewrite branch February 4, 2022 12:32

layus mentioned this pull request Feb 6, 2022

autoPatchelfHook contains unused code intended for optimisation #148547

Closed

OPNA2608 mentioned this pull request Mar 12, 2022

treewide: autoPatchelfHook / fixDarwinDylibNames only on Linux / Darwin #163924

Merged

13 tasks

whentze mentioned this pull request Mar 24, 2022

GraalVM builds broken for x86_64-darwin on unstable #165670

Closed

thiagokokada mentioned this pull request Mar 28, 2022

rar: fix build on darwin, 6.0.2 -> 6.11 #166055

Merged

13 tasks

felschr mentioned this pull request Mar 28, 2022

breitbandmessung build fails due to missing command: autoPatchelfFile #166086

Closed

layus mentioned this pull request Mar 28, 2022

adapt semantics of autopatchelfHook for single files #166118

Closed

This was referenced Mar 29, 2022

ci: use autoPatchelfHook from pinned nixpkgs for emulators trezor/trezor-firmware#2194

Merged

Fix patching of library paths of old emulator binaries trezor/trezor-firmware#2195

Closed

aszlig mentioned this pull request Apr 14, 2022

CODEOWNERS: Add layus for autoPatchelfHook #168597

Merged

Yarny0 mentioned this pull request May 10, 2022

tsm-client: 8.1.14.0 -> 8.1.15.0 & fix patching rpath with autoPatchelf #172372

Merged

13 tasks

Yarny0 mentioned this pull request Jan 15, 2023

autoPatchelfHook: fix symlink handling #210908

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite autoPatchelfHook in python #149731

rewrite autoPatchelfHook in python #149731

layus commented Dec 8, 2021

aszlig left a comment •

edited

Loading

layus commented Dec 10, 2021 •

edited

Loading

Artturin commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

aszlig left a comment

DavHau commented Feb 3, 2022

FRidh commented Feb 4, 2022

layus commented Feb 4, 2022

rewrite autoPatchelfHook in python #149731

rewrite autoPatchelfHook in python #149731

Conversation

layus commented Dec 8, 2021

Motivation for this change

Things done

aszlig left a comment • edited Loading

Choose a reason for hiding this comment

layus commented Dec 10, 2021 • edited Loading

Artturin commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

layus commented Dec 22, 2021

aszlig left a comment

Choose a reason for hiding this comment

DavHau commented Feb 3, 2022

FRidh commented Feb 4, 2022

layus commented Feb 4, 2022

aszlig left a comment •

edited

Loading

layus commented Dec 10, 2021 •

edited

Loading