auto-patchelf: add support for __structuredAttrs

[ConnorBaker]

Description of changes

As discovered in #256324, autoPatchelf doesn't work correctly when __structuredAttrs is true. This patch fixes that.

This PR is tagged with a CUDA label because it being merged allows refactoring the work-around hack in #256324 where autoPatchelfIgnoreMissingDeps was moved inside env to ensure it was not treated as an array.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[jonringer]

At first glance, this look right to me.

cc @jtojnar @infinisil @Mic92

[infinisil]

Can we have a test for this? 😃

[jonringer]

ran into this issue earlier... I'll try to find what package it was and get a minimal repo.

[ConnorBaker]

Can we have a test for this? 😃

I’d love to write one! Unfortunately I don’t know how to write tests for Nix things, much less bash in Nix things.

Do you have a reference you’d recommend that I could look at? That or any docs for how to do so would be greatly appreciated!

[SomeoneSerge]

Oh, if you wanted to test expected failures, I recently played with running nix-build from inside a nixosTest: 4e6972e

And expected successes don't even need a VM: you could e.g. generate a handful of derivations building dummy libraries. Then you pass them as buildInputs to another __structuredAttrs dummy derivation, which would build something linking dynamically the libraries from the previous step plus a few sonames to be ignored, strip their runpaths clean, and finally check that the hook has recovered the dummies and didn't fail at the missing SOs

[infinisil]

Yeah, see pkgs/test/default.nix, which is just recursive set of packages that get built by Hydra.

[jonringer]

ran into this issue earlier... I'll try to find what package it was and get a minimal repo.

Turns out I had a different issue with autoPatchelfHook...

[ConnorBaker]

I'm stuck :(

I don't have more time to devote to figuring out how to make tests for this. I've spent on the order of about eight hours struggling to figure out how to get this working (using #272752 (comment) as a guide), but wasn't able to produce anything which even evaluated.

I agree that there should be tests for this, but given that there aren't any already I can use as a reference, I don't think I have the skill or time necessary to write them.

Is the lack of tests a blocker for merging?

[infinisil]

Ah sorry no I don't think a VM test is necessary. We can just use a Nix derivation that uses both __structuredAttrs and autoPatchelfHook.

• Get some binary that needs autoPatchelfHook. Probably easiest to just search for an existing package in Nixpkgs. I found sslmate-agent to be a good one, since it has a small closure.
• Create pkgs/test/auto-patchelf-hook and add a copy of sslmate-agents Nix expression, but remove everything that's not necessary for the test, and add some installCheckPhase that verifies that the binary was patched successfully (you can use patchelf --print... for this).
• Then create another copy of that expression, but also set __structuredAttrs = true.
• Then create a pkgs/test/auto-patchelf-hook/default.nix that references both of those tests.
• Finally, include that default.nix from pkgs/test/default.nix with a recurseIntoAttrs like autoPatchelfHook = recurseIntoAttrs (callPackages ./auto-patchelf-hook { })
• Make sure both build successfully when you do nix-build -A tests.autoPatchelfHook (from the Nixpkgs root), ideally also do a quick check that when you break something intentionally, the check fails.

[yannham]

@infinisil thanks a lot for the detailed instructions! I set up something in e92f2dc following your advice. I built sslmate-agent with and without autoPatchelf to see the difference, and it seems that the only thing which is modified is the interpreter (no RUNPATH is set), so I wrote the test in consequence.

If I remove the autoPatchelf from the native build inputs of the test, the test fails as expected, so at least it checks that patching happens. However, if I mess a bit with autoPatchelf (for example reverting back to before this PR) the test continues to pass, so I'm not entirely convinced this really triggers the code path which checks this new functionality.

[infinisil]

Oh I see, that's not a great test case then, because most code paths aren't actually exercised. autoPatchelfHook is most useful when providing libraries as dependencies.

I looked around a bit more and found tonelib-jam to be a better example. Here's my recommendation that also sets some autoPatchelfHook flags to influence its behavior:

{ stdenv
, fetchurl
, autoPatchelfHook
, dpkg
, freetype
}:

stdenv.mkDerivation {
  name = "auto-patchelf-test";

  src = fetchurl {
    url = "https://tonelib.net/download/221222/ToneLib-Jam-amd64.deb";
    sha256 = "sha256-c6At2lRPngQPpE7O+VY/Hsfw+QfIb3COIuHfbqqIEuM=";
  };

  unpackCmd = "dpkg -x $curSrc source";

  nativeBuildInputs = [
    dpkg
    autoPatchelfHook
  ];

  installPhase = ''
    mv usr $out
  '';

  buildInputs = [
    freetype
  ];

  autoPatchelfIgnoreMissingDeps = [
    "libGL.so.1"
    "libasound.so.2"
  ];

  runtimeDependencies = [
    "/some/dep"
    # This might only work with __structuredAttrs!
    "/a dep with a space"
  ];
}

You'll have to add an installCheckPhase still, but now you can verify both the result of patchelf --print-interpreter and patchelf --print-rpath.

[yannham]

@infinisil done in the last commit (update via force push). This test looks more solid 🙂 and indeed the test fails if I add the path with spaces when __structureAttrs is set to false.

[ConnorBaker]

OfBorg failure (https://gist.github.com/GrahamcOfBorg/06df46e68204b4ce5b6d11e05b9fe30f) is unrelated and caused by the python package aioconsole.

Thank you @yannham for writing the test cases and dragging this over the finish line!

[github-actions]

Successfully created backport PR for staging-23.11:

• [Backport staging-23.11] auto-patchelf: add support for __structuredAttrs #275923

[vcunat]

Is this really safe for 23.11, i.e. sufficiently compatible? (PR #275923)