-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
auto-patchelf: refactor structuredAttrs support #340858
auto-patchelf: refactor structuredAttrs support #340858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with a single suggestion comment).
Hello! I'll take a look at this when I'm able, but don't let me approval be a blocker. I've noticed a lot of PRs targeting staging recently dealing with various aspects of support for |
There is no tracking issue, the discussion around this all happened in #318614. |
aab1529
to
57721a1
Compare
I updated an old tracking issue: #205690. If you do additional PRs, would you mind referencing it? Makes it easier to track related work (which will come in handy when I eventually need to update or troubleshoot the setup hooks the CUDA ecosystem uses). |
I haven't looked at it yet, but will try to do it by tomorrow. I originally wrote the tests for #272752 - I don't think they were supposed to fail, but I can't be 100% sure either honestly as it's been some time. Will at least take a quick look at the output of |
@wolfgangwalther Thanks for pinging me. The test is broken on master, and fails at I suspect it's a change in stdenv or something, but this simple patch should fix the test (at least it does on master and it's unrelated to what we're interested in testing): --- a/pkgs/test/auto-patchelf-hook/package.nix
+++ b/pkgs/test/auto-patchelf-hook/package.nix
@@ -60,7 +60,6 @@ stdenv.mkDerivation {
# Additional phase performing the actual test.
installCheckPhase =
let allDeps = runtimeDependencies ++ [
- (lib.getLib stdenv.cc.libc)
(lib.getLib freetype)
];
in If you want to make sure your change didn't break the test, given the one line diff, you can include the fix in this PR I guess. |
Fix suggested by Yann Hamdaoui in NixOS#340858 (comment).
stdenv now provides better tooling to support structuredAttrs without depending on $__structuredAttrs itself.
57721a1
to
bc0395e
Compare
Did that, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a few builds and everything came up roses. Let's merge this in the next day or so.
The Zig build hook appears to have been broken by these changes. |
Do you mean the autoPatchelfHook ones specifically, or a previous PR @jcollie? |
I'm not exactly sure which PR broke what, but I see this now when trying to build a Zig package:
|
This is very likely unrelated to this PR here. The Where (on which branch?) and how (any specific package to build?) can we reproduce this? |
This is unfortunately in a package that isn't open source (yet), but it was on unstable. I'll see if I can reproduce it tonight in another package that is open source. |
Description of changes
Follow up to #318614 to use
concatTo
insetup-hooks/auto-patchelf.sh
.This needs to support
*
in the non-structuredAttrs case, so adjustedconcatTo
for that.Can be tested via
nix-build -A tests.auto-patchelf-hook
. This will fail - but in the same way as on current master and the output clearly shows that flags are passed correctly. Not sure whether it is supposed to fail, though? CC @ConnorBaker and @yannham, who introduced it in #272752.CC @emilazy @philiptaron @tie
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.