structured attrs: prefer NIX_ATTRS_*_FILE over .attrs.*
#257919
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ma27
requested review from
grahamc,
lheckemann,
roberth,
globin and
Artturin
Ma27
requested review from
ulrikstrid,
stigtsp,
zakame,
dasJ and
Ericson2314
as code owners
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin-stdenv
Artturin
reviewed
Oct 1, 2023
lheckemann
reviewed
Oct 2, 2023
lheckemann
approved these changes
Oct 2, 2023
delroth
added
the
12.approvals: 1
Relying on `.attrs.sh` to exist in `$NIX_BUILD_TOP` is problematic
because that's not compatible with how `nix-shell(1)` behaves. It places
`.attrs.{json,sh}` into a temporary directory and makes them accessible via
`$NIX_ATTRS_{SH,JSON}_FILE` in the environment[1]. The sole reason that
`nix-shell(1)` still works with structured-attrs enabled derivations
is that the contents of `.attrs.sh` are sourced into the
shell before sourcing `$stdenv/setup` (if `$stdenv` exists) by `nix-shell`.
However, the assumption that two files called `.attrs.sh` and
`.attrs.json` exist in `$NIX_BUILD_TOP` is wrong in an interactive shell
session and thus an inconsistency between shell debug session and actual
builds which can lead to unexpected problems.
To be precise, we currently have the following problem: an expression
like
with import ./. {};
runCommand "foo" { __structuredAttrs = true; foo.bar = [ 1 2 3 ]; }
''
echo "''${__structuredAttrs@Q}"
touch $out
''
prints `1` in its build-log. However when building interactively in a
`nix-shell`, it doesn't.
Because of that, I'm considering to propose a full deprecation of
`$NIX_BUILD_TOP/.attrs.{json,sh}`. A first step is to only mention the
environment variables, but not the actual paths anymore in Nix's
manual[2]. The second step - this patch - is to fix nixpkgs' stdenv
accordingly.
Please note that we cannot check for `-e "$NIX_ATTRS_JSON_FILE"` because
certain outdated Nix minors (that are still in the range of supported
Nix versions in `nixpkgs`) have a bug where `NIX_ATTRS_JSON_FILE` points
to the wrong file while building[3].
Also, for compatibility with Nix 2.3 which doesn't provide these
environment variables at all we still need to check for the existence of
.attrs.json/.attrs.sh here. As soon as we bump nixpkgs' minver to 2.4,
this can be dropped.
Finally, dropped the check for ATTRS_SH_FILE because that was never
relevant. In nix#4770 the ATTRS_SH_FILE variable was introduced[4] and
in a review iteration prefixed with NIX_[5]. In other words, these
variables were never part of a release and you'd only have this problem
if you'd use a Nix from a git revision of my branch from back then. In
other words, that's dead code.
[1] NixOS/nix#4770 (comment)
[2] NixOS/nix#9032
[3] NixOS/nix#6736
[4] NixOS/nix@3944a12
[5] NixOS/nix@27ce722
When specifying the `builder` attribute in `stdenv.mkDerivation`, this
will be effectively transformed into
builtins.derivation {
builder = stdenv.shell;
args = [ "-e" builder ];
}
This also means that `default-builder.sh` is never sourced and as a
result it's not guaranteed that `$NIX_ATTRS_SH_FILE` is set to a correct
location[1].
Also, we need to source `.attrs.sh` to source `$stdenv`. So, the
following is done now:
* If `$NIX_ATTRS_SH_FILE` points to a correct location, then use it.
Directly using `.attrs.sh` is problematic for `nix-shell(1)` usage
(see previous commit for more context), so prefer the environment
variable if possible.
* Otherwise, if `.attrs.sh` exists, then use it. See [1] for when this
can happen.
* If neither applies, it can be assumed that `__structuredAttrs` is
turned off and thus nothing needs to be done.
[1] It's possible that it doesn't exist at all - in case of Nix 2.3 or
it can point to a wrong location on older Nix versions with a bug in
`__structuredAttrs`.
Derivations affected by this patch set `__structuredAttrs = true;` and provide their own `builder`, i.e. it's necessary to `source .attrs.sh`. Rather than adding even more `if`-`source` monstrums, I decided to modify all of those derivations to use `buildCommand` or `runCommand`, without `builder` being set. Then, `$stdenv/setup` is sourced already and as a result it's safe to assume that `NIX_ATTRS_JSON_FILE`/`NIX_ATTRS_SH_FILE` point to a usable location both in a build and a shell session.
Ma27
force-pushed
the
structured-attrs-env-vars
branch
from
db0afdc to
c8f5c30
Compare
|
@ofborg build tests.stdenv.structuredAttrsByDefault tests.stdenv.structuredAttrsByDefault.hooks |
2 tasks
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
tl;dr relying on
$NIX_BUILD_TOP/.attrs.*is problematic because it doesn't exist innix-shell/nix develop. While most ofnix-shellworks "by accident" already, this change ensures consistent environments in regular builds and interactive debugging sessions usingnix-shell/nix develop.For that, references of
.attrs.sh/.attrs.jsonare replaced (where possible) in a backwards-compatible manner (Nix 2.3 is the minimum Nix supported and doesn't provide these vars) instdenvand a custombuilder(second commit)__structuredAttrsalready and use a custombuilder(third commit)See also NixOS/nix#9032.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)