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: reflect command line order in PATH order #9648

Merged
merged 3 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/libcmd/installables.cc
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,7 @@ BuiltPaths Installable::toBuiltPaths(
}
}

StorePathSet Installable::toStorePaths(
StorePathSet Installable::toStorePathSet(
ref<Store> evalStore,
ref<Store> store,
Realise mode, OperateOn operateOn,
Expand All @@ -729,13 +729,27 @@ StorePathSet Installable::toStorePaths(
return outPaths;
}

StorePaths Installable::toStorePaths(
ref<Store> evalStore,
ref<Store> store,
Realise mode, OperateOn operateOn,
const Installables & installables)
{
StorePaths outPaths;
for (auto & path : toBuiltPaths(evalStore, store, mode, operateOn, installables)) {
auto thisOutPaths = path.outPaths();
outPaths.insert(outPaths.end(), thisOutPaths.begin(), thisOutPaths.end());
}
return outPaths;
}

StorePath Installable::toStorePath(
ref<Store> evalStore,
ref<Store> store,
Realise mode, OperateOn operateOn,
ref<Installable> installable)
{
auto paths = toStorePaths(evalStore, store, mode, operateOn, {installable});
auto paths = toStorePathSet(evalStore, store, mode, operateOn, {installable});

if (paths.size() != 1)
throw Error("argument '%s' should evaluate to one store path", installable->what());
Expand Down
9 changes: 8 additions & 1 deletion src/libcmd/installables.hh
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,14 @@ struct Installable
const Installables & installables,
BuildMode bMode = bmNormal);

static std::set<StorePath> toStorePaths(
static std::set<StorePath> toStorePathSet(
ref<Store> evalStore,
ref<Store> store,
Realise mode,
OperateOn operateOn,
const Installables & installables);

static std::vector<StorePath> toStorePaths(
ref<Store> evalStore,
ref<Store> store,
Realise mode,
Expand Down
4 changes: 2 additions & 2 deletions src/nix/develop.cc
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ struct Common : InstallableCommand, MixProfile
for (auto & [installable_, dir_] : redirects) {
auto dir = absPath(dir_);
auto installable = parseInstallable(store, installable_);
auto builtPaths = Installable::toStorePaths(
auto builtPaths = Installable::toStorePathSet(
getEvalStore(), store, Realise::Nothing, OperateOn::Output, {installable});
for (auto & path: builtPaths) {
auto from = store->printStorePath(path);
Expand Down Expand Up @@ -631,7 +631,7 @@ struct CmdDevelop : Common, MixEnvironment

bool found = false;

for (auto & path : Installable::toStorePaths(getEvalStore(), store, Realise::Outputs, OperateOn::Output, {bashInstallable})) {
for (auto & path : Installable::toStorePathSet(getEvalStore(), store, Realise::Outputs, OperateOn::Output, {bashInstallable})) {
auto s = store->printStorePath(path) + "/bin/bash";
if (pathExists(s)) {
shell = s;
Expand Down
9 changes: 6 additions & 3 deletions src/nix/run.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,15 @@ struct CmdShell : InstallablesCommand, MixEnvironment

setEnviron();

auto unixPath = tokenizeString<Strings>(getEnv("PATH").value_or(""), ":");
std::vector<std::string> pathAdditions;

while (!todo.empty()) {
auto path = todo.front();
todo.pop();
if (!done.insert(path).second) continue;

if (true)
unixPath.push_front(store->printStorePath(path) + "/bin");
pathAdditions.push_back(store->printStorePath(path) + "/bin");

auto propPath = CanonPath(store->printStorePath(path)) + "nix-support" + "propagated-user-env-packages";
if (auto st = accessor->maybeLstat(propPath); st && st->type == SourceAccessor::tRegular) {
Expand All @@ -131,7 +131,10 @@ struct CmdShell : InstallablesCommand, MixEnvironment
}
}

setenv("PATH", concatStringsSep(":", unixPath).c_str(), 1);
auto unixPath = tokenizeString<Strings>(getEnv("PATH").value_or(""), ":");
unixPath.insert(unixPath.begin(), pathAdditions.begin(), pathAdditions.end());
auto unixPathString = concatStringsSep(":", unixPath);
setenv("PATH", unixPathString.c_str(), 1);

Strings args;
for (auto & arg : command) args.push_back(arg);
Expand Down
16 changes: 16 additions & 0 deletions tests/functional/shell-hello.nix
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,20 @@ with import ./config.nix;
chmod +x $dev/bin/hello2
'';
};

salve-mundi = mkDerivation {
name = "salve-mundi";
outputs = [ "out" ];
meta.outputsToInstall = [ "out" ];
buildCommand =
''
mkdir -p $out/bin

cat > $out/bin/hello <<EOF
#! ${shell}
echo "Salve Mundi from $out/bin/hello"
EOF
chmod +x $out/bin/hello
'';
};
}
8 changes: 8 additions & 0 deletions tests/functional/shell.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

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;

# Test that command line attribute ordering is reflected in the PATH
# https://github.com/NixOS/nix/issues/7905
nix shell -f shell-hello.nix hello salve-mundi -c hello | grep 'Hello World'
nix shell -f shell-hello.nix salve-mundi hello -c hello | grep 'Salve Mundi'
fi

requireSandboxSupport

chmod -R u+w $TEST_ROOT/store0 || true
Expand Down
Loading