Source complete env in nix-shell with __structuredAttrs = true;
#4770
Conversation
|
Also, anybody with sufficient Darwin & C++ knowledge to tell me what to use instead of |
I think it’s an old-llvm issue rather than a darwin one: https://releases.llvm.org/7.0.0/projects/libcxx/docs/UsingLibcxx.html#using-filesystem-and-libc-fs . Apparently you need to add a |
There was a problem hiding this comment.
Did a first review.
- A big 👍 on the end-goal
- Implementation-wise, I’m not really fond of inlining the
DerivationGoalinnix-build.cc. That’s quite a big abstraction breach (and time has already brought enough of these…) - I’m also not thrilled by the
.attrs.jsonin PWD. It’d be nice if there were an alternative way of doing that − if only because I’d like to start twonix-shells from the same directory without them meddling with each other.
(this also made me realise that nix-shell doesn’t really support CA derivations right now… but that’s another topic)
| worker.store.pathInfoToJSON(jsonRoot, | ||
| exportReferences(storePaths), false, true); | ||
| } | ||
| json[i.key()] = nlohmann::json::parse(str.str()); // urgh |
There was a problem hiding this comment.
I guess you can rewrite pathInfoToJSON to use nlohmann::json to avoid that (letting @edolstra judge on that because that would probably mean making store-api.cc depend on <nlohmann/json.hpp>)
There was a problem hiding this comment.
Hmm, should this also happen in this PR?
There was a problem hiding this comment.
Nah, I just didn’t notice that this was only appearing in the diff because it was code that moved around rather than new code. So can be left as a follow-up :)
|
@regnat wrote
Yes i would strongly prefer that too. Let's always take things apart as much as possible. |
|
@regnat @Ericson2314 I'm not sure about it and this is in fact my primary pain point as well. The problem is that from my understanding Other than that, it's totally possible to move this to ParsedDerivation (in fact I even have something for it in my git stash lying around, but I didn't use it in the end because of my problem with Regarding |
There are quite a number of uses of .attrs.sh with structured-attrs, e.g. Maybe calling nix-shell could create a subdir in the cwd to mitigate the cleanup problem? Although that creates other problems with nix-shell, when wanting to run only selected phases and not the whole |
Ahah, I missed that ;)
You’d need to be a bit more careful − somehow keep a reference count of all the I wonder whether we could strengthen the semantics of |
Ok, that's good to know. I'll check if that's doable without having to fix too much stuff after that. @regnat do you think it makes sense to add it to the
Ohhhh, you're absolutely right, I didn't think of that, sorry!! I actually think that |
Ma27
force-pushed
the
structured-attrs-shell
branch
from
976844e
to
8dded0d
Compare
|
@regnat changed your primary two issues (moved code to generate (would also appreciate a review from @Ericson2314 & @edolstra btw) |
| @@ -695,6 +695,8 @@ public: | |||
|
|
|||
| const Stats & getStats(); | |||
|
|
|||
| StorePathSet exportReferences(const StorePathSet & storePaths, const StorePathSet & inputPaths); | |||
There was a problem hiding this comment.
To be honest, I'm a bit way of putting more odd and ends (i.e. non-virtual methods) in the Store class (I prefer to keep "interfaces" slim like type classes.
But then again, this method doesn't have any other types it might be associated with (Store paths are "upstream" of stores), so maybe it's fine.
Yes, I think that's a good idea. Writing to BTW given the legacy status of |
There was a problem hiding this comment.
@regnat changed your primary two issues (moved code to generate .attrs.{sh,json} into ParsedDerivation, use env vars to reference these files in both the nix-shell & the build sandbox). Does the approach look okay to you now?
Yes, looks great. And I have to say that I’m utterly pleased by
😄
After realising that implementing this isn't as hard as I thought, I added it in 2a247b8
I'm currently on
I'm happy to take a look at this as well at some point :) Btw given that 2.4 doesn't seem to have a roadmap and @globin told me that he'd like to try getting this into one of the upcoming NixOS releases (not 21.05 though), I'd like to raise the question whether it'd be okay for you to backport the |
|
Since I'm not sure if we should add even more changes in this already non-trivial (but kinda ready) PR and AFAIU the other suggestion isn't applicable. I split the JSON and bash generation into two methods of |
|
@regnat Added a testcase and a few comments :) |
|
@edolstra anything missing here to get this merged? :) |
| } | ||
| json[i.key()] = nlohmann::json::parse(str.str()); // urgh | ||
| } | ||
| if (auto structAttrsJson = parsedDrv->prepareStructuredAttrs(inputRewrites, worker.store, inputPaths)) { |
There was a problem hiding this comment.
Does prepareStructuredAttrs need inputRewrites? It should be possible to apply the rewrites to the structAttrsJson.value() right?
There was a problem hiding this comment.
@edolstra Yeah I kept it in this codepath as it used to be there previously and seemed kinda correct to do the rewrite when building the datastructure. But if you'd prefer to move it to LocalDerivationGoal, that'd work for me as well :)
There was a problem hiding this comment.
This is needed to push the adoption of structured attrs[1] forward. It's now checked if a `__json` exists in the environment-map of the derivation to be openend in a `nix-shell`. Derivations with structured attributes enabled also make use of a file named `.attrs.json` containing every environment variable represented as JSON which is useful for e.g. `exportReferencesGraph`[2]. To provide an environment similar to the build sandbox, `nix-shell` now adds a `.attrs.json` to `cwd` (which is mostly equal to the one in the build sandbox) and removes it using an exit hook when closing the shell. To avoid leaking internals of the build-process to the `nix-shell`, the entire logic to generate JSON and shell code for structured attrs was moved into the `ParsedDerivation` class. [1] https://nixos.mayflower.consulting/blog/2020/01/20/structured-attrs/ [2] https://nixos.org/manual/nix/unstable/expressions/advanced-attributes.html#advanced-attributes
This way no derivation has to expect that these files are in the `cwd` during the build. This is problematic for `nix-shell` where these files would have to be inserted into the nix-shell's `cwd` which can become problematic with e.g. recursive `nix-shell`. To remain backwards-compatible, the location inside the build sandbox will be kept, however using these files directly should be deprecated from now on.
Ma27
force-pushed
the
structured-attrs-shell
branch
from
bfe2a63
to
6f20654
Compare
There was a problem hiding this comment.
Just had a look at the last commit (9952e4e), but it looks OK to me.
The thing with the rewrites of the json is a bit weird, but it was already weird before the PR, so it’s not really a problem for now
Ma27
force-pushed
the
structured-attrs-shell
branch
from
9952e4e
to
644415d
Compare
|
cc @edolstra anything else tbd to get this merged? :) |
Conflicts:
src/nix/develop.cc
src/nix/get-env.sh
tests/shell.nix
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 I realized that the shell experience isn't as nice as I remembered and I identified the following problems: * A lot of builders in `nixpkgs` don't care about the env vars and assume that `.attrs.sh` is in `$NIX_BUILD_TOP`. I considered changing Nix that way, 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. I think 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)
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 current state and 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 works. However, the same assumption cannot be made for `.attrs.json` which can only be used in a `nix-shell(1)` session when using `$NIX_ATTRS_JSON_FILE`. 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. In other words, this is pretty inconsistent with how the buildscripts behave in an actual build vs in a debug shell and I think that relying solely on the environment variabels is the best way to solve that. Also, I think 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)
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)
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)
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
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)
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
Notes
ParsedDerivation.attrs.jsonin PWDCCing @regnat (because I had to touch input rewrites inside
{Local,}DerivationGoala bit), @Ericson2314 (because he touched a lot of code down there), @globin (because of the nixpkgs PR), @edolstraCommit message
This is needed to push the adoption of structured attrs[1] forward. It's
now checked if a
__jsonexists in the environment-map of the derivationto be openend in a
nix-shell.Derivations with structured attributes enabled also make use of a file
named
.attrs.jsoncontaining every environment variable represented asJSON which is useful for e.g.
exportReferencesGraph[2]. Toprovide an environment similar to the build sandbox,
nix-shellnowadds a
.attrs.jsontocwd(which is mostly equal to the one in thebuild sandbox) and removes it using an exit hook when closing the shell.
To avoid leaking internals of the build-process to the
nix-shell, theentire logic to generate JSON and shell code for structured attrs was
moved into the
ParsedDerivationclass.[1] https://nixos.mayflower.consulting/blog/2020/01/20/structured-attrs/
[2] https://nixos.org/manual/nix/unstable/expressions/advanced-attributes.html#advanced-attributes