Skip to content

Commit

Permalink
Add builtins.addDrvOutputDependencies
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Ericson2314 committed Jun 27, 2023
1 parent 71d4fd8 commit 3438db9
Show file tree
Hide file tree
Showing 10 changed files with 84 additions and 24 deletions.
47 changes: 41 additions & 6 deletions src/libexpr/primops/context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -67,10 +61,51 @@ 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, 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`.
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");

NixStringContext context2;
for (auto && c : context) {
if (auto * ptr = std::get_if<NixStringContextElem::Opaque>(&c)) {
context2.emplace(NixStringContextElem::DrvDeep {
.drvPath = ptr->path
});
} else {
/* Can reuse original item */
context2.emplace(std::move(c));
}
}

v.mkString(*s, context2);
}

static RegisterPrimOp primop_addDrvOutputDependencies({
.name = "__addDrvOutputDependencies",
.arity = 1,
.doc = R"(
TODO!
Opposite of [`builtins.addDrvOutputDependencies`](#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
Expand Down
4 changes: 2 additions & 2 deletions tests/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"))
'

Expand Down
24 changes: 20 additions & 4 deletions tests/config.nix.in
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,28 @@ 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.
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 {};
};
}
2 changes: 1 addition & 1 deletion tests/dyn-drv/recursive-mod-json.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion tests/dyn-drv/text-hashed-output.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
2 changes: 1 addition & 1 deletion tests/export-graph.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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) ];
};

}
4 changes: 2 additions & 2 deletions tests/flakes/build-paths.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion tests/lang/eval-okay-context-introspection.exp
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 ]
17 changes: 13 additions & 4 deletions tests/lang/eval-okay-context-introspection.nix
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,20 @@ 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);

bothRules = str: [ (etaRule str) (almostEtaRule 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 bothRules [
"foo"
drv.drvPath
drv.foo.outPath
(builtins.addDrvOutputDependencies drv.drvPath)
(builtins.unsafeDiscardOutputDependency drv.drvPath)
]
4 changes: 2 additions & 2 deletions tests/multiple-outputs.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 3438db9

Please sign in to comment.