structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE#9032
structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE#9032roberth merged 3 commits intoNixOS:masterfrom
Conversation
Was confused why `make html` didn't work while working on NixOS#9032, but then I realized that after this section was written, the target was renamed to `manual-html` in 6910f5d.
|
Hmm OK while the error in CI looks related, I cannot reproduce it locally, neither with |
roberth
left a comment
There was a problem hiding this comment.
Cursory review. Concept mostly lgtm.
Should we consider moving the .attrs.* files into a subdir for a release as a "brown out"? It would find erroneous assumptions in expressions, but it might cause disproportionate harm.
src/nix/develop.cc
Outdated
| return json; | ||
| } | ||
|
|
||
| bool supportsStructuredAttrs() const |
There was a problem hiding this comment.
| bool supportsStructuredAttrs() const | |
| bool wantsStructuredAttrs() const |
There was a problem hiding this comment.
Hmm, I think I understand why supports is not a fitting verb here. But IMHO wants is also misleading: this returns true only if structured attrs actually exist (i.e. get-env.sh picked them up and wrote them into the JSON).
Hence, what about providesStructuredAttrs?
There was a problem hiding this comment.
Switched to providesStructuredAttrs for now.
I think that might be reasonable to do in the future: pushing people to do the right thing has a benefit, namely better support for However, we would break The fallout outside of |
ef4f78d to
dbd7ba0
Compare
dbd7ba0 to
ed77786
Compare
|
@roberth I'd still need some support on how to fix the failing test 😅 |
In NixOS#4770 I implemented proper `nix-shell(1)` support for derivations using `__structuredAttrs = true;`. Back then we decided to introduce two new environment variables, `NIX_ATTRS_SH_FILE` for `.attrs.sh` and `NIX_ATTRS_JSON_FILE` for `.attrs.json`. This was to avoid having to copy these files to `$NIX_BUILD_TOP` in a `nix-shell(1)` session which effectively meant copying these files to the project dir without cleaning up afterwords[1]. On last NixCon I resumed hacking on `__structuredAttrs = true;` by default for `nixpkgs` with a few other folks and getting back to it, I identified a few problems with the how it's used in `nixpkgs`: * A lot of builders in `nixpkgs` don't care about the env vars and assume that `.attrs.sh` and `.attrs.json` are in `$NIX_BUILD_TOP`. The sole reason why this works is that `nix-shell(1)` sources the contents of `.attrs.sh` and then sources `$stdenv/setup` if it exists. This may not be pretty, but it mostly works. One notable difference when using nixpkgs' stdenv as of now is however that `$__structuredAttrs` is set to `1` on regular builds, but set to an empty string in a shell session. Also, `.attrs.json` cannot be used in shell sessions because it can only be accessed by `$NIX_ATTRS_JSON_FILE` and not by `$NIX_BUILD_TOP/.attrs.json`. I considered changing Nix to be compatible with what nixpkgs effectively does, but then we'd have to either move $NIX_BUILD_TOP for shell sessions to a temporary location (and thus breaking a lot of assumptions) or we'd reintroduce all the problems we solved back then by using these two env vars. This is partly because I didn't document these variables back then (mea culpa), so I decided to drop all mentions of `.attrs.{json,sh}` in the manual and only refer to `$NIX_ATTRS_SH_FILE` and `$NIX_ATTRS_JSON_FILE`. The same applies to all our integration tests. Theoretically we could deprecated using `"$NIX_BUILD_TOP"/.attrs.sh` in the future now. * `nix develop` and `nix print-dev-env` don't support this environment variable at all even though they're supposed to be part of the replacement for `nix-shell` - for the drv debugging part to be precise. This isn't a big deal for the vast majority of derivations, i.e. derivations relying on nixpkgs' `stdenv` wiring things together properly. This is because `nix develop` effectively "clones" the derivation and replaces the builder with a script that dumps all of the environment, shell variables, functions etc, so the state of structured attrs being "sourced" is transmitted into the dev shell and most of the time you don't need to worry about `.attrs.sh` not existing because the shell is correctly configured and the if [ -e .attrs.sh ]; then source .attrs.sh; fi is simply omitted. However, this will break when having a derivation that reads e.g. from `.attrs.json` like with import <nixpkgs> {}; runCommand "foo" { __structuredAttrs = true; foo.bar = 23; } '' cat $NIX_ATTRS_JSON_FILE # doesn't work because it points to /build/.attrs.json '' To work around this I employed a similar approach as it exists for `nix-shell`: the `NIX_ATTRS_{JSON,SH}_FILE` vars are replaced with temporary locations. The contents of `.attrs.sh` and `.attrs.json` are now written into the JSON by `get-env.sh`, the builder that `nix develop` injects into the derivation it's debugging. So finally the exact file contents are present and exported by `nix develop`. I also made `.attrs.json` a JSON string in the JSON printed by `get-env.sh` on purpose because then it's not necessary to serialize the object structure again. `nix develop` only needs the JSON as string because it's only written into the temporary file. I'm not entirely sure if it makes sense to also use a temporary location for `nix print-dev-env` (rather than just skipping the rewrite in there), but this would probably break certain cases where it's relied upon `$NIX_ATTRS_SH_FILE` to exist (prime example are the `nix print-dev-env` test-cases I wrote in this patch using `tests/shell.nix`, these would fail because the env var exists, but it cannot read from it). [1] NixOS#4770 (comment)
4d164a3 to
7a0886e
Compare
|
@roberth the macos test fail seems unrelated, right? Is there anything else I'd need to fix to make this mergeable? :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
You're right. That failure was unrelated. Thank you for fixing structuredAttrs support in |
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
structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE
(cherry picked from commit 3c042f3)
Change-Id: I7e41838338ee1edf31fff6f9e354c3db2bba6c0e
Motivation
NIX_ATTRS_JSON_FILE/NIX_ATTRS_SH_FILEinnix develop.attrs.{sh,json}.Context
In #4770 I implemented proper
nix-shell(1)support for derivationsusing
__structuredAttrs = true;. Back then we decided to introduce twonew environment variables,
NIX_ATTRS_SH_FILEfor.attrs.shandNIX_ATTRS_JSON_FILEfor.attrs.json. This was to avoid having tocopy these files to
$NIX_BUILD_TOPin anix-shell(1)session whicheffectively meant copying these files to the project dir without
cleaning up afterwords[1].
On last NixCon I resumed hacking on
__structuredAttrs = true;bydefault for
nixpkgswith a few other folks and getting back to it,I identified a few problems with the how it's used in
nixpkgs:A lot of builders in
nixpkgsdon't care about the env vars andassume that
.attrs.shand.attrs.jsonare in$NIX_BUILD_TOP.The sole reason why this works is that
nix-shell(1)sourcesthe contents of
.attrs.shand then sources$stdenv/setupif itexists. This may not be pretty, but it mostly works. One notable
difference when using nixpkgs' stdenv as of now is however that
$__structuredAttrsis set to1on regular builds, but set toan empty string in a shell session.
Also,
.attrs.jsoncannot be used in shell sessions becauseit can only be accessed by
$NIX_ATTRS_JSON_FILEand not by$NIX_BUILD_TOP/.attrs.json.I considered changing Nix to be compatible with what nixpkgs
effectively does, but then we'd have to either move $NIX_BUILD_TOP for
shell sessions to a temporary location (and thus breaking a lot of
assumptions) or we'd reintroduce all the problems we solved back then
by using these two env vars.
This is partly because I didn't document these variables back
then (mea culpa), so I decided to drop all mentions of
.attrs.{json,sh}in the manual and only refer to$NIX_ATTRS_SH_FILEand
$NIX_ATTRS_JSON_FILE. The same applies to all our integration tests.Theoretically we could deprecated using
"$NIX_BUILD_TOP"/.attrs.shinthe future now.
nix developandnix print-dev-envdon't support this environmentvariable at all even though they're supposed to be part of the replacement
for
nix-shell- for the drv debugging part to be precise.This isn't a big deal for the vast majority of derivations, i.e.
derivations relying on nixpkgs'
stdenvwiring things togetherproperly. This is because
nix developeffectively "clones" thederivation and replaces the builder with a script that dumps all of
the environment, shell variables, functions etc, so the state of
structured attrs being "sourced" is transmitted into the dev shell and
most of the time you don't need to worry about
.attrs.shnotexisting because the shell is correctly configured and the
is simply omitted.
However, this will break when having a derivation that reads e.g. from
.attrs.jsonlikeTo work around this I employed a similar approach as it exists for
nix-shell: theNIX_ATTRS_{JSON,SH}_FILEvars are replaced withtemporary locations.
The contents of
.attrs.shand.attrs.jsonare now written into theJSON by
get-env.sh, the builder thatnix developinjects into thederivation it's debugging. So finally the exact file contents are
present and exported by
nix develop.I also made
.attrs.jsona JSON string in the JSON printed byget-env.shon purpose because then it's not necessary to serializethe object structure again.
nix developonly needs the JSONas string because it's only written into the temporary file.
I'm not entirely sure if it makes sense to also use a temporary
location for
nix print-dev-env(rather than just skipping therewrite in there), but this would probably break certain cases where
it's relied upon
$NIX_ATTRS_SH_FILEto exist (prime example are thenix print-dev-envtest-cases I wrote in this patch usingtests/shell.nix, these would fail because the env var exists, but itcannot read from it).
[1] #4770 (comment)
cc @globin @lheckemann @WilliButz - because we worked on structured attrs again recently
cc @edolstra @Ericson2314 @thufschmitt @roberth
Priorities
Add 👍 to pull requests you find important.