Get rid of .drv special-casing for store path installables

The release notes document the change in behavior, I don't include it here so there is no risk to it getting out of sync. > Motivation >> Plumbing CLI should be simple Store derivation installations are intended as "plumbing": very simple utilities for advanced users and scripts, and not what regular users interact with. (Similarly, regular Git users will use branch and tag names not explicit hashes for most things.) The plumbing CLI should prize simplicity over convenience; that is its raison d'etre. If the user provides a path, we should treat it the same way not caring what sort of path it is. >> Scripting This is especially important for the scripting use-case. when arbitrary paths are sent to e.g. `nix copy` and the script author wants consistent behavior regardless of what those store paths are. Otherwise the script author needs to be careful to filter out `.drv` ones, and then run `nix copy` again with those paths and `--derivation`. That is not good! >> Surprisingly low impact Only two lines in the tests need changing, showing that the impact of this is pretty light. Many command, like `nix log` will continue to work with just the derivation passed as before. This because we used to: - Special case the drv path and replace it with it's outputs (what this gets rid of). - Turn those output path *back* into the original drv path. Now we just skip that entire round trip! > Context Issue #7261 lays out a broader vision for getting rid of `--derivation`, and has this as one of its dependencies. But we can do this with or without that. `Installable::toDerivations` is changed to handle the case of a `DerivedPath::Opaque` ending in `.drv`, which is new: it simply doesn't need to do any extra work in that case. On this basis, commands like `nix {show-derivation,log} /nix/store/...-foo.drv` still work as before, as described above. When testing older daemons, the post-build-hook will be run against the old CLI, so we need the old version of the post-build-hook to support that use-case. Co-authored-by: Travis A. Everett <travis.a.everett@gmail.com> Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
2022-12-15 22:44:14 -05:00 · 2022-12-15 22:44:14 -05:00 · ea0adfc582
commit ea0adfc582
parent db14e1d4ae
8 changed files with 64 additions and 26 deletions
--- a/doc/manual/src/release-notes/rl-next.md
+++ b/doc/manual/src/release-notes/rl-next.md
@ -1,2 +1,18 @@
 # Release X.Y (202?-??-??)

+* The special handling of an [installable](../command-ref/new-cli/nix.md#installables) with `.drv` suffix being interpreted as all of the given [store derivation](../glossary.md#gloss-store-derivation)'s output paths is removed, and instead taken as the literal store path that it represents.
+
+  The new `^` syntax for store paths introduced in Nix 2.13 allows explicitly referencing output paths of a derivation.
+  Using this is better and more clear than relying on the now-removed `.drv` special handling.
+
+  For example,
+  ```shell-session
+  $ nix path-info /nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv
+  ```
+
+  now gives info about the derivation itself, while
+
+  ```shell-session
+  $ nix path-info /nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv^*
+  ```
+  provides information about each of its outputs.
--- a/src/libcmd/installable-derived-path.cc
+++ b/src/libcmd/installable-derived-path.cc
@ -31,27 +31,24 @@ InstallableDerivedPath InstallableDerivedPath::parse(
    ExtendedOutputsSpec extendedOutputsSpec)
 {
    auto derivedPath = std::visit(overloaded {
-        // If the user did not use ^, we treat the output more liberally.
+        // If the user did not use ^, we treat the output more
+        // liberally: we accept a symlink chain or an actual
+        // store path.
        [&](const ExtendedOutputsSpec::Default &) -> DerivedPath {
-            // First, we accept a symlink chain or an actual store path.
            auto storePath = store->followLinksToStorePath(prefix);
-            // Second, we see if the store path ends in `.drv` to decide what sort
-            // of derived path they want.
-            //
-            // This handling predates the `^` syntax. The `^*` in
-            // `/nix/store/hash-foo.drv^*` unambiguously means "do the
-            // `DerivedPath::Built` case", so plain `/nix/store/hash-foo.drv` could
-            // also unambiguously mean "do the DerivedPath::Opaque` case".
-            //
-            // Issue #7261 tracks reconsidering this `.drv` dispatching.
-            return storePath.isDerivation()
-                ? (DerivedPath) DerivedPath::Built {
-                    .drvPath = std::move(storePath),
-                    .outputs = OutputsSpec::All {},
-                }
-                : (DerivedPath) DerivedPath::Opaque {
-                    .path = std::move(storePath),
+            // Remove this prior to stabilizing the new CLI.
+            if (storePath.isDerivation()) {
+                auto oldDerivedPath = DerivedPath::Built {
+                    .drvPath = storePath,
+                    .outputs = OutputsSpec::All { },
                };
+                warn(
+                    "The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '%s'",
+                    oldDerivedPath.to_string(*store));
+            };
+            return DerivedPath::Opaque {
+                .path = std::move(storePath),
+            };
        },
        // If the user did use ^, we just do exactly what is written.
        [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> DerivedPath {
--- a/src/libcmd/installables.cc
+++ b/src/libcmd/installables.cc
@ -677,9 +677,12 @@ StorePathSet Installable::toDerivations(
        for (const auto & b : i->toDerivedPaths())
            std::visit(overloaded {
                [&](const DerivedPath::Opaque & bo) {
-                    if (!useDeriver)
-                        throw Error("argument '%s' did not evaluate to a derivation", i->what());
-                    drvPaths.insert(getDeriver(store, *i, bo.path));
+                    drvPaths.insert(
+                        bo.path.isDerivation()
+                            ? bo.path
+                        : useDeriver
+                            ? getDeriver(store, *i, bo.path)
+                        : throw Error("argument '%s' did not evaluate to a derivation", i->what()));
                },
                [&](const DerivedPath::Built & bfd) {
                    drvPaths.insert(bfd.drvPath);
--- a/tests/binary-cache.sh
+++ b/tests/binary-cache.sh
@ -23,7 +23,7 @@ nix log $outPath 2>&1 | grep 'is not available'
 nix log --substituters file://$cacheDir $outPath | grep FOO

 # Test copying build logs from the binary cache.
-nix store copy-log --from file://$cacheDir $(nix-store -qd $outPath)
+nix store copy-log --from file://$cacheDir $(nix-store -qd $outPath)^'*'
 nix log $outPath | grep FOO

 basicDownloadTests() {
--- a/tests/ca/substitute.sh
+++ b/tests/ca/substitute.sh
@ -28,6 +28,12 @@ nix realisation info --file ./content-addressed.nix transitivelyDependentCA
 nix realisation info --file ./content-addressed.nix dependentCA
 # nix realisation info --file ./content-addressed.nix rootCA --outputs out

+if isDaemonNewer "2.13"; then
+    pushToStore="../push-to-store.sh"
+else
+    pushToStore="../push-to-store-old.sh"
+fi
+
 # Same thing, but
 # 1. With non-ca derivations
 # 2. Erasing the realisations on the remote store
@ -37,7 +43,7 @@ nix realisation info --file ./content-addressed.nix dependentCA
 #
 # Regression test for #4725
 clearStore
-nix build --file ../simple.nix -L --no-link --post-build-hook ../push-to-store.sh
+nix build --file ../simple.nix -L --no-link --post-build-hook "$pushToStore"
 clearStore
 rm -r "$REMOTE_STORE_DIR/realisations"
 nix build --file ../simple.nix -L --no-link --substitute --substituters "$REMOTE_STORE" --no-require-sigs -j0
@ -52,7 +58,7 @@ if [[ -z "$(ls "$REMOTE_STORE_DIR/realisations")" ]]; then
 fi

 # Test the local realisation disk cache
-buildDrvs --post-build-hook ../push-to-store.sh
+buildDrvs --post-build-hook "$pushToStore"
 clearStore
 # Add the realisations of rootCA to the cachecache
 clearCacheCache
--- a/tests/post-hook.sh
+++ b/tests/post-hook.sh
@ -9,8 +9,14 @@ echo 'require-sigs = false' >> $NIX_CONF_DIR/nix.conf

 restartDaemon

+if isDaemonNewer "2.13"; then
+    pushToStore="$PWD/push-to-store.sh"
+else
+    pushToStore="$PWD/push-to-store-old.sh"
+fi
+
 # Build the dependencies and push them to the remote store.
-nix-build -o $TEST_ROOT/result dependencies.nix --post-build-hook $PWD/push-to-store.sh
+nix-build -o $TEST_ROOT/result dependencies.nix --post-build-hook "$pushToStore"

 clearStore

--- a/tests/push-to-store-old.sh
+++ b/tests/push-to-store-old.sh
@ -0,0 +1,10 @@
+#!/bin/sh
+
+set -x
+set -e
+
+[ -n "$OUT_PATHS" ]
+[ -n "$DRV_PATH" ]
+
+echo Pushing "$OUT_PATHS" to "$REMOTE_STORE"
+printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs
--- a/tests/push-to-store.sh
+++ b/tests/push-to-store.sh
@ -7,4 +7,4 @@ set -e
 [ -n "$DRV_PATH" ]

 echo Pushing "$OUT_PATHS" to "$REMOTE_STORE"
-printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs
+printf "%s" "$DRV_PATH"^'*' | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs