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.

Sign up for GitHub

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

Jump to bottom

nix shell: reflect command line order in PATH order #9648

Merged
merged 3 commits into from
Jan 9, 2024
Merged

nix shell: reflect command line order in PATH order #9648

merged 3 commits into from
Jan 9, 2024

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Dec 20, 2023

Motivation

I wanted to help with the CLI stabilization project, and this was a relatively easy problem to pick from the project board.

Context

Fixes #7905.

The first commit is a test that will fail without the second commit (to demonstrate the problem and ensure we don't regress).

The second switches CmdShell to use an Installable::toStorePaths alternative that returns an unsorted collection.

I'm under no illusions that my outPaths2 and toStorePaths2 functions will be accepted as-is. However, a solution just like them will be necessary: std::set is sorted (according to https://en.cppreference.com/w/cpp/container/set), which is what causes this problem in the first place and means that anything that returns a std::set needs to be replaced with an unordered alternative. I went with StorePaths (std::vector<StorePath>) because it already existed.

I'm open to any suggestions on how to improve this.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Dec 20, 2023
@cole-h cole-h mentioned this pull request Dec 20, 2023
Closed
roberth
roberth reviewed Dec 21, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I suppose this gives us a homomorphic law:

nix shell A -- nix shell B == nix shell A B

That is, unless we do something non-trivial to combine A and B that I'm unaware of right now.

src/libcmd/installables.hh Outdated Show resolved Hide resolved
src/libcmd/built-path.cc Outdated Show resolved Hide resolved
tests/functional/shell.sh
@@ -10,6 +10,14 @@ nix shell -f shell-hello.nix hello -c hello NixOS | grep 'Hello NixOS'
nix shell -f shell-hello.nix hello^dev -c hello2 | grep 'Hello2'
nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2'


if isDaemonNewer "2.20.0pre20231220"; then
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the store layer is affected. All changes were client side.
This test should run unconditionally.

Copy link
Member Author

@cole-h cole-h Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We test older versions of the CLI against newer daemons, so while this isn't daemon-side, older CLIs obviously don't have this property (the ordering, or lack thereof), causing the install tests to fail on those cross-comparisons. It would be better to introduce a isCliNewerThan but I didn't want to do that until I was sure this was the right direction.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC/IIUC when we run against a newer version, the tests consists of

  • old tests suite
  • old client
  • new daemon

Ie the test suite and client are always in sync.
Did you see it fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did, yes:

$ nix build .#checks.x86_64-linux.installTests -L
[...snip...]
nix-tests> ran test tests/functional/shell.sh... [FAIL]
nix-tests>     +(shell.sh:1) source common.sh
nix-tests>     ++(common.sh:1) set -eu -o pipefail
nix-tests>     ++(common.sh:3) [[ -z '' ]]
nix-tests>     ++(common.sh:5) COMMON_SH_SOURCED=1
nix-tests>     ++++(common.sh:7) dirname common.sh
nix-tests>     +++(common.sh:7) readlink -f .
nix-tests>     ++(common.sh:7) source /build/source/tests/functional/common/vars-and-functions.sh
nix-tests>     +++(/build/source/tests/functional/common/vars-and-functions.sh:1) set -eu -o pipefail
nix-tests>     +++(/build/source/tests/functional/common/vars-and-functions.sh:3) [[ -z '' ]]
nix-tests>     +++(/build/source/tests/functional/common/vars-and-functions.sh:5) COMMON_VARS_AND_FUNCTIONS_SH_SOURCED=1
nix-tests>     +++(/build/source/tests/functional/common/vars-and-functions.sh:7) set +x
nix-tests>     +++(/build/source/tests/functional/common/vars-and-functions.sh:274) trap onError ERR
nix-tests>     ++(common.sh:8) [[ -n /nix/store/kh7qf50abg77aglrvmx0fnlr71y4p6sh-nix-2.15.3 ]]
nix-tests>     ++(common.sh:9) startDaemon
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:93) [[ '' != '' ]]
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:97) rm -f /build/nix-test/shell/dSocket
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:98) PATH=/nix/store/kh7qf50abg77aglrvmx0fnlr71y4p6sh-nix-2.15.3/bin:/nix/store/i8jhzjq8kb3vn5j1z3mx22kccfy7r4hz-nix-2.20.0pre20231221_a81fb26/bin:/nix/store/41bd0qrlf3gkbvkimlshh6cglbss6dmz-nix-tests-2.20.0pre20231221_a81fb26-against-2.15.3-2.20.0/bin:/nix/store/nhp2wyp7wgar861mqaysc8fcl77q8m7w-autoconf-2.71/bin:/nix/store/2kwsypbmd1hxs9i3x819awr92rd0i716-automake-1.16.5/bin:/nix/store/nyv90w9cgrqna86fxxzd6y3dqyh17732-gettext-0.21/bin:/nix/store/d8k5sn9yxp6ncarn2s8vswdv6ahdqjvi-libtool-2.4.7/bin:/nix/store/cqcyk8rpz971xsb2f6rpp365p68sgr67-gnum4-1.4.19/bin:/nix/store/lysbgkhsin7bzhlmvr3ml7kq9b4nifyn-file-5.44/bin:/nix/store/7l5gbdz3rcab57r24arg61i4vavpf2cz-pkg-config-wrapper-0.29.2/bin:/nix/store/wgqafnfqzqa6p51bb32wdmsplgz62nnj-jq-1.6-bin/bin:/nix/store/k0my1jwjxmc9b07gz1fa1c041dd0zns7-util-linux-2.38.1-bin/bin:/nix/store/gwf21xyl0x0bhgapi66avzf6qmddr44s-patchelf-0.15.0/bin:/nix/store/3hbxw05vfs6m1155xa6pvribs3bv777n-gcc-wrapper-12.2.0/bin:/nix/store/jgdz5wfqc6q6qhy17lsqnfpbkfi1fcar-gcc-12.2.0/bin:/nix/store/sb5sf8pnbb5dxgr5p8qm344ky8k7f62r-glibc-2.37-45-bin/bin:/nix/store/w8vm09hri2zz7yacryzzzxvsapik4ps4-coreutils-9.1/bin:/nix/store/syp0507hrby53pprfi7n4wppms3b7ba2-binutils-wrapper-2.40/bin:/nix/store/3r87a2wq1w4l66wnsm7rqvy608mx23h6-binutils-2.40/bin:/nix/store/w8vm09hri2zz7yacryzzzxvsapik4ps4-coreutils-9.1/bin:/nix/store/4cxs4cigh2zdxvma52ygm3mh2igq70iw-findutils-4.9.0/bin:/nix/store/vikpb6rhmi8zzgsx8syjng9dic8dplm7-diffutils-3.9/bin:/nix/store/4mca20b13q88s6llkr8mc468rh9l9bmr-gnused-4.9/bin:/nix/store/b4in4hmq54h6l34a0v6ha40z97c0lzw2-gnugrep-3.7/bin:/nix/store/p7v8rc82yi4zngjw2z7isgqvfi32l1aj-gawk-5.2.1/bin:/nix/store/2gcfy5f68y7gkryp3wrjmvlci42qb61a-gnutar-1.35/bin:/nix/store/5xzh0z66hpcd3k578my76lx4qgra334q-gzip-1.12/bin:/nix/store/6d9vlwwy7kvxjqmqg4cldgg9mr8nflff-bzip2-1.0.8-bin/bin:/nix/store/qlcm8dzsaa6wyycmx7in8rz6x3pzf6zf-gnumake-4.4.1/bin:/nix/store/0rwyq0j954a7143p0wzd4rhycny8i967-bash-5.2-p15/bin:/nix/store/0g52i0ih3h0d9s40m84gksi2jjm4k3bf-patch-2.7.6/bin:/nix/store/agry2lhy6ilgyacx75w2ycqsdfwr513f-xz-5.4.3-bin/bin:/nix/store/rgg874xkzlfmkzkm7911dwnihswkf54g-file-5.44/bin
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:98) nix-daemon
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:99) _NIX_TEST_DAEMON_PID=12328
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:100) export _NIX_TEST_DAEMON_PID
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:101) (( i = 0 ))
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:101) (( i < 300 ))
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:102) [[ -S /build/nix-test/shell/dSocket ]]
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:106) sleep 0.1
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:101) (( i++ ))
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:101) (( i < 300 ))
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:102) [[ -S /build/nix-test/shell/dSocket ]]
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:103) DAEMON_STARTED=1
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:104) break
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:108) [[ -z x ]]
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:111) trap killDaemon EXIT
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:113) NIX_REMOTE_OLD=
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:114) export NIX_REMOTE=daemon
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:114) NIX_REMOTE=daemon
nix-tests>     +(shell.sh:3) clearStore
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:74) echo 'clearing store...'
nix-tests>     clearing store...
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:75) chmod -R +w /build/nix-test/shell/store
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:76) rm -rf /build/nix-test/shell/store
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:77) mkdir /build/nix-test/shell/store
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:78) rm -rf /build/nix-test/shell/var/nix
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:79) mkdir /build/nix-test/shell/var/nix
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:80) clearProfiles
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:69) profiles=/build/nix-test/shell/test-home/.local/state/nix/profiles
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:70) rm -rf /build/nix-test/shell/test-home/.local/state/nix/profiles
nix-tests>     +(shell.sh:4) clearCache
nix-tests>     +(/build/source/tests/functional/common/vars-and-functions.sh:84) rm -rf /build/nix-test/shell/binary-cache
nix-tests>     +(shell.sh:6) nix shell -f shell-hello.nix hello -c hello
nix-tests>     +(shell.sh:6) grep 'Hello World'
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 13045, user nixbld (trusted)
nix-tests>     this derivation will be built:
nix-tests>       /build/nix-test/shell/store/2lgrfx1swfjzg3xffkf0yspag2plkhba-hello.drv
nix-tests>     building '/build/nix-test/shell/store/2lgrfx1swfjzg3xffkf0yspag2plkhba-hello.drv'...
nix-tests>     Hello World from /build/nix-test/shell/store/bdhdwkp91gbpkm7hsn5p3bc0qzqlg9b8-hello/bin/hello
nix-tests>     +(shell.sh:7) nix shell -f shell-hello.nix hello -c hello NixOS
nix-tests>     +(shell.sh:7) grep 'Hello NixOS'
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 13432, user nixbld (trusted)
nix-tests>     Hello NixOS from /build/nix-test/shell/store/bdhdwkp91gbpkm7hsn5p3bc0qzqlg9b8-hello/bin/hello
nix-tests>     +(shell.sh:10) nix shell -f shell-hello.nix 'hello^dev' -c hello2
nix-tests>     +(shell.sh:10) grep Hello2
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 13606, user nixbld (trusted)
nix-tests>     Hello2
nix-tests>     +(shell.sh:11) nix shell -f shell-hello.nix 'hello^*' -c hello2
nix-tests>     +(shell.sh:11) grep Hello2
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 13730, user nixbld (trusted)
nix-tests>     Hello2
nix-tests>     +(shell.sh:17) nix shell -f shell-hello.nix hello salve-mundi -c hello
nix-tests>     +(shell.sh:17) grep 'Hello World'
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 13827, user nixbld (trusted)
nix-tests>     this derivation will be built:
nix-tests>       /build/nix-test/shell/store/canb3m121mj40xjf9yzlss4ssxd0jhyi-salve-mundi.drv
nix-tests>     building '/build/nix-test/shell/store/canb3m121mj40xjf9yzlss4ssxd0jhyi-salve-mundi.drv'...
nix-tests>     Hello World from /build/nix-test/shell/store/bdhdwkp91gbpkm7hsn5p3bc0qzqlg9b8-hello/bin/hello
nix-tests>     +(shell.sh:18) nix shell -f shell-hello.nix salve-mundi hello -c hello
nix-tests>     +(shell.sh:18) grep 'Salve Mundi'
nix-tests>     warning: you don't have Internet access; disabling some network-dependent features
nix-tests>     accepted connection from pid 14062, user nixbld (trusted)
nix-tests>     ++(shell.sh:18) onError
nix-tests>     ++(/build/source/tests/functional/common/vars-and-functions.sh:237) set +x
nix-tests>     shell.sh: test failed at:
nix-tests>       main in shell.sh:18
nix-tests> make: *** [mk/lib.mk:107: tests/functional/shell.sh.test] Error 1
nix-tests> make: *** Waiting for unfinished jobs....
[...snip...]
error: builder for '/nix/store/zhlyr6280ysidv928rvn09cpiwamfvv8-nix-tests-2.20.0pre20231221_a81fb26-against-2.15.3-2.20.0.drv' failed with exit code 2;

@cole-h
Copy link
Member Author

cole-h commented Dec 21, 2023

I suppose this gives us a homomorphic law:

nix shell A -- nix shell B == nix shell A B

Actually, it's the other way around, since we prepend the paths ahead of the existing PATH -- it would instead be:

nix shell A -- nix shell B == nix shell B A

I can't think of any way around this, since we want to prepend the paths to ensure the installable is used before anything that may already be in the environment -- which includes other installables from previous nix shell invocations.

(I tested on this branch by adding nix shell -f shell-hello.nix salve-mundi -c nix shell -f shell-hello.nix hello -c hello | grep 'Salve Mundi' to the existing tests and observing that it still prints out "Hello World", until I change the order where it will print out the desired "Salve Mundi")

@cole-h
nix shell: reflect command line order in PATH order
f4454aa 
Prior to this change, Nix would prepend every installable to the PATH
list in order to ensure that installables appeared before the current
PATH from the ambient environment.

With this change, all the installables are still prepended to the PATH,
but in the same order as they appear on the command line. This means
that the first of two packages that expose an executable `hello` would
appear in the PATH first, and thus be executed first.

See the test in the prior commit for a more concrete example.
@thufschmitt thufschmitt added this to the CLI Stabilisation milestone Dec 22, 2023
@edolstra edolstra merged commit b91c935 into NixOS:master Jan 9, 2024
8 checks passed
@cole-h cole-h deleted the nix-shell-ordering branch January 9, 2024 18:05
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Merge pull request NixOS#9648 from cole-h/nix-shell-ordering
1809841 
nix shell: reflect command line order in PATH order

(cherry picked from commit b91c935)
Change-Id: If16c120bb74857c2817366e74e5b0877eb997260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees
No one assigned
Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Nix team
Archived in project
Milestone
nix-command stabilisation
Development

Successfully merging this pull request may close these issues.

nix shell does not insert PATH items in command line order
4 participants
@cole-h @roberth @edolstra @thufschmitt