tAdd builtins.addDrvOutputDependencies

The new primop is fairly simple. What is far more weird is the test
plan.

The test plan is taken by Robert's comment in
NixOS#7910 (comment)
describing how we could migrate *Nixpkgs* without a breaking change in
Nix. The Nix testsuite has its own `mkDerivation`, and we so we do the
same thing with it:

 - `drvPath` is now overridden to not have the funky `DrvDeep` string
   context anymore.

 - Tests that previously needed to
   `builtins.unsafeDiscardOutputDependency` a `drvPath` no don't.

 - Tests that previous did *not* need to
   `builtins.unsafeDiscardOutputDependency` a `drvPath` now *do*.

Also, there is a regular language test testing the `derivation` primop
in isolation (again, no breaking change to it!) which has been extended.

Finally, we will need (TODO ACTUALLY MAKE!) unit tests catching failures
to demonstrate when `builtins.addDrvOutputDependencies` intentionally
does *not* work.

Co-authored-by: Robert Hensing <[email protected]>

src/libexpr/primops/context.cc

@@ -38,12 +38,6 @@ static RegisterPrimOp primop_hasContext({
 });
 
 
-/* Sometimes we want to pass a derivation path (i.e. pkg.drvPath) to a
-   builder without causing the derivation to be built (for instance,
-   in the derivation that builds NARs in nix-push, when doing
-   source-only deployment).  This primop marks the string context so
-   that builtins.derivation adds the path to drv.inputSrcs rather than
-   drv.inputDrvs. */
 static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     NixStringContext context;
@@ -67,10 +61,73 @@ static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx p
 static RegisterPrimOp primop_unsafeDiscardOutputDependency({
     .name = "__unsafeDiscardOutputDependency",
     .arity = 1,
+    .doc = R"(
+      TODO! Old comment is not good.
+
+      Sometimes we want to pass a derivation path (i.e. `pkg.drvPath`) to a builder without causing the derivation to be built
+      (for instance, when building a deployment script that builds already-instantiated derivations on the deployment target; a source based deployment that's independent of fetcher configuration among other things).
+      This primop marks the string context so that `builtins.derivation` adds the path to `drv.inputSrcs` rather than `drv.inputDrvs`.
+
+      Opposite of [`builtins.addDrvOutputDependencies`](#builtins-addDrvOutputDependencies).
+    )",
     .fun = prim_unsafeDiscardOutputDependency
 });
 
 
+static void prim_addDrvOutputDependencies(EvalState & state, const PosIdx pos, Value * * args, Value & v)
+{
+    NixStringContext context;
+    auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.addDrvOutputDependencies");
+
+	auto contextSize = context.size();
+    if (contextSize != 1) {
+        throw EvalError({
+            .msg = hintfmt("context of string '%s' must have exactly one element, but has %d", *s, contextSize),
+            .errPos = state.positions[pos]
+        });
+    }
+    NixStringContext context2 {
+        (NixStringContextElem { std::visit(overloaded {
+            [&](const NixStringContextElem::Opaque & c) -> NixStringContextElem::DrvDeep {
+                if (!hasSuffix(c.path.name(), drvExtension)) {
+                    throw EvalError({
+                        .msg = hintfmt("path '%s' is not a derivation",
+                            state.store->printStorePath(c.path)),
+                        .errPos = state.positions[pos],
+                    });
+                }
+                return NixStringContextElem::DrvDeep {
+                    .drvPath = c.path,
+                };
+            },
+            [&](const NixStringContextElem::Built & c) -> NixStringContextElem::DrvDeep {
+                throw EvalError({
+                    .msg = hintfmt("derivation output string context elem is not allowed"),
+                    .errPos = state.positions[pos],
+                });
+            },
+            [&](const NixStringContextElem::DrvDeep & c) -> NixStringContextElem::DrvDeep {
+                /* Reuse original item because we want this to be idempotent. */
+                return std::move(c);
+            },
+        }, *context.begin()) }),
+    };
+
+    v.mkString(*s, context2);
+}
+
+static RegisterPrimOp primop_addDrvOutputDependencies({
+    .name = "__addDrvOutputDependencies",
+    .arity = 1,
+    .doc = R"(
+      TODO!
+
+      Opposite of [`builtins.unsafeDiscardOutputDependency`](#builtins-addDrvOutputDependencies).
+    )",
+    .fun = prim_addDrvOutputDependencies
+});
+
+
 /* Extract the context of a string as a structured Nix value.
 
    The context is represented as an attribute set whose keys are the

tests/build.sh

@@ -66,7 +66,7 @@ nix build -f multiple-outputs.nix --json 'e.a_a.outPath' --no-link | jq --exit-s
 '
 
 # Illegal type of string context
-expectStderr 1 nix build -f multiple-outputs.nix 'e.a_a.drvPath' \
+expectStderr 1 nix build --impure --expr 'builtins.addDrvOutputDependencies (import ./multiple-outputs.nix).e.a_a.drvPath' \
   | grepQuiet "has a context which refers to a complete source and binary closure."
 
 # No string context
@@ -77,7 +77,7 @@ expectStderr 1 nix build --expr '""' --no-link \
 expectStderr 1 nix build --impure --expr 'with (import ./multiple-outputs.nix).e.a_a; "${drvPath}${outPath}"' --no-link \
   | grepQuiet "has 2 entries in its context. It should only have exactly one entry"
 
-nix build --impure --json --expr 'builtins.unsafeDiscardOutputDependency (import ./multiple-outputs.nix).e.a_a.drvPath' --no-link | jq --exit-status '
+nix build --json -f multiple-outputs.nix e.a_a.drvPath --no-link | jq --exit-status '
   (.[0] | .path | match(".*multiple-outputs-e.drv"))
 '
 

tests/config.nix.in

@@ -16,12 +16,29 @@ rec {
 
   shared = builtins.getEnv "_NIX_TEST_SHARED";
 
-  mkDerivation = args:
-    derivation ({
+  mkDerivation = args: let
+
+    drv = derivation ({
       inherit system;
       builder = shell;
       args = ["-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")];
       PATH = path;
-    } // caArgs // removeAttrs args ["builder" "meta"])
-    // { meta = args.meta or {}; };
+    } // caArgs // removeAttrs args ["builder" "meta"]);
+
+    fixUp = value: value // outputsMap // {
+      # Not unsafe, and we will add back if we need it.
+      # Nixpkgs should do this too, when `addDrvOutputDependencies` makes it into the NixOS stable release.
+      drvPath = (builtins.unsafeDiscardOutputDependency value.drvPath);
+    };
+
+    outputsMap = builtins.listToAttrs (map
+      (name: {
+        inherit name;
+        value = fixUp drv.${name};
+      })
+      (drv.outputs or []));
+
+  in fixUp drv // {
+    meta = args.meta or {};
+  };
 }

tests/dyn-drv/recursive-mod-json.nix

@@ -8,7 +8,7 @@ mkDerivation rec {
 
   requiredSystemFeatures = [ "recursive-nix" ];
 
-  drv = builtins.unsafeDiscardOutputDependency (import ./text-hashed-output.nix).hello.drvPath;
+  drv = (import ./text-hashed-output.nix).hello.drvPath;
 
   buildCommand = ''
     export NIX_CONFIG='experimental-features = nix-command ca-derivations'

tests/dyn-drv/text-hashed-output.nix

@@ -20,7 +20,7 @@ rec {
     name = "hello.drv";
     buildCommand = ''
       echo "Copying the derivation"
-      cp ${builtins.unsafeDiscardOutputDependency hello.drvPath} $out
+      cp ${hello.drvPath} $out
     '';
     __contentAddressed = true;
     outputHashMode = "text";

tests/export-graph.nix

@@ -23,7 +23,7 @@ rec {
   foo."bar.buildGraph" = mkDerivation {
     name = "dependencies";
     builder = builtins.toFile "build-graph-builder" "${printRefs}";
-    exportReferencesGraph = ["refs" (import ./dependencies.nix).drvPath];
+    exportReferencesGraph = ["refs" (builtins.addDrvOutputDependencies (import ./dependencies.nix).drvPath) ];
   };
 
 }

tests/flakes/build-paths.sh

@@ -49,9 +49,9 @@ cat > $flake1Dir/flake.nix <<EOF
       goodPath = path;
     };
 
-    a10 = builtins.unsafeDiscardOutputDependency self.drvCall.drvPath;
+    a10 = self.drvCall.drvPath;
 
-    a11 = self.drvCall.drvPath;
+    a11 = builtins.addDrvOutputDependencies self.drvCall.drvPath;
 
     a12 = self.drvCall.outPath;
 

tests/lang/eval-okay-context-introspection.exp

@@ -1 +1 @@
-[ true true true true true true ]
+[ true true true true true true true true true true true true ]

tests/lang/eval-okay-context-introspection.nix

@@ -31,11 +31,29 @@ let
       (builtins.unsafeDiscardStringContext str)
       (builtins.getContext str);
 
+  # Only holds true if string context contains both a `DrvDeep` and
+  # `Opaque` element.
+  almostEtaRule = str:
+    str == builtins.addDrvOutputDependencies
+      (builtins.unsafeDiscardOutputDependency str);
+
+  addDrvOutputDependencies_idemopotent = str:
+    builtins.addDrvOutputDependencies str ==
+    builtins.addDrvOutputDependencies (builtins.addDrvOutputDependencies str);
+
+  rules = str: [
+    (etaRule str)
+    (almostEtaRule str)
+    (addDrvOutputDependencies_idemopotent str)
+  ];
+
 in [
   (legit-context == desired-context)
   (reconstructed-path == combo-path)
   (etaRule "foo")
-  (etaRule drv.drvPath)
   (etaRule drv.foo.outPath)
-  (etaRule (builtins.unsafeDiscardOutputDependency drv.drvPath))
+] ++ builtins.concatMap rules [
+  drv.drvPath
+  (builtins.addDrvOutputDependencies drv.drvPath)
+  (builtins.unsafeDiscardOutputDependency drv.drvPath)
 ]

tests/multiple-outputs.nix

@@ -59,7 +59,7 @@ rec {
 
   c = mkDerivation {
     name = "multiple-outputs-c";
-    drv = b.drvPath;
+    drv = builtins.addDrvOutputDependencies b.drvPath;
     builder = builtins.toFile "builder.sh"
       ''
         mkdir $out
@@ -69,7 +69,7 @@ rec {
 
   d = mkDerivation {
     name = "multiple-outputs-d";
-    drv = builtins.unsafeDiscardOutputDependency b.drvPath;
+    drv = b.drvPath;
     builder = builtins.toFile "builder.sh"
       ''
         mkdir $out