nix-shell: look up shell.nix when argument is a directory (NixOS#…

…11057)

* Refactor: rename runEnv -> isNixShell

* Refactor: rename left -> remainingArgs

* nix-build.cc: Refactor: extract baseDir variable

* nix-build.cc: Refactor: extract sourcePath, resolvedPath variables

* nix-shell: Look for shell.nix when directory is specified

* Add legacy setting: nix-shell-always-looks-for-shell-nix

* rl-next: Add note about shell.nix lookups

* tests/functional/shell.nix: Implement runHook for dummy stdenv

doc/manual/rl-next/nix-shell-looks-for-shell-nix.md

@@ -0,0 +1,28 @@
+---
+synopsis: "`nix-shell <directory>` looks for `shell.nix`"
+significance: significant
+issues:
+- 496
+- 2279
+- 4529
+- 5431
+- 11053
+prs:
+- 11057
+---
+
+`nix-shell $x` now looks for `$x/shell.nix` when `$x` resolves to a directory.
+
+Although this might be seen as a breaking change, its primarily interactive usage makes it a minor issue.
+This adjustment addresses a commonly reported problem.
+
+This also applies to `nix-shell` shebang scripts. Consider the following example:
+
+```shell
+#!/usr/bin/env nix-shell
+#!nix-shell -i bash
+```
+
+This will now load `shell.nix` from the script's directory, if it exists; `default.nix` otherwise.
+
+The old behavior can be opted into by setting the option [`nix-shell-always-looks-for-shell-nix`](@docroot@/command-ref/conf-file.md#conf-nix-shell-always-looks-for-shell-nix) to `false`.

src/libcmd/common-eval-args.cc

@@ -11,6 +11,8 @@
 #include "command.hh"
 #include "tarball.hh"
 #include "fetch-to-store.hh"
+#include "compatibility-settings.hh"
+#include "eval-settings.hh"
 
 namespace nix {
 
@@ -33,6 +35,11 @@ EvalSettings evalSettings {
 
 static GlobalConfig::Register rEvalSettings(&evalSettings);
 
+CompatibilitySettings compatibilitySettings {};
+
+static GlobalConfig::Register rCompatibilitySettings(&compatibilitySettings);
+
+
 MixEvalArgs::MixEvalArgs()
 {
     addFlag({

src/libcmd/common-eval-args.hh

@@ -13,6 +13,7 @@ namespace nix {
 class Store;
 class EvalState;
 struct EvalSettings;
+struct CompatibilitySettings;
 class Bindings;
 struct SourcePath;
 
@@ -21,6 +22,11 @@ struct SourcePath;
  */
 extern EvalSettings evalSettings;
 
+/**
+ * Settings that control behaviors that have changed since Nix 2.3.
+ */
+extern CompatibilitySettings compatibilitySettings;
+
 struct MixEvalArgs : virtual Args, virtual MixRepair
 {
     static constexpr auto category = "Common evaluation options";

src/libcmd/compatibility-settings.hh

@@ -0,0 +1,19 @@
+#pragma once
+#include "config.hh"
+
+namespace nix {
+struct CompatibilitySettings : public Config
+{
+
+    CompatibilitySettings() = default;
+
+    Setting<bool> nixShellAlwaysLooksForShellNix{this, true, "nix-shell-always-looks-for-shell-nix", R"(
+        Before Nix 2.24, [`nix-shell`](@docroot@/command-ref/nix-shell.md) would only look at `shell.nix` if it was in the working directory - when no file was specified.
+
+        Since Nix 2.24, `nix-shell` always looks for a `shell.nix`, whether that's in the working directory, or in a directory that was passed as an argument.
+
+        You may set this to `false` to revert to the Nix 2.3 behavior.
+    )"};
+};
+
+};

src/libcmd/meson.build

@@ -97,6 +97,7 @@ headers = [config_h] + files(
   'command-installable-value.hh',
   'command.hh',
   'common-eval-args.hh',
+  'compatibility-settings.hh',
   'editor-for.hh',
   'installable-attr-path.hh',
   'installable-derived-path.hh',

src/libexpr/eval.cc

@@ -2650,7 +2650,7 @@ void EvalState::printStatistics()
 }
 
 
-SourcePath resolveExprPath(SourcePath path)
+SourcePath resolveExprPath(SourcePath path, bool addDefaultNix)
 {
     unsigned int followCount = 0, maxFollow = 1024;
 
@@ -2666,7 +2666,7 @@ SourcePath resolveExprPath(SourcePath path)
     }
 
     /* If `path' refers to a directory, append `/default.nix'. */
-    if (path.resolveSymlinks().lstat().type == SourceAccessor::tDirectory)
+    if (addDefaultNix && path.resolveSymlinks().lstat().type == SourceAccessor::tDirectory)
         return path / "default.nix";
 
     return path;

src/libexpr/eval.hh

@@ -850,8 +850,10 @@ std::string showType(const Value & v);
 
 /**
  * If `path` refers to a directory, then append "/default.nix".
+ *
+ * @param addDefaultNix Whether to append "/default.nix" after resolving symlinks.
  */
-SourcePath resolveExprPath(SourcePath path);
+SourcePath resolveExprPath(SourcePath path, bool addDefaultNix = true);
 
 /**
  * Whether a URI is allowed, assuming restrictEval is enabled

src/nix-build/nix-build.cc

@@ -26,6 +26,7 @@
 #include "legacy.hh"
 #include "users.hh"
 #include "network-proxy.hh"
+#include "compatibility-settings.hh"
 
 using namespace nix;
 using namespace std::string_literals;
@@ -90,24 +91,50 @@ static std::vector<std::string> shellwords(const std::string & s)
     return res;
 }
 
+/**
+ * Like `resolveExprPath`, but prefers `shell.nix` instead of `default.nix`,
+ * and if `path` was a directory, it checks eagerly whether `shell.nix` or
+ * `default.nix` exist, throwing an error if they don't.
+ */
+static SourcePath resolveShellExprPath(SourcePath path)
+{
+    auto resolvedOrDir = resolveExprPath(path, false);
+    if (resolvedOrDir.resolveSymlinks().lstat().type == SourceAccessor::tDirectory) {
+        if ((resolvedOrDir / "shell.nix").pathExists()) {
+            if (compatibilitySettings.nixShellAlwaysLooksForShellNix) {
+                return resolvedOrDir / "shell.nix";
+            } else {
+                warn("Skipping '%1%', because the setting '%2%' is disabled. This is a deprecated behavior. Consider enabling '%2%'.",
+                    resolvedOrDir / "shell.nix",
+                    "nix-shell-always-looks-for-shell-nix");
+            }
+        }
+        if ((resolvedOrDir / "default.nix").pathExists()) {
+            return resolvedOrDir / "default.nix";
+        }
+        throw Error("neither '%s' nor '%s' found in '%s'", "shell.nix", "default.nix", resolvedOrDir);
+    }
+    return resolvedOrDir;
+}
+
 static void main_nix_build(int argc, char * * argv)
 {
     auto dryRun = false;
-    auto runEnv = std::regex_search(argv[0], std::regex("nix-shell$"));
+    auto isNixShell = std::regex_search(argv[0], std::regex("nix-shell$"));
     auto pure = false;
     auto fromArgs = false;
     auto packages = false;
     // Same condition as bash uses for interactive shells
     auto interactive = isatty(STDIN_FILENO) && isatty(STDERR_FILENO);
     Strings attrPaths;
-    Strings left;
+    Strings remainingArgs;
     BuildMode buildMode = bmNormal;
     bool readStdin = false;
 
     std::string envCommand; // interactive shell
     Strings envExclude;
 
-    auto myName = runEnv ? "nix-shell" : "nix-build";
+    auto myName = isNixShell ? "nix-shell" : "nix-build";
 
     auto inShebang = false;
     std::string script;
@@ -132,7 +159,7 @@ static void main_nix_build(int argc, char * * argv)
     // Heuristic to see if we're invoked as a shebang script, namely,
     // if we have at least one argument, it's the name of an
     // executable file, and it starts with "#!".
-    if (runEnv && argc > 1) {
+    if (isNixShell && argc > 1) {
         script = argv[1];
         try {
             auto lines = tokenizeString<Strings>(readFile(script), "\n");
@@ -186,9 +213,9 @@ static void main_nix_build(int argc, char * * argv)
             dryRun = true;
 
         else if (*arg == "--run-env") // obsolete
-            runEnv = true;
+            isNixShell = true;
 
-        else if (runEnv && (*arg == "--command" || *arg == "--run")) {
+        else if (isNixShell && (*arg == "--command" || *arg == "--run")) {
             if (*arg == "--run")
                 interactive = false;
             envCommand = getArg(*arg, arg, end) + "\nexit";
@@ -206,7 +233,7 @@ static void main_nix_build(int argc, char * * argv)
         else if (*arg == "--pure") pure = true;
         else if (*arg == "--impure") pure = false;
 
-        else if (runEnv && (*arg == "--packages" || *arg == "-p"))
+        else if (isNixShell && (*arg == "--packages" || *arg == "-p"))
             packages = true;
 
         else if (inShebang && *arg == "-i") {
@@ -246,7 +273,7 @@ static void main_nix_build(int argc, char * * argv)
             return false;
 
         else
-            left.push_back(*arg);
+            remainingArgs.push_back(*arg);
 
         return true;
     });
@@ -266,7 +293,7 @@ static void main_nix_build(int argc, char * * argv)
     auto autoArgs = myArgs.getAutoArgs(*state);
 
     auto autoArgsWithInNixShell = autoArgs;
-    if (runEnv) {
+    if (isNixShell) {
         auto newArgs = state->buildBindings(autoArgsWithInNixShell->size() + 1);
         newArgs.alloc("inNixShell").mkBool(true);
         for (auto & i : *autoArgs) newArgs.insert(i);
@@ -276,19 +303,26 @@ static void main_nix_build(int argc, char * * argv)
     if (packages) {
         std::ostringstream joined;
         joined << "{...}@args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) \"shell\" { buildInputs = [ ";
-        for (const auto & i : left)
+        for (const auto & i : remainingArgs)
             joined << '(' << i << ") ";
         joined << "]; } \"\"";
         fromArgs = true;
-        left = {joined.str()};
-    } else if (!fromArgs) {
-        if (left.empty() && runEnv && pathExists("shell.nix"))
-            left = {"shell.nix"};
-        if (left.empty())
-            left = {"default.nix"};
+        remainingArgs = {joined.str()};
+    } else if (!fromArgs && remainingArgs.empty()) {
+        if (isNixShell && !compatibilitySettings.nixShellAlwaysLooksForShellNix && std::filesystem::exists("shell.nix")) {
+            // If we're in 2.3 compatibility mode, we need to look for shell.nix
+            // now, because it won't be done later.
+            remainingArgs = {"shell.nix"};
+        } else {
+            remainingArgs = {"."};
+
+            // Instead of letting it throw later, we throw here to give a more relevant error message
+            if (isNixShell && !std::filesystem::exists("shell.nix") && !std::filesystem::exists("default.nix"))
+                throw Error("no argument specified and no '%s' or '%s' file found in the working directory", "shell.nix", "default.nix");
+        }
     }
 
-    if (runEnv)
+    if (isNixShell)
         setEnv("IN_NIX_SHELL", pure ? "pure" : "impure");
 
     PackageInfos drvs;
@@ -299,7 +333,7 @@ static void main_nix_build(int argc, char * * argv)
     if (readStdin)
         exprs = {state->parseStdin()};
     else
-        for (auto i : left) {
+        for (auto i : remainingArgs) {
             if (fromArgs)
                 exprs.push_back(state->parseExprFromString(std::move(i), state->rootPath(".")));
             else {
@@ -310,14 +344,18 @@ static void main_nix_build(int argc, char * * argv)
                 auto [path, outputNames] = parsePathWithOutputs(absolute);
                 if (evalStore->isStorePath(path) && hasSuffix(path, ".drv"))
                     drvs.push_back(PackageInfo(*state, evalStore, absolute));
-                else
+                else {
                     /* If we're in a #! script, interpret filenames
                        relative to the script. */
-                    exprs.push_back(
-                        state->parseExprFromFile(
-                            resolveExprPath(
-                                lookupFileArg(*state,
-                                    inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i))));
+                    auto baseDir = inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i;
+
+                    auto sourcePath = lookupFileArg(*state,
+                                    baseDir);
+                    auto resolvedPath =
+                        isNixShell ? resolveShellExprPath(sourcePath) : resolveExprPath(sourcePath);
+
+                    exprs.push_back(state->parseExprFromFile(resolvedPath));
+                }
             }
         }
 
@@ -330,7 +368,7 @@ static void main_nix_build(int argc, char * * argv)
 
         std::function<bool(const Value & v)> takesNixShellAttr;
         takesNixShellAttr = [&](const Value & v) {
-            if (!runEnv) {
+            if (!isNixShell) {
                 return false;
             }
             bool add = false;
@@ -381,7 +419,7 @@ static void main_nix_build(int argc, char * * argv)
             store->buildPaths(paths, buildMode, evalStore);
     };
 
-    if (runEnv) {
+    if (isNixShell) {
         if (drvs.size() != 1)
             throw UsageError("nix-shell requires a single derivation");
 

tests/functional/nix-shell.sh

@@ -21,6 +21,10 @@ output=$(nix-shell --pure "$shellDotNix" -A shellDrv --run \
 
 [ "$output" = " - foo - bar - true" ]
 
+output=$(nix-shell --pure "$shellDotNix" -A shellDrv --option nix-shell-always-looks-for-shell-nix false --run \
+    'echo "$IMPURE_VAR - $VAR_FROM_STDENV_SETUP - $VAR_FROM_NIX - $TEST_inNixShell"')
+[ "$output" = " - foo - bar - true" ]
+
 # Test --keep
 output=$(nix-shell --pure --keep SELECTED_IMPURE_VAR "$shellDotNix" -A shellDrv --run \
     'echo "$IMPURE_VAR - $VAR_FROM_STDENV_SETUP - $VAR_FROM_NIX - $SELECTED_IMPURE_VAR"')
@@ -91,6 +95,55 @@ sed -e "s|@ENV_PROG@|$(type -P env)|" shell.shebang.nix > $TEST_ROOT/shell.sheba
 chmod a+rx $TEST_ROOT/shell.shebang.nix
 $TEST_ROOT/shell.shebang.nix
 
+mkdir $TEST_ROOT/lookup-test $TEST_ROOT/empty
+
+echo "import $shellDotNix" > $TEST_ROOT/lookup-test/shell.nix
+cp config.nix $TEST_ROOT/lookup-test/
+echo 'abort "do not load default.nix!"' > $TEST_ROOT/lookup-test/default.nix
+
+nix-shell $TEST_ROOT/lookup-test -A shellDrv --run 'echo "it works"' | grepQuiet "it works"
+# https://github.com/NixOS/nix/issues/4529
+nix-shell -I "testRoot=$TEST_ROOT" '<testRoot/lookup-test>' -A shellDrv --run 'echo "it works"' | grepQuiet "it works"
+
+expectStderr 1 nix-shell $TEST_ROOT/lookup-test -A shellDrv --run 'echo "it works"' --option nix-shell-always-looks-for-shell-nix false \
+  | grepQuiet -F "do not load default.nix!" # we did, because we chose to enable legacy behavior
+expectStderr 1 nix-shell $TEST_ROOT/lookup-test -A shellDrv --run 'echo "it works"' --option nix-shell-always-looks-for-shell-nix false \
+  | grepQuiet "Skipping .*lookup-test/shell\.nix.*, because the setting .*nix-shell-always-looks-for-shell-nix.* is disabled. This is a deprecated behavior\. Consider enabling .*nix-shell-always-looks-for-shell-nix.*"
+
+(
+  cd $TEST_ROOT/empty;
+  expectStderr 1 nix-shell | \
+    grepQuiet "error.*no argument specified and no .*shell\.nix.* or .*default\.nix.* file found in the working directory"
+)
+
+expectStderr 1 nix-shell -I "testRoot=$TEST_ROOT" '<testRoot/empty>' |
+  grepQuiet "error.*neither .*shell\.nix.* nor .*default\.nix.* found in .*/empty"
+
+cat >$TEST_ROOT/lookup-test/shebangscript <<EOF
+#!$(type -P env) nix-shell
+#!nix-shell -A shellDrv -i bash
+[[ \$VAR_FROM_NIX == bar ]]
+echo "script works"
+EOF
+chmod +x $TEST_ROOT/lookup-test/shebangscript
+
+$TEST_ROOT/lookup-test/shebangscript | grepQuiet "script works"
+
+# https://github.com/NixOS/nix/issues/5431
+mkdir $TEST_ROOT/marco{,/polo}
+echo 'abort "marco/shell.nix must not be used, but its mere existence used to cause #5431"' > $TEST_ROOT/marco/shell.nix
+cat >$TEST_ROOT/marco/polo/default.nix <<EOF
+#!$(type -P env) nix-shell
+(import $TEST_ROOT/lookup-test/shell.nix {}).polo
+EOF
+chmod a+x $TEST_ROOT/marco/polo/default.nix
+(cd $TEST_ROOT/marco && ./polo/default.nix | grepQuiet "Polo")
+
+
+#####################
+# Flake equivalents #
+#####################
+
 # Test 'nix develop'.
 nix develop -f "$shellDotNix" shellDrv -c bash -c '[[ -n $stdenv ]]'