lib.trivial: rename NIX_ABORT_ON_WARN to NIXPKGS_ABORT_ON_WARN #306026
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
There was a problem hiding this comment.
Just minor thoughts, but overall this is looking good to me! This is a really tricky change, but your explanation makes a lot of sense!
The multiple messages for Flakes can be fixed by putting the builtins.trace before even the { lib }: part of the file (builtins.trace "..." ({ lib }: ...)). But this does feel a bit too ugly..
So indeed I think it would be better to figure out why the Flake handling appears to import lib that many times. But this can be done in a separate PR :)
Ping @roberth as the one who introduced this environment variable
lib/trivial.nix
Outdated
| # uses of this variable would immediately result in an unrecoverable | ||
| # error where there should be a deprecation warning. | ||
| printDeprecationWarning = | ||
| if builtins.getEnv "NIX_ABORT_ON_WARN" != "" then |
There was a problem hiding this comment.
| if builtins.getEnv "NIX_ABORT_ON_WARN" != "" then | |
| if lib.isInOldestRelease 2405 && builtins.getEnv "NIX_ABORT_ON_WARN" != "" then |
This needs some code rearranging or inlining to get working without infinite recursion, but this makes sure that there's a transition period where stable third party projects can switch to NIXPKGS_ABORT_ON_WARN without warnings for end-users.
There was a problem hiding this comment.
I have done so, though aesthetically I don't particularly like the result. If you can think of any nicer way to do this, please let me know / feel free to edit this branch.
| if lib.elem (builtins.getEnv "NIX_ABORT_ON_WARN") ["1" "true" "yes"] | ||
| then msg: builtins.trace "[1;31mwarning: ${msg}[0m" (abort "NIX_ABORT_ON_WARN=true; warnings are treated as unrecoverable errors.") | ||
| if lib.elem (builtins.getEnv "NIXPKGS_ABORT_ON_WARN") [ "1" "true" "yes" ] | ||
| || lib.elem (builtins.getEnv "NIX_ABORT_ON_WARN") [ "1" "true" "yes" ] |
There was a problem hiding this comment.
Let's have a comment here to mention that this one is deprecated, referring to printDeprecationWarning
|
While your reasoning makes sense, my original goal was to build this functionality into Nix.
This PR would not prevent that from happening, but it would be messier, as users first have to learn to use Also there's an argument to be made that |
this makes it consistent with other environment variables read by nixpkgs to configure its behaviour. The previous naming appeared to suggest that the variable is read by Nix itself, which it is not. This is especially a source of confusion when used together with Nix's pure evaluation mode, in which case setting the variable has no effect. The old NIX_ABORT_ON_WARN is still honoured, but produces a deprecation warning. This warning's check is added as a wrapper to the entire lib.trivial attrset, as it should be checked (and, if necessary, printed) regardless of whether or not a use of lib.warn is encountered, but whenever the lib.warn function is available (i.e. whenever lib itself is evaluated), so that users of NIX_ABORT_ON_WARN will see it even if their code does not currently produce any warnings. Likewise, this means that the warning does not actually abort evaluation, as otherwise downstream users of NIX_ABORT_ON_WARN would be immediately presented with an unrecoverable error.
as NIX_ABORT_ON_WARN has been deprecated.
As by NixOS#306026 (comment) This has to temporarily move oldestSupportedRelease up outside the main attribute set so that it can be used before lib.trivial itself is defined.
This would, of course, be the cleaner way to do it, and I'd much prefer it over the current situation. However, the current state has now existed for ~two and a half years, and unless I've overlooked something (admittedly possible, github search being what it is) there have been no plans to actually change it on the nix side after it was not done during or after adding
yes there is: the need to learn that this variable behaves differently then expected, and that it does not work in pure evaluation mode (which recently cost me some time during and after #303841 while looking at how the module options documentation is built, and how to check that it does, indeed, still work — and I expect others will have run into similarly confusing corners when dealing with it). So while I can see the motivation for waiting further, to reduce possible future confusion later on (in case this gets merged & then nix does add a similar feature), I'm a little skeptical of doing so if this means keeping a current source of confusion around for (potentially) an undefined future period of time. |
stuebinm
force-pushed
the
rename-nix-abort-on-warn
branch
from
b8882a8 to
7edb2fc
Compare
|
When I found out about this environment variable recently (ngi-nix/ngipkgs#155 and consequently https://discourse.nixos.org/t/disallow-warnings-on-nix-flake-check/38964) I had this change in mind, too, but did not follow through. I support @stuebinm's reasoning. No matter what happens upstream in Nix, this environment variable should have the prefix |
|
I've just opened NixOS/nix#10592. Sorry for dropping the ball earlier. |
wegank
added
the
2.status: merge conflict
Description of changes
Renames the
NIX_ABORT_ON_WARNenvironment variable toNIXPKGS_ABORT_ON_WARNto make it consistent with other environment variables read by nixpkgs to configure its behaviour (e.g.NIXPKGS_ALLOW_INSECUREetc.). The previous naming appeared to suggest that the variable is read by Nix itself, which it is not. This is especially a source of confusion when used together with Nix's pure evaluation mode, in which case setting the variable has no effect.The old
NIX_ABORT_ON_WARNis still honoured, but produces a deprecation warning. This warning's check is added as a wrapper to the entire lib.trivial attrset, as it should be checked (and, if necessary, printed) regardless of whether or not a use oflib.warnis encountered, but whenever thelib.warnfunction is available (i.e. wheneverlibitself is evaluated), so that users ofNIX_ABORT_ON_WARNwill see it even if their code does not currently produce any warnings. Likewise, this means that the warning does not actually abort evaluation, as otherwise downstream users ofNIX_ABORT_ON_WARNwould be immediately presented with an unrecoverable error.This deprecation warning and honouring of the older behaviour can then be removed in a future release.
Downsides
For reasons which remain mysterious to me, the deprecation warning is printed eight times if
nixpkgsis imported as a flake:but only once if imported in the traditional way:
This is only a minor annoyance, but does perhaps (?) point to an unrelated issue with how the nixpkgs flake handles importing
lib.Alternatives
There are a few possible placements for the deprecation warning & its check in the code:
lib.warnitself: seems the most logical place to put it, but it would then only be shown if a warning is actually produced. As (presumably) most users ofNIX_ABORT_ON_WARNwill strive to keep their code free of warnings, this could lead to them never seeing the deprecation message.lib/, e.g. inpkgs/top-level/impure.nix: i am very hesitant to touch that file, since even slight mistakes would possibly have large consequences. However, it is the only place i could find where, if placed there, the warning is only ever printed once even when using nixpkgs as a flake.Question
Should this change be mentioned in the release notes? I was unable to find any clear guidelines on what (if any) changes to
libshould get an entry there.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.