nix: develop: use SHELL from rc script

[bobvanderlinden]

Motivation

Currently the SHELL environment variable is ignored in nix-develop. This means the variable is inherited from the user's shell.

This goes wrong when the SHELL variable refers to a different kind of shell like fish. Bash is started, while SHELL still refers to a fish shell. Configure scripts will fail in some cases.

I do have to say that I'm unsure about SHELLOPTS, but since the SHELL variable is changing I presume SHELLOPTS might be invalid for a different shell.

I contemplated about BASHOPTS and thought it was related to the SHELL. Having BASHOPTS from a different SHELL seemed error-prone. So it seemed sensible to also inherit BASHOPTS.

Context

Resolves #7971

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

[iFreilicht]

Tested and seems to work perfectly for SHELL:

$ echo $SHELL
/bin/zsh
$ echo $SHELLOPTS

$ echo $BASHOPTS

$
$ nix develop  # 2.15.1
felix@mac:~/repos/nix$ echo $SHELL
/bin/zsh
felix@mac:~/repos/nix$ echo $BASHOPTS
checkwinsize:cmdhist:complete_fullquote:expand_aliases:extquote:force_fignore:globasciiranges:histappend:hostcomplete:interactive_comments:progcomp:promptvars:sourcepath
felix@mac:~/repos/nix$ echo $SHELLOPTS
braceexpand:emacs:hashall:histexpand:history:interactive-comments:monitor
$
$ nix run github:bobvanderlinden/nix/pr-shell-env -- develop
felix@mac:~/repos/nix$ echo $SHELL
/nix/store/49wn01k9yikhjlxc1ym5b6civ29zz3gv-bash-5.1-p16/bin/bash
felix@mac:~/repos/nix$ echo $BASHOPTS
checkwinsize:cmdhist:complete_fullquote:expand_aliases:extquote:force_fignore:globasciiranges:histappend:hostcomplete:interactive_comments:progcomp:promptvars:sourcepath
felix@mac:~/repos/nix$ echo $SHELLOPTS
braceexpand:emacs:hashall:histexpand:history:interactive-comments:monitor

I'm not sure what's going on with SHELLOPTS and BASHOPTS, they seem to already be set properly. AFAIK, those are set by bash itself when it starts, so not sure if removing them from the ignore list makes sense.

[bobvanderlinden]

I see I had forgotten about this PR.

I'm not sure what's going on with SHELLOPTS and BASHOPTS, they seem to already be set properly. AFAIK, those are set by bash itself when it starts, so not sure if removing them from the ignore list makes sense.

Looking at this again I see that it might indeed be safer to keep the *OPTS. Thanks for the suggestion. I have restored these again.

@edolstra @fricklerhandwerk could this be looked at during the next Nix team meeting?

[fricklerhandwerk]

I think that's a reasonable fix.

In the big picture I'd like us to see it as a stopgap though, and nudge derivation authors, such as in Nixpkgs, to provide an executable that invokes a shell. Then we can do nix run on it and nix develop will be a slightly fancy alias to nix run <something that evaluates to an attrset>.shell that can fall back to the current magic mkShell logic for backward compatibility.

[fricklerhandwerk]

Triaged in Nix team meeting:

• this needs a test, then we can merge
• assigned to @edolstra to review in detail

[edolstra]

Won't this cause nix develop to use the non-interactive bash from the dev shell?

[nixos-discourse]

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

https://discourse.nixos.org/t/2023-11-10-nix-team-meeting-minutes-102/35379/1

[bobvanderlinden]

Won't this cause nix develop to use the non-interactive bash from the dev shell?

I think I see now what you mean by non-interactive bash. nix develop uses flake:nixpkgs#bashInteractive to start a shell if available in the inputs of the flake.

Though I still don't understand how passing through SHELL would change the shell Nix uses for nix develop?

this needs a test, then we can merge

Since I didn't see tests regarding nix develop without -c already, I introduced tests/flakes/develop.sh. It is still WIP as it seems to behave unexpectedly as it always sets SHELL to /run/current-system/sw/bin/fish, even though SHELL was something entirely different. I'm guessing it has something to do with bashInteractive being a 'mock' inside the test instead of a real interactive bash shell (like in nixpkgs). After half a day looking into this problem I'm quite confused. Suggestions of any kind are very welcome 😅

[iFreilicht]

So I tried it out again, and it seems to still work like before 👍

One thing I did notice is that this can create an opposite problem for everyone using nix develop -c zsh:

$ nix develop -c zsh
$ echo $SHELL
/nix/store/q0l990ga8fly926kc4sw5svjf12zfscf-bash-5.2-p15/bin/bash

I'm not sure if this is a huge issue, but it doesn't seem to affect my setup in any way. Additionally, using direnv fixes this quite nicely.

[bobvanderlinden]

So I tried it out again, and it seems to still work like before 👍

Thanks!

One thing I did notice is that this can create an opposite problem for everyone using nix develop -c zsh:

$ nix develop -c zsh
$ echo $SHELL
/nix/store/q0l990ga8fly926kc4sw5svjf12zfscf-bash-5.2-p15/bin/bash

I'm not sure if this is a huge issue, but it doesn't seem to affect my setup in any way.

Hmm, I think this is still correct behavior. zsh is only executed as a command, it isn't the shell that is required to build the derivation. SHELL should point to the shell that the package derivation uses as its shell (which usually is bash).

The main case I ran into is where make uses SHELL to execute its sub-commands. The Makefile expects the shell to be the one defined in the derivation. Not nushell/fish that are likely incompatible with the commands in Makefile.

[iFreilicht]

Hm, I don't really understand why the tests are failing inside CI. I tried it out on macOS and Linux inside nix develop --ignore-environment and checked that lsof wasn't available there, and they passed, as you would expect. :/

[bobvanderlinden]

Hm, I don't really understand why the tests are failing inside CI. I tried it out on macOS and Linux inside nix develop --ignore-environment and checked that lsof wasn't available there, and they passed, as you would expect. :/

I can comment out that check, as it isn't directly related to this PR. It was merely to check which shell is executed by nix develop itself, not the behavior of the SHELL variable.

[iFreilicht]

Yeah I think that would be ok. It would be nice to add this test again later to avoid a regression there.

[iFreilicht]

I'm happy with this now. LGTM!

[bobvanderlinden]

Awesome, thanks! 👍

[Ifropc]

Hmm, I think this is still correct behavior. zsh is only executed as a command, it isn't the shell that is required to build the derivation. SHELL should point to the shell that the package derivation uses as its shell (which usually is bash).

But isn't the running shell zsh, and so $SHELL should point to it?
Sorry for reviving the thread.
Maybe I'm missing something, but I'd like to be able to get currently running shell, which seems to be the $SHELL var?
I'm setting up my devshell and would like to keep dependencies minimal (e.g. just nix installed), and ideally I don't want to make other devs to install extra stuff such as direnv (which I couldn't get to make $SHELL to work either tbh)
Are there any alternative methods? (I simply want to know the currently running shell)
Not sure if it's a right place to ask, but it seems to be related to the issue.

[iFreilicht]

There are other methods, see this SO answer. It also explains the caveat with using $SHELL. This sort of question is better suited to the forum at discourse.nixos.org