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

Always use "bash" for dev-shell #3107

Closed
wants to merge 1 commit into from

Conversation

matthewbauer
Copy link
Member

Unfortunately zshell does not support the --rcfile arg. Best to
hardcode bash here. Fixes #2965

@grahamc
Copy link
Member

grahamc commented Sep 26, 2019

It would be nice to support more than just bash. I wonder if zsh has something like rcfile?

Unfortunately zshell does not support the --rcfile arg. Best to
hardcode bash here.
@matthewbauer
Copy link
Member Author

@zimbatm
Copy link
Member

zimbatm commented Sep 27, 2019

also all the phases contain bash-specific code in nixpkgs.

@zimbatm zimbatm closed this Sep 27, 2019
@zimbatm zimbatm reopened this Sep 27, 2019
@zimbatm
Copy link
Member

zimbatm commented Sep 27, 2019

oops.

I wonder if we should not use the builder attribute so that if your builder is python, it would start a python shell.

We would need a new interface that says that the builder can accept a --shell argument I think, so that it can load all the libraries (or stdenv)

@edolstra
Copy link
Member

We can't use the builder attribute because that would give us a non-interactive bash.

@edolstra
Copy link
Member

Maybe we should copy the nix-shell behavior, which is to use bashInteractive from nixpkgs. The modern variant would be to fetch nixpkgs#bashInteractive. This ensures that things work on macOS (where the system bash is ancient) and that nested shells work properly.

src/nix/shell.cc Outdated
@@ -245,7 +245,7 @@ struct CmdDevShell : Common

stopProgressBar();

auto shell = getEnv("SHELL", "bash");
auto shell = "bash";
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't compile because it infers the wrong type for shell (char * rather than std::string).

@zimbatm
Copy link
Member

zimbatm commented Sep 30, 2019

Maybe we should copy the nix-shell behavior, which is to use bashInteractive from nixpkgs. The modern variant would be to fetch nixpkgs#bashInteractive. This ensures that things work on macOS (where the system bash is ancient) and that nested shells work properly.

It's quite common to pin nixpkgs using builtins.fetchTarball in the code and completely ignore NIX_PATH. So this might risk of loading a version of bash coming from another channel. I guess once flakes is finished it will be easier to unify everything under one roof.

@edolstra
Copy link
Member

edolstra commented Oct 1, 2019

@zimbatm Ideally we would use the nixpkgs flake from the inputs of the top-level flake.

BTW, commit 9b9de3a makes it feasible to support other shells since it separately extracts environment variables and bash functions.

@balsoft
Copy link
Member

balsoft commented May 22, 2020

@zimbatm

So this might risk of loading a version of bash coming from another channel

Why is this an issue for an interactive shell? I would imagine that it's better to do this than to use bash from path in most cases. Am I missing something?

@matthewbauer
Copy link
Member Author

Closing now that #3565 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants