structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE #9032

Ma27 · 2023-09-24T11:55:23Z

Motivation

Support for NIX_ATTRS_JSON_FILE/NIX_ATTRS_SH_FILE in nix develop
Document NIX_ATTRS_*_FILE vars, drop references to .attrs.{sh,json}.

Context

In #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] #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.

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.

Ma27 · 2023-09-24T13:00:30Z

Hmm OK while the error in CI looks related, I cannot reproduce it locally, neither with make nor with nix build/nix flake check. Assistance on fixing that would be greatly appreciated.

Was confused why `make html` didn't work while working on #9032, but then I realized that after this section was written, the target was renamed to `manual-html` in 6910f5d.

roberth

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.

roberth · 2023-09-25T16:44:44Z

src/nix/develop.cc

        auto json = res.dump();

        assert(BuildEnvironment::fromJSON(json) == *this);

        return json;
    }

+    bool supportsStructuredAttrs() const


Suggested change

bool supportsStructuredAttrs() const

bool wantsStructuredAttrs() const

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?

Switched to providesStructuredAttrs for now.

Ma27 · 2023-09-26T10:30:20Z

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.

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 nix-shell.

However, we would break nixpkgs: we have opt-in structured attrs support now and 43 occurrences of .attrs.sh (according to rg '\.attrs\.sh'|wc -l). I'd suggest to fix nixpkgs first (that's what I was up to anyways, just didn't get to it so far - can also request a review from you if that's OK) and then we can discuss how to do that in here.

The fallout outside of nixpkgs should be rather small considering that structured attrs are purely opt-in. But that's something we should discuss as a follow-up (if everyone agrees I'll open an issue in thsi repo).

src/nix/develop.cc

tests/structured-attrs.sh

src/nix/develop.cc

Ma27 · 2023-09-28T17:20:41Z

@roberth I'd still need some support on how to fix the failing test 😅
I even tried nix flake check locally and I didn't encounter that issue.

doc/manual/src/language/advanced-attributes.md

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)

Ma27 · 2023-10-02T12:11:22Z

@roberth the macos test fail seems unrelated, right? Is there anything else I'd need to fix to make this mergeable? :)

nixos-discourse · 2023-10-03T01:57:19Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-29-nix-team-meeting-minutes-90/33774/1

roberth · 2023-10-04T09:58:54Z

You're right. That failure was unrelated.

Thank you for fixing structuredAttrs support in nix develop!

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

Ma27 requested review from fricklerhandwerk and edolstra as code owners September 24, 2023 11:55

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Sep 24, 2023

Ma27 mentioned this pull request Sep 24, 2023

doc/hacking: fix make target to build the docs #9033

Merged

roberth added the nix-shell nix-shell, nix develop, nix print-dev-env, etc label Sep 25, 2023

roberth reviewed Sep 25, 2023

View reviewed changes

Ma27 force-pushed the structured-attrs-env-vars branch 2 times, most recently from ef4f78d to dbd7ba0 Compare September 28, 2023 16:16

roberth reviewed Sep 28, 2023

View reviewed changes

src/nix/develop.cc Outdated Show resolved Hide resolved

tests/structured-attrs.sh Outdated Show resolved Hide resolved

src/nix/develop.cc Show resolved Hide resolved

Ma27 force-pushed the structured-attrs-env-vars branch from dbd7ba0 to ed77786 Compare September 28, 2023 17:19

Ma27 requested a review from roberth September 28, 2023 17:20

Ma27 mentioned this pull request Sep 28, 2023

structured attrs: prefer NIX_ATTRS_*_FILE over .attrs.* NixOS/nixpkgs#257919

Merged

12 tasks

thufschmitt assigned roberth Sep 29, 2023

fricklerhandwerk reviewed Sep 29, 2023

View reviewed changes

doc/manual/src/language/advanced-attributes.md Show resolved Hide resolved

Ma27 requested a review from fricklerhandwerk September 29, 2023 15:15

Ma27 and others added 3 commits October 1, 2023 13:22

doc: reference NIX_ATTRS_*_FILE vars at the env var reference for drvs

42e3c6d

tests/structured-attrs.sh: grep -q -> grepQuiet

7a0886e

roberth force-pushed the structured-attrs-env-vars branch from 4d164a3 to 7a0886e Compare October 1, 2023 12:25

roberth approved these changes Oct 4, 2023

View reviewed changes

roberth merged commit 3c042f3 into NixOS:master Oct 4, 2023
8 checks passed

Ma27 deleted the structured-attrs-env-vars branch October 4, 2023 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE #9032

structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE #9032

Ma27 commented Sep 24, 2023 •

edited

Loading

Ma27 commented Sep 24, 2023

roberth left a comment

roberth Sep 25, 2023

Ma27 Sep 26, 2023

Ma27 Sep 28, 2023

Ma27 commented Sep 26, 2023

Ma27 commented Sep 28, 2023

Ma27 commented Oct 2, 2023

nixos-discourse commented Oct 3, 2023

roberth commented Oct 4, 2023

	bool supportsStructuredAttrs() const
	bool wantsStructuredAttrs() const

structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE #9032

structured attrs: improve support / usage of NIX_ATTRS_{SH,JSON}_FILE #9032

Conversation

Ma27 commented Sep 24, 2023 • edited Loading

Motivation

Context

Priorities

Ma27 commented Sep 24, 2023

roberth left a comment

Choose a reason for hiding this comment

roberth Sep 25, 2023

Choose a reason for hiding this comment

Ma27 Sep 26, 2023

Choose a reason for hiding this comment

Ma27 Sep 28, 2023

Choose a reason for hiding this comment

Ma27 commented Sep 26, 2023

Ma27 commented Sep 28, 2023

Ma27 commented Oct 2, 2023

nixos-discourse commented Oct 3, 2023

roberth commented Oct 4, 2023

Ma27 commented Sep 24, 2023 •

edited

Loading