nix-shell: Support --out-link

[jsoo1]

Closes #2208.

nix-shell previously had no way of creating gcroots for shell derivation. This can cause problems if a garbage collection happens concurrently with a nix-shell incantation that runs long enough. nix develop solves this, as far as I understand it, but --out-link functionality would still be useful in some circumstances.

[jsoo1]

My only concern with this change is the following scenario:

1. default.nix builds both the shell and the primary artifact of some project
2. nix-shell --out-link <some-path> fails to build because the project is in a broken state

The user could omit --out-link in this case. But there is currently no indication why the shell can't be accessed to work on the project in a failiing state.

Edit: I guess this is already the way things work. So no change introduced by this PR.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/1379

[roberth]

Wouldn't a temporary root solve the problem of retaining a long running shell's dependencies? This would be a cleaner solution where you don't need to put in the file system what seem to be useless links.

[roberth]

It seems that execvp might stop us from holding on to temporary roots. We might want to run a child process instead.

[jsoo1]

Wouldn't a temporary root solve the problem of retaining a long running shell's dependencies? This would be a cleaner solution where you don't need to put in the file system what seem to be useless links.

I think probably nix-shell would ideally always create a temp root, what do you think?

--out-link mirrors nix-build, though, and was definitely easier to implement 😅

It seems that execvp might stop us from holding on to temporary roots. We might want to run a child process instead.

Would that be a breaking change to nix-shell?

[roberth]

I think probably nix-shell would ideally always create a temp root, what do you think?

👍

--out-link mirrors nix-build, though, and was definitely easier to implement sweat_smile

nix-shell is quite different from nix-build though. A shell doesn't really produce outputs, so it shouldn't mirror nix-build like that.

It seems that execvp might stop us from holding on to temporary roots. We might want to run a child process instead.

Would that be a breaking change to nix-shell?

We're definitely past the point where any detail could become a breaking change, but I'd expect even a naive implementation to fix more problems than it creates. One potential problem we can solve is passing posix signals along to the child process. The siginfo_t sending pid will still be different, but now I'm talking about a niche in a niche. We could add an --exec flag as a contingency.

[jsoo1]

nix-shell is quite different from nix-build though. A shell doesn't really produce outputs, so it shouldn't mirror nix-build like that.

I agree. Though I think it might actually be nice to provide an out path for the shell.

We're definitely past the point where any detail could become a breaking change, but I'd expect even a naive implementation to fix more problems than it creates. One potential problem we can solve is passing posix signals along to the child process. The siginfo_t sending pid will still be different, but now I'm talking about a niche in a niche. We could add an --exec flag as a contingency.

Suppose I were to try doing temp roots in a following commit, could you do some closer review/collaboration? I don't count myself as a systems programming expert. I'm not sure I could handle signals properly, for instance.

[roberth]

I'm not much of an expert either, so I don't think I'd be super helpful, but I can do a pass of review.

[jsoo1]

Wait a second. I think temproots shouldn't even be required. GC reads /proc/*/{environ,maps} etc to check for live roots anyways:

nix/src/libstore/gc.cc

Line 348 in ffca3e3

void LocalStore::findRuntimeRoots(Roots & roots, bool censor)

and:

$ nix-shell -p tmux --run 'nix-store --query --roots $(type -p tmux) | grep $BASHPID'                                                                               
these paths will be fetched (0.34 MiB download, 0.97 MiB unpacked):
  /nix/store/zfwyd14l1z0sg54d6m7jw43469si1pdw-tmux-3.3a
copying path '/nix/store/zfwyd14l1z0sg54d6m7jw43469si1pdw-tmux-3.3a' from 'https://cache.nixos.org'...
/proc/3277340/environ -> /nix/store/zfwyd14l1z0sg54d6m7jw43469si1pdw-tmux-3.3a

$ nix-store --query --roots /nix/store/zfwyd14l1z0sg54d6m7jw43469si1pdw-tmux-3.3a
=> empty

Edit: So I'm not actually really sure if creating temproots is as much a solution as it is papering over a bug somewhere.

Edit 2: Knowing that temproots may not be required: should --out-link be supported? It does provide a workaround for a bug. I also have a branch with a working child process/explicit temproots but now I'm not sure that should go in...

[jsoo1]

@roberth #7281 for temp roots

[jsoo1]

@thufschmitt

Mh, I couldn't find anything really relevant in #2208, it seems to really be related to keeping the gc root once the shell is closed (which is also something important that we've left aside for too long).

Do you think --out-link is the right flag for this?

[thufschmitt]

@thufschmitt

Mh, I couldn't find anything really relevant in #2208, it seems to really be related to keeping the gc root once the shell is closed (which is also something important that we've left aside for too long).

Do you think --out-link is the right flag for this?

Indeed, it is. The current approach isn't exactly right though, because:

1. It requires the derivation to build, which often won't be the case (mkShell for example produces a failing derivation by design)
2. It will only keep the runtime references of the output, which is the subset of the dependencies of the shell (in the pathological case where the builder is touch $out like you've added it for -p, it probably won't keep anything)

What nix develop (which supports an --out-path flag) and I think lorri do is that they build a modified version of the derivation which retains all the build-time inputs (something along the lines of env > $out might be a good approximation of that).

[bjornfor]

It requires the derivation to build, which often won't be the case (mkShell for example produces a failing derivation by design)

mkShell was thankfully fixed (made buildable) in NixOS/nixpkgs@1e91020.

[jsoo1]

Do you think --out-link is the right flag for this?

Indeed, it is. The current approach isn't exactly right though, because:

1. It requires the derivation to build, which often won't be the case (mkShell for example produces a failing derivation by design)

I mentioned this above as a UX problem . I am not sure this is entirely a problem. For back-compat I could see there being problem cases. But in the back-compat case --no-out-link is still the default.

Two additional things about this:

1. I'm not sure that's true (see https://github.com/NixOS/nixpkgs/blob/0106a6855599fbe26a7c331782d9672f829bd1e1/pkgs/build-support/mkshell/default.nix#L46).
2. As mentioned in mkshell/default.nix, the (non)existence of an output is a nixpkgs implementation detail. I would say nix probably should be less coupled to nixpkgs if at all possible. (I know there is a lot of nixpkgs-specific code in nix already, but I worry those cause other, more problematic back-compat problems).

2. It will only keep the runtime references of the output, which is the subset of the dependencies of the shell (in the pathological case where the builder is touch $out like you've added it for -p, it probably won't keep anything)

What nix develop (which supports an --out-path flag) and I think lorri do is that they build a modified version of the derivation which retains all the build-time inputs (something along the lines of env > $out might be a good approximation of that).

I think this is generally a very hard problem to solve with the tools nix has and with the existence of nuke-references and friends. I would like there to be a solution that would also capture build-time inputs. Does it need to go in as part of this set of patches, though?

Edit: Another thought: this --out-link works the same way that --out-link from nix-build does. I think including build-time inputs might violate the principle of least surprise. If build-time inputs were also kept as gc roots for every other derivation (say by nix-build or nix-store --realize --add-root ...) then I would be for that change here.

[thufschmitt]

1. I'm not sure that's true (see https://github.com/NixOS/nixpkgs/blob/0106a6855599fbe26a7c331782d9672f829bd1e1/pkgs/build-support/mkshell/default.nix#L46).

Indeed. Looks like I'm nearly one year late to the party 😛

2. As mentioned in mkshell/default.nix, the (non)existence of an output is a nixpkgs implementation detail. I would say nix probably should be less coupled to nixpkgs if at all possible. (I know there is a lot of nixpkgs-specific code in nix already, but I worry those cause other, more problematic back-compat problems).

Do you mean that there's no guaranty that out will exist? That's true, but we'd want to set outputs = [ "out" ] anyway because we want to fix the outputs list.

2. It will only keep the runtime references of the output, which is the subset of the dependencies of the shell (in the pathological case where the builder is touch $out like you've added it for -p, it probably won't keep anything)

What nix develop (which supports an --out-path flag) and I think lorri do is that they build a modified version of the derivation which retains all the build-time inputs (something along the lines of env > $out might be a good approximation of that).

I think this is generally a very hard problem to solve with the tools nix has and with the existence of nuke-references and friends. I would like there to be a solution that would also capture build-time inputs. Does it need to go in as part of this set of patches, though?

I don't think it is. Things like nuke-references are external programs that can't interfere here. The only potential limitation would be #7087, but even then we can just make sure that we unset __visibleReferences.

Edit: Another thought: this --out-link works the same way that --out-link from nix-build does. I think including build-time inputs might violate the principle of least surprise. If build-time inputs were also kept as gc roots for every other derivation (say by nix-build or nix-store --realize --add-root ...) then I would be for that change here.

Quite the contrary, I would say. nix-shell and nix-build are conceptually very different (despite sharing the same code), and having nix-shell's --out-path flag mirror nix-build's wouldn't make that much sense imho.

At any rate, this new flag is just combining nix-build --out-path foo && nix-shell into nix-shell --out-path foo, which I think is not worth it given the confusion about the semantics of --out-path.

[jsoo1]

Indeed. Looks like I'm nearly one year late to the party 😛

It fits with my correct-by-construction philosophy, at least 😄 .

(I know there is a lot of nixpkgs-specific code in nix already, but I worry those cause other, more problematic back-compat problems).

Do you mean that there's no guaranty that out will exist? That's true, but we'd want to set outputs = [ "out" ] anyway because we want to fix the outputs list.

I mean things like this:

nix/src/nix-build/nix-build.cc

Line 266 in f225f43

joined << "{...}@args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) \"shell\" { buildInputs = [ ";

which rely on pkgs.runCommand{,NoCC}, etc.

As an aside, because of that line in nix-build.cc, --no-out-link would preserve packages specified with -p which should satisfy your concern that runtime reference wouldn't be preserved here:

2. It will only keep the runtime references of the output, which is the subset of the dependencies of the shell (in the pathological case where the builder is touch $out like you've added it for -p, it probably won't keep anything)

Regarding --out-flags semantics:

What nix develop (which supports an --out-path flag) and I think lorri do is that they build a modified version of the derivation which retains all the build-time inputs (something along the lines of env > $out might be a good approximation of that).

I think this is generally a very hard problem to solve with the tools nix has and with the existence of nuke-references and friends. I would like there to be a solution that would also capture build-time inputs. Does it need to go in as part of this set of patches, though?

I don't think it is. Things like nuke-references are external programs that can't interfere here. The only potential limitation would be #7087, but even then we can just make sure that we unset __visibleReferences.

If nix develop already does this, I can look into making nix-shell's --out-link work more like that one. Because...

Edit: Another thought: this --out-link works the same way that --out-link from nix-build does. I think including build-time inputs might violate the principle of least surprise. If build-time inputs were also kept as gc roots for every other derivation (say by nix-build or nix-store --realize --add-root ...) then I would be for that change here.

Quite the contrary, I would say. nix-shell and nix-build are conceptually very different (despite sharing the same code), and having nix-shell's --out-path flag mirror nix-build's wouldn't make that much sense imho.

... I agree.

At any rate, this new flag is just combining nix-build --out-path foo && nix-shell into nix-shell --out-path foo, which I think is not worth it given the confusion about the semantics of --out-path.

"Just" combining nix-build ... && nix-shell is problematic in a couple ways:

1. nix-shell interpreter scripts cannot nix-build in the interpreter line (at least as far as I know). Meaning that you would have to nix-build ... either before the script is run or as the first line in the script.
2. It is in violation of the principle that nix commands should be atomic

(Edit: I guess if nix-build --out-link might make the command atomic, but there may be some trivial sliver of TOCTOU issues with it)

Edit 2:

not worth it given the confusion about the semantics of --out-path.

Would you reconsider if nix-shell's --out-path semantics were the same as nix develop's?

[roberth]

Adding --out-link to nix-shell is like giving Frankenstein's monster a tail fin. I really appreciate the effort to try to help the monster, but there's no fixing it.
nix-shell already has conflicting requirements, and adding --out-link will only make that worse. The new flag doesn't even really solve the root cause of @jsoo1's problem.
What we should do instead is

1. Fix bugs instead of adding workarounds

2. Deprecate nix-shell in favor of specific commands, some of which are nix run, and some others which require new nix-daemon features. Here's an analysis (probably should be its own issue) of the problem space that nix-shell necessarily fails to cover.

[jsoo1]

I think this change is still worthwhile since #7281 won't merge, but I don't have time to do the work and continue this conversation. Thank you for your consideration @roberth !