-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
treewide: support structuredAttrs in setup hooks #318614
treewide: support structuredAttrs in setup hooks #318614
Conversation
7aa43fa
to
b4bc78e
Compare
Hm, so @AndersonTorres is listed as maintainer for Do I need to mention all maintainers manually in this case? I assumed ofborg would also work by commit prefix |
b4bc78e
to
6ef4018
Compare
6ef4018
to
db773bd
Compare
Rebased to resolve merge conflicts. Anyone up for a review of the first five |
We'll have to look out to ensure nobody else starts to depend on the stdout-based |
Really great to see this land! Looking forward to the follow‐up for the other hooks. Let’s all do our best to get structured attributes on by default as soon as we can. |
Bisect says the bfd97a6
This happenens because configureFlagsArray = [
"--with-passprompt=[sudo] password for %p: " # intentional trailing space
]; It used to be passed as a single string, but now it gets expanded to an argument list. |
I guess we just need to restore the special handling of |
Most peculiar, I'm surprised this used to work because this does not seem to be a EDIT: it probably worked because |
I guess just toggling on |
👍🏻 Two separate matters: make sudo structuredAttrs, add a warning/error out-of-tree users when |
FWIW I think the way EDIT:
I'm probably free in a couple of hours |
With #334973 merged, I think we're safe for staging-next. Anyone disagree? |
The previously used pattern was introduced in NixOS#318614, but technically leaked the default flags into the global scope. While this would probably not make much of a practical difference, making concatTo support default values is a much cleaner approach.
cudaPackages already builts with structuredAttrs, but the cmakeFlags+= pattern incorrectly appends the additional flags to the first array argument with a space - which is now part of that argument itself since NixOS#318614, which added support for structuredAttrs to cmake.
Description of changes
I tried to build a package with
__structuredAttrs
which uses meson, realized that meson setup hooks don't support structuredAttrs, yet, and then... this happened.The first 5 commits make the helpers in
stdenv
related to structuredAttrs a bit better re-usable. The remaining commits are using the "new"concatTo
andprependToVar
/appendToVar
to fix many setup hooks.The main goal here is to support structuredAttrs in those setup hooks without having any "if structuredAttrs then ..." conditionals except for a few in
stdenv
. I think this works pretty well.There's a lot of changes in here, but many of them should be very easy to review. Most of them were tested by building a package using the related hooks: Once with
structuredAttrs
on master, which would fail, then with and withoutstructuredAttrs
on this branch - both would pass.In any case, I pushed all those commits mostly to show how those helpers in
stdenv
would be used across the different hooks. I am happy to split up the PR into smaller pieces later and would welcome feedback especially on the first 5 commits.The problem in 99% of those hooks is that
$xxxFlags
is used without[@]
, which will always just return the first array item when usingstructuredAttrs
. However a simple change to$xxxFlags[@]
isn't going to fly, if whitespace should be treated properly.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.