Skip to content

Conversation

alexshpilkin
Copy link
Member

Fixes #6500 (the nix develop implementation bug part per my description there, I’m assuming the nix why-depends doc / semantics part will get its own bug)

I really tried to make a test this time, but while I can verify interactively that the problem was present and is now gone, I just can’t seem to get the test suite to either succeed here or to fail on master. FWIW, the simplest thing I tried was

diff --git a/tests/nix-shell.sh b/tests/nix-shell.sh                                                                                                           
index 3241d7a0f..96d6cf063 100644                                                                                                                              
--- a/tests/nix-shell.sh                                                                                                                                       
+++ b/tests/nix-shell.sh                                                                                                                                       
@@ -85,7 +85,7 @@ output=$($TEST_ROOT/spaced\ \\\'\"shell.shebang.rb abc ruby)                                                                                 
 [ "$output" = '-e load(ARGV.shift) -- '"$TEST_ROOT"'/spaced \'\''"shell.shebang                                                                               
.rb abc ruby' ]                                                                                                                                                
                                                                                                                                                               
 # Test 'nix develop'.                                                                                                                                         
-nix develop -f "$shellDotNix" shellDrv -c bash -c '[[ -n $stdenv ]]'                                                                                          
+nix develop -f "$shellDotNix" shellDrv -c bash -c '[[ -n $stdenv ]]' |& grep -qv error                                                                        
                                       
 # Ensure `nix develop -c` preserves stdin                                                                                                                     
 echo foo | nix develop -f "$shellDotNix" shellDrv -c cat | grep -q foo                                                                                        

but that seems to succeed on master for some reason (?!..).

@Artturin
Copy link
Member

Artturin commented May 8, 2022

works

$ nix develop "nixpkgs#bash"
error (ignored): error: argument 'flake:nixpkgs#bashInteractive' should evaluate to one store path

[artturin@artturinlinux:~/nixgits/my-nixpkgs]$
exit
$ nix run "github:alexshpilkin/nix/fix-6500" -- develop "nixpkgs#bash"

[artturin@artturinlinux:~/nixgits/my-nixpkgs]$
exit

installable->nixpkgsFlakeRef(),
"bashInteractive",
DefaultOutputs(),
OutputNames{"out"},
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that this will break if bashInteractive ever changes its outputs (e.g. moves bash to a bin output).

Copy link
Member Author

Choose a reason for hiding this comment

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

It will also break if bashInteractive is ever renamed, and there’s a big fat warning comment about that in Nixpkgs, so I assumed that would be okay? The warning should probably be extended to mention the output name as well, of course.

@edolstra
Copy link
Member

edolstra commented May 9, 2022

A more future-proof fix is to use toStorePaths() instead of toStorePath(), and look in each output for a bin/bash.

@thufschmitt
Copy link
Member

@edolstra would it make sense to merge this as-it-is and add the proper fix later? This is a rather big breakage and it’s really sad to keep master in this state

@edolstra
Copy link
Member

Thanks, I've fixed it in a different way in #6518.

@edolstra edolstra closed this May 10, 2022
@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

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.

error: bashInteractive should evaluate to one store path

5 participants