check-meta: warn about misinterpreted *Array variables #335110
Conversation
pkgs/stdenv/generic/check-meta.nix
Outdated
There was a problem hiding this comment.
Outputs look like this:
❯ nix-build -A sudo
trace: Package sudo-1.9.15p5 in /home/ss/Sources/nixpkgs/.worktree/structuredAttrs/pkgs/tools/security/sudo/default.nix:77 The following variables will be misinterpreted as single element bash arrays because __structuredAttrs is not set: configureFlagsArray, continuing anyway.
/nix/store/5y6c2b30ywcliyazpfqxl9xcr3wri46g-sudo-1.9.15p5
There was a problem hiding this comment.
The following variables ...: configureFlagsArray, continuing anyway.
This run-on sentence is going to look really odd when there's more than 1 variable
There was a problem hiding this comment.
Perhaps just “The … variables” (there are unlikely to be many), or “Some variables will be …. The affected variables are: …”.
There was a problem hiding this comment.
trace: Package sudo-1.9.15p5 in /home/ss/Sources/nixpkgs/.worktree/structuredAttrs/pkgs/tools/security/sudo/default.nix:77 The variable "configureFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set, continuing anyway.
ofborg
bot
added
10.rebuild-darwin: 0
|
Can we make this an error instead of a warning? Imho, using
All of this is is pointless, if we can support structuredAttrs properly. Let's do this:
If you want to go ahead with the warning instead, let's make it have the "this will be an error in the future" bit from #334973, too:
|
There was a problem hiding this comment.
I used nix-instantiate --strict --eval --json --system x86_64-linux pkgs/top-level/release-attrpaths-superset.nix -A names | jq -r '.[]' > /tmp/packages to see this error show up.
Blast radius:
trace: Warning while evaluating python3.12-six-1.16.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytest-xdist-3.6.1: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-execnet-2.1.1: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pycparser-2.22: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-greenlet-3.0.3: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytz-2024.1: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-six-1.16.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytz-2024.1: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytest-timeout-2.3.1: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-numpy-1.26.4: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-fs-2.4.16: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pandas-2.2.2: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-six-1.16.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytest-xdist-3.6.1: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-execnet-2.1.1: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pycparser-2.22: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-greenlet-3.0.3: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytz-2024.1: «unstructured-arrays»: The variable "unittestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-mypy-extensions-1.0.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-jupyter-core-5.7.2: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-cryptography-43.0.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pytest-subprocess-1.5.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-anyio-4.4.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-debugpy-1.8.2: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-fastapi-0.112.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-sh-2.0.6: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-httpx-0.27.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-pydantic-2.8.2: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-phonenumbers-8.13.39: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-starlette-0.37.2: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-tqdm-4.66.4: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating python3.12-faker-25.8.0: «unstructured-arrays»: The variable "pytestFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
trace: Warning while evaluating sudo-1.9.15p5: «unstructured-arrays»: The variable "configureFlagsArray" will be misinterpreted as single element bash array because __structuredAttrs is not set
So it's high, but it's not super high.
| usePlural = variables != [(builtins.head variables)]; | ||
| plural = optionalString usePlural "s"; | ||
| in | ||
| { valid = "warn"; |
There was a problem hiding this comment.
I agree with @wolfgangwalther that this ought to be an error, but the set of packages it touches is pretty high. We'll patch it all up in the next staging-next.
There was a problem hiding this comment.
We need to consider out‐of‐tree users too. I agree with the suggestion to add the “this will break” warning and then make it a hard error on the next release.
| quote = s: ''"${s}"''; | ||
| variables = unstructuredArrays attrs; | ||
| variablesStr = builtins.concatStringsSep ", " (map quote variables); | ||
| usePlural = variables != [(builtins.head variables)]; | ||
| plural = optionalString usePlural "s"; |
There was a problem hiding this comment.
Is there a reason we’re spelling lib.length variables != 1 like that…?
|
You can also see that it breaks ofBorg eval. That might mean that effectively this is an error, at least within nixpkgs. |
|
Yes, ofborg doesn’t like warnings. We have to fix in‐tree users first. Re: Python packages, |
This comment was marked as outdated.
This comment was marked as outdated.
|
This is actually quite a lot 😳 |
|
Now imagine all the derivations that aren’t in Nixpkgs :) For |
args+=" ${pytestFlagsArray[@]}"That’s not quite… Eh, whatever. |
I'll start working on all the non-python ones first. |
|
This is a false-positive. |
Those would not trigger the warning in bash, because we are not using |
|
Given #335110 (review), it seems like we can special‐case |
I'm not sure about that, yet. There are more cases where Edit: Ah, of course, we have the fallback to the old behavior already. So yes, of course, all should be good right now. |
|
Right, I just mean in terms of what @philiptaron’s eval showed up on the Nix warning side :) |
|
Here's the actual warnings, after my evidently error-prone searches to find their root causes. https://gist.github.com/philiptaron/c2335eab92e5f0b2b14438aa95edb6d9 |
|
Here’s the non‐Python ones: |
|
|
I'm not sure I fully understand the sentiment: with
The hook is funny indeed (let's get rid of this non-sense too), but yeah we literally have the Nixpkgs manual saying "use pytestFlagsArray, here's a code example", and I'm pretty sure I can find examples in Nixpkgs where I haven't touched anything over the weekend, but I agree with the plan formed above: we fix in-tree users by upgrading them to __structuredAttrs or by exception, introduce a warning with the "will become an error" phrasing, and schedule a hard error. |
|
Also a sentiment: I think we have only very limited tools for overriding the behaviour of |
|
If we default to |
Except the stuff that manually |
|
Yeah, I don’t know… out‐of‐tree derivations always felt like a bit of an awkward case to me and it’s hard to keep stuff working when we don’t have any visibility into it. Keeping old Nixpkgs trees themselves running is one thing but there are lots of things that can break an existing derivation if you try to mix it into a newer Nixpkgs than it was made for. Like, every backwards‐incompatible library update we ever do. We should warn about this stuff but I don’t think we can maintain perfect backwards compatibility with every out‐of‐tree derivation forever. Every treewide change we have to make is something that probably breaks somebody’s package. |
Yeah about that specifically, I think we're under no obligation wrt out-of-tree, and it's just the good will if we're averse to surprising and disruptive changes. There's currently just one "channel" for out-of-tree projects to cast light on themselves and prevent nixpkgs changes that negatively affect them: direct participation, investing human hours in nixpkgs discussions and maintenance. Idk if this can be improved, but it's not unreasonable |
|
Right. I think warning for 24.11 and removing the hack in 25.05 is more generous to out‐of‐tree users than a lot of breaking changes we make are. |
My point about the "with structuredAttrs" case is: When you have structuredAttrs, you can just use the non-Array variants with the same result. They are treated as arrays, too. So you don't need Take the example where we fixed sudo to use structuredAttrs. The fix still left the option with the trailing whitespace in So with structuredAttrs, |
|
Then perhaps we should warn about using these variables regardless of |
All of them removed in #335925. |
Yes, possibly with some hint about "you probably wanted to do X instead". |
|
while reading this I thought maybe we should update the |
|
@ofborg eval |
wegank
added
the
12.approvals: 1
Description of changes
As per #334973 (comment). The issue originally noted in #318614 (comment).
I never worked withcheck-metabefore, I observe that the warning isn't actually displayed by default, but changing"warn"tovalidity "no"does produce the expected effectI also realize that Nix is expected to be painfully slow and that we may not want to extend
check-metawithout a good reason lest we inflate the eval timesThings 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.