stdenv: concatTo: fallback for non-__structuredAttrs derivations#334973
stdenv: concatTo: fallback for non-__structuredAttrs derivations#334973philiptaron merged 2 commits intoNixOS:stagingfrom
Conversation
pkgs/stdenv/generic/setup.sh
Outdated
There was a problem hiding this comment.
The Bash looks fine to me. Is there any chance we could make the warning happen at evaluation time so it’s more visible?
There was a problem hiding this comment.
Huh. I guess we could add a warning in mkDerivation, and that'd also make Ofborg light up all (ab)users in Nixpkgs. The fallback, if we choose to go with the fallback and we probably do because of out-of-tree users, is still easiest to implement in bash though
There was a problem hiding this comment.
Yeah, the fallback is good. We should probably even warn in Bash too, because you could set the wrong thing in bash code. But a mkDerivation warning would be nice if it’s not too invasive.
There was a problem hiding this comment.
I think this should be using ${arr[@]} notation for *Array variables.
There was a problem hiding this comment.
The sudo had an issue because it set *Array variable as a derivation argument, so it got exported as a string and with ${var[@]} expansion strings are treated as single-element arrays.
There was a problem hiding this comment.
Is there any chance we could make the warning happen at evaluation time so it’s more visible?
Never touched check-meta.nix before, opening a separate PR targetting master for this discussion: #335110
I think this should be using ${arr[@]} notation for *Array variables.
Cannot think of a single case test case where this would produce a different result, but I recovered the original code (including the ${nameref+...} part) verbatim
There was a problem hiding this comment.
I think this should be using ${arr[@]} notation for *Array variables.
Cannot think of a single case test case where this would produce a different result, but I recovered the original code (including the
${nameref+...}part) verbatim
I don't think we should use [@] notation here - because we don't actually have an array here. That's the whole point.
There was a problem hiding this comment.
Sorry I didn't reply before the merge, I was away. I agree to some extend, and I did start with treating the input as just a string. But then I do hope to remove this branch maybe after a release, and the intent of reintroducing this branch was to mimic exactly what the old implementation was doing so... I guess it's OK we merged it the way it was
ofborg
bot
added
10.rebuild-darwin-stdenv
ofborg
bot
added
10.rebuild-darwin: 501+
rhendric
left a comment
rhendric
left a comment
There was a problem hiding this comment.
Just the sudo change has my 👍; I haven't been following what changes are happening with concatTo and have no opinion on that part of this.
Fixes e.g. the `sudo` build failure
SomeoneSerge
force-pushed
the
fix/structuredAttrs/xxxArray
branch
from
603d9f7 to
f974fa1
Compare
| # shellcheck disable=SC2206 | ||
| targetref+=( ${nameref-} ) ;; | ||
| if [[ "$name" = *"Array" ]]; then | ||
| nixErrorLog "concatTo(): $name is not declared as array, treating as a singleton. This will become an error in future" |
There was a problem hiding this comment.
| nixErrorLog "concatTo(): $name is not declared as array, treating as a singleton. This will become an error in future" | |
| nixErrorLog "concatTo(): $name is not declared as array, treating as a singleton. This will become an error in future." |
Also, should this use nixWarnLog, since it's not an error, yet?
There was a problem hiding this comment.
Ah, just read this:
making it output a warning instead of failing (using log level "error", otherwise I couldn't see the warning without extra flags...):
Hm.. nixErrorLog is not used anywhere in nixpkgs right now. Although.. none of the other log functions are used either, except for nixTalkativeLog and nixWarnLog directly inside generic/setup.sh.
This makes me wonder whether this is really the right "tool" to use?
With my arguments in #335110 (comment), maybe we should just silently apply the fallback here and do all the warnings / errors only on the nix-level?
There was a problem hiding this comment.
I think this is right, since nixErrorLog just spits something out on stderr. It might become structured logging in the future.
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
LGTM from the stdenv side.
| # shellcheck disable=SC2206 | ||
| targetref+=( ${nameref-} ) ;; | ||
| if [[ "$name" = *"Array" ]]; then | ||
| nixErrorLog "concatTo(): $name is not declared as array, treating as a singleton. This will become an error in future" |
There was a problem hiding this comment.
I think this is right, since nixErrorLog just spits something out on stderr. It might become structured logging in the future.
6 participants
Description of changes
The first commit fixes sudo #318614 (comment), making it output a warning instead of failing (using log level "error", otherwise I couldn't see the warning without extra flags...):
The second makes sudo a structuredAttrs derivation.
Not sure if I should target staging or staging-next
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.