Add builtins.addDrvOutputDependencies

End goal: make `(mkDerivation x).drvPath` behave like a non-DrvDeep context. Problem: users won't be able to recover the DrvDeep behavior when nixpkgs makes this change. Solution: add this primop. The new primop is fairly simple, and is supposed to complement other existing ones (`builtins.storePath`, `builtins.outputOf`) so there are simple ways to construct strings with every type of string context element. (It allows nothing we couldn't already do with `builtins.getContext` and `builtins.appendContext`, which is also true of those other two primops.) This was originally in #8595, but then it was proposed to land some doc changes separately. So now the code changes proper is just moved to this, and the doc will be done in that. Co-authored-by: Robert Hensing <[email protected]> Co-authored-by: Théophane Hufschmitt <[email protected] github.com> Co-authored-by: Valentin Gagarin <[email protected]
NixOS · Oct 23, 2023 · 845c771 · 845c771
1 parent 8b68bbb
commit 845c771
Show file tree

Hide file tree

Showing 9 changed files with 177 additions and 12 deletions.
diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc
@@ -30,20 +30,27 @@ static RegisterPrimOp primop_hasContext({
     .name = "__hasContext",
     .args = {"s"},
     .doc = R"(
-      Return `true` if string *s* has a non-empty context. The
-      context can be obtained with
+      Return `true` if string *s* has a non-empty context.
+      The context can be obtained with
       [`getContext`](#builtins-getContext).
+
+      > **Example**
+      >
+      > Many operations require a string context to be empty because they are intended only to work with "regular" strings, and also to help users avoid unintentionally loosing track of string context elements.
+      > `builtins.hasContext` can help create better domain-specific errors in those case.
+      >
+      > ```nix
+      > name: meta:
+      >
+      > if builtins.hasContext name
+      > then throw "package name cannot contain string context"
+      > else { ${name} = meta; }
+      > ```
     )",
     .fun = prim_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;
@@ -66,11 +73,91 @@ static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx p
 
 static RegisterPrimOp primop_unsafeDiscardOutputDependency({
     .name = "__unsafeDiscardOutputDependency",
-    .arity = 1,
+    .args = {"s"},
+    .doc = R"(
+      Create a copy of the given string where every
+      [derivation deep](@docroot@/language/string-context.md#string-context-element-derivation-deep)
+      string context element is turned into a
+      [constant](@docroot@/language/string-context.md#string-context-element-constant)
+      string context element.
+
+      This is the opposite of [`builtins.addDrvOutputDependencies`](#builtins-addDrvOutputDependencies).
+
+      This is unsafe because it allows us to "forget" store objects we would have otherwise refered to with the string context,
+      whereas Nix normally tracks all dependencies consistently.
+      Safe operations "grow" but never "shrink" string contexts.
+      [`builtins.addDrvOutputDependencies`] in contrast is safe because "derivation deep" string context element always refers to the underlying derivation (among many more things).
+      Replacing a constant string context element with a "derivation deep" element is a safe operation that just enlargens the string context without forgetting anything.
+
+      [`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 (!c.path.isDerivation()) {
+                    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("`addDrvOutputDependencies` can only act on derivations, not on a derivation output such as '%1%'", c.output),
+                    .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()->raw) }),
+    };
+
+    v.mkString(*s, context2);
+}
+
+static RegisterPrimOp primop_addDrvOutputDependencies({
+    .name = "__addDrvOutputDependencies",
+    .args = {"s"},
+    .doc = R"(
+      Create a copy of the given string where a single
+      [constant](@docroot@/language/string-context.md#string-context-element-constant)
+      string context element is turned into a
+      [derivation deep](@docroot@/language/string-context.md#string-context-element-derivation-deep)
+      string context element.
+
+      The store path that is the constant string context element should point to a valid derivation, and end in `.drv`.
+
+      The original string context element must not be empty or have multiple elements, and it must not have any other type of element other than a constant or derivation deep element.
+      The latter is supported so this function is idempotent.
+
+      This is the 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

diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-empty-context.err.exp b/tests/functional/lang/eval-fail-addDrvOutputDependencies-empty-context.err.exp
@@ -0,0 +1,10 @@
+error:
+       … while calling the 'addDrvOutputDependencies' builtin
+
+         at /pwd/lang/eval-fail-addDrvOutputDependencies-empty-context.nix:1:1:
+
+            1| builtins.addDrvOutputDependencies ""
+             | ^
+            2|
+
+       error: context of string '' must have exactly one element, but has 0
diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-empty-context.nix b/tests/functional/lang/eval-fail-addDrvOutputDependencies-empty-context.nix
@@ -0,0 +1 @@
+builtins.addDrvOutputDependencies ""
diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-multi-elem-context.err.exp b/tests/functional/lang/eval-fail-addDrvOutputDependencies-multi-elem-context.err.exp
@@ -0,0 +1,11 @@
+error:
+       … while calling the 'addDrvOutputDependencies' builtin
+
+         at /pwd/lang/eval-fail-addDrvOutputDependencies-multi-elem-context.nix:18:4:
+
+           17|
+           18| in builtins.addDrvOutputDependencies combo-path
+             |    ^
+           19|
+
+       error: context of string '/nix/store/pg9yqs4yd85yhdm3f4i5dyaqp5jahrsz-fail.drv/nix/store/2dxd5frb715z451vbf7s8birlf3argbk-fail-2.drv' must have exactly one element, but has 2
diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-multi-elem-context.nix b/tests/functional/lang/eval-fail-addDrvOutputDependencies-multi-elem-context.nix
@@ -0,0 +1,18 @@
+let
+  drv0 = derivation {
+    name = "fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+
+  drv1 = derivation {
+    name = "fail-2";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+
+  combo-path = "${drv0.drvPath}${drv1.drvPath}";
+
+in builtins.addDrvOutputDependencies combo-path
diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-wrong-element-kind.err.exp b/tests/functional/lang/eval-fail-addDrvOutputDependencies-wrong-element-kind.err.exp
@@ -0,0 +1,11 @@
+error:
+       … while calling the 'addDrvOutputDependencies' builtin
+
+         at /pwd/lang/eval-fail-addDrvOutputDependencies-wrong-element-kind.nix:9:4:
+
+            8|
+            9| in builtins.addDrvOutputDependencies drv.outPath
+             |    ^
+           10|
+
+       error: `addDrvOutputDependencies` can only act on derivations, not on a derivation output such as 'out'
diff --git a/tests/functional/lang/eval-fail-addDrvOutputDependencies-wrong-element-kind.nix b/tests/functional/lang/eval-fail-addDrvOutputDependencies-wrong-element-kind.nix
@@ -0,0 +1,9 @@
+let
+  drv = derivation {
+    name = "fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+
+in builtins.addDrvOutputDependencies drv.outPath
diff --git a/tests/functional/lang/eval-okay-context-introspection.exp b/tests/functional/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 true ]
diff --git a/tests/functional/lang/eval-okay-context-introspection.nix b/tests/functional/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_idempotent = str:
+    builtins.addDrvOutputDependencies str ==
+    builtins.addDrvOutputDependencies (builtins.addDrvOutputDependencies str);
+
+  rules = str: [
+    (etaRule str)
+    (almostEtaRule str)
+    (addDrvOutputDependencies_idempotent 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)
 ]
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		[ true true true true true true ]
		[ true true true true true true true true true true true true true ]