Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#! nix-shell -i bash use bash from shell.nix #6472

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Radvendii
Copy link
Contributor

@Radvendii Radvendii commented May 2, 2022

When nix-shell is non-interactive, we don't need to make sure we launch
an interactive bash, and it's more important to get the pinned bash

I'm a little worried this will break NixOS/nixpkgs#27493

@domenkozar (from that nixpkgs issue)

@edolstra @thufschmitt (discussed the problem & solution earlier)

TODO:

  • communicate better what is going on to the user. if they unset NIX_PATH, they still get a scary error message despite it not really being an issue in shebang mode.

@Radvendii Radvendii changed the title Draft: #! nix-shell -i bash use bash from shell.nix #! nix-shell -i bash use bash from shell.nix May 3, 2022
@Radvendii
Copy link
Contributor Author

Radvendii commented May 3, 2022

Oh. As I feared there's an error on Mac. Does anyone who uses mac have an insight into what's going on?

error is here https://github.com/NixOS/nix/runs/6275913110?check_suite_focus=true#step:6:1635

@Radvendii
Copy link
Contributor Author

never mind? error seems to have magically disappeared.

@thufschmitt
Copy link
Member

thufschmitt commented May 4, 2022

never mind? error seems to have magically disappeared.

I’ve restarted the CI ;)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-30/19112/1

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Looks good, I’d just like to see a little test showing the various combinations of bash-es that can happen if that’s not too tricky to do (maybe creating different symlinks to the bash available in the test sandbox, and using $BASH to know which one was used)

@Radvendii
Copy link
Contributor Author

Right now $BASH and $SHELL are still set to the same value they were before. This was a while ago, so I don't remember our reasoning exactly. Was it deliberate that we left the SHELL= and BASH= lines in?

It would be nice if we could go further and remove the error message (and the whole code-path that leads up to it) in the non-interactive case:

error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I)

       at «string»:1:9:

            1| (import <nixpkgs> {}).bashInteractive
             |         ^
will use bash from your environment

Since it's not really an error, and we're not using bash from the environment. I am wary though because this code is reused by a bunch of different commands.

I will add tests, and then maybe we should merge this PR as is, and change other things, like the error message, separately?

@domenkozar
Copy link
Member

+1 for removing that error message, I set NIX_PATH to be empty on all my machines/environments and it's annoying to see that error pop up.

@fricklerhandwerk fricklerhandwerk added nix-shell nix-shell, nix develop, nix print-dev-env, etc error-messages Confusing messages and better diagnostics labels Mar 3, 2023
@thufschmitt thufschmitt self-assigned this Mar 17, 2023
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 22, 2023

Triaged in the Nix team meeting 2023-03-17:

We'll wait for tests to be added. @Radvendii do you still have capacity to pursue this?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-03-17-nix-team-meeting-minutes-41/26614/1

@fricklerhandwerk fricklerhandwerk added the UX The way in which users interact with Nix. Higher level than UI. label Dec 18, 2023
@Radvendii Radvendii requested a review from edolstra as a code owner June 23, 2024 23:48
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jun 23, 2024
@Radvendii
Copy link
Contributor Author

So I stumbled across this again 2 years later and finally sat down to tackle it. I realised I'd gotten a couple things wrong.

  1. What I thought had to do with interactive/non-interactive really had to do with whether we're in a shebang. If you run nix shell --run "<command>", it might be relevant which bash is running that <command>. If you #!nix-shell -i ruby, it doesn't really matter which bash is executing exec ruby.

  2. It still doesn't sit right to me that nix-shell --run "<command>" won't use the bash provided by your shell.nix. I don't think there's a great way to solve this though. We just don't have access to it at the right time.

  3. The whole PATH=" + shellEscape(dirOf(*shell)) + ":$PATH" thing going on has nothing to do with anything else, and I'm starting to think it should just go away entirely.

    What it does is set it up so that if you run bash from inside a nix-shell, it will get you the same bash that you were already in. I'm guessing this is in place so you get bashInteractive rather than some non-interactive bash provided by stdenv.mkDerivation, but I would argue that's an issue with mkShell, which should provide an interactive bash by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Confusing messages and better diagnostics nix-shell nix-shell, nix develop, nix print-dev-env, etc UX The way in which users interact with Nix. Higher level than UI. with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants