Merge pull request #4056 from tweag/non-ca-depending-on-ca

Allow non-CA derivations to depend on CA ones

src/libexpr/primops.cc

@@ -1089,18 +1089,35 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
 
         // Regular, non-CA derivation should always return a single hash and not
         // hash per output.
-        Hash h = std::get<0>(hashDerivationModulo(*state.store, Derivation(drv), true));
+        auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true);
+        std::visit(overloaded {
+            [&](Hash h) {
+                for (auto & i : outputs) {
+                    auto outPath = state.store->makeOutputPath(i, h, drvName);
+                    drv.env[i] = state.store->printStorePath(outPath);
+                    drv.outputs.insert_or_assign(i,
+                        DerivationOutput {
+                            .output = DerivationOutputInputAddressed {
+                                .path = std::move(outPath),
+                            },
+                        });
+                }
+            },
+            [&](CaOutputHashes) {
+                // Shouldn't happen as the toplevel derivation is not CA.
+                assert(false);
+            },
+            [&](UnknownHashes) {
+                for (auto & i : outputs) {
+                    drv.outputs.insert_or_assign(i,
+                        DerivationOutput {
+                            .output = DerivationOutputDeferred{},
+                        });
+                }
+            },
+        },
+        hashModulo);
 
-        for (auto & i : outputs) {
-            auto outPath = state.store->makeOutputPath(i, h, drvName);
-            drv.env[i] = state.store->printStorePath(outPath);
-            drv.outputs.insert_or_assign(i,
-                DerivationOutput {
-                    .output = DerivationOutputInputAddressed {
-                        .path = std::move(outPath),
-                    },
-                });
-        }
     }
 
     /* Write the resulting term into the Nix store directory. */

src/libstore/build/derivation-goal.cc

@@ -493,7 +493,8 @@ void DerivationGoal::inputsRealised()
     if (useDerivation) {
         auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());
 
-        if (!fullDrv.inputDrvs.empty() && fullDrv.type() == DerivationType::CAFloating) {
+        if ((!fullDrv.inputDrvs.empty() &&
+             fullDrv.type() == DerivationType::CAFloating) || fullDrv.type() == DerivationType::DeferredInputAddressed) {
             /* We are be able to resolve this derivation based on the
                now-known results of dependencies. If so, we become a stub goal
                aliasing that resolved derivation goal */
@@ -3166,6 +3167,15 @@ void DerivationGoal::registerOutputs()
             [&](DerivationOutputCAFloating dof) {
                 return newInfoFromCA(dof);
             },
+                [&](DerivationOutputDeferred) {
+                // No derivation should reach that point without having been
+                // rewritten first
+                assert(false);
+                // Ugly, but the compiler insists on having this return a value
+                // of type `ValidPathInfo` despite the `assert(false)`, so
+                // let's provide it
+                return *(ValidPathInfo*)0;
+            },
         }, output.output);
 
         /* Calculate where we'll move the output files. In the checking case we

src/libstore/derivations.cc

@@ -21,6 +21,9 @@ std::optional<StorePath> DerivationOutput::path(const Store & store, std::string
         [](DerivationOutputCAFloating dof) -> std::optional<StorePath> {
             return std::nullopt;
         },
+        [](DerivationOutputDeferred) -> std::optional<StorePath> {
+            return std::nullopt;
+        },
     }, output);
 }
 
@@ -37,6 +40,7 @@ bool derivationIsCA(DerivationType dt) {
     case DerivationType::InputAddressed: return false;
     case DerivationType::CAFixed: return true;
     case DerivationType::CAFloating: return true;
+    case DerivationType::DeferredInputAddressed: return false;
     };
     // Since enums can have non-variant values, but making a `default:` would
     // disable exhaustiveness warnings.
@@ -48,6 +52,7 @@ bool derivationIsFixed(DerivationType dt) {
     case DerivationType::InputAddressed: return false;
     case DerivationType::CAFixed: return true;
     case DerivationType::CAFloating: return false;
+    case DerivationType::DeferredInputAddressed: return false;
     };
     assert(false);
 }
@@ -57,6 +62,7 @@ bool derivationIsImpure(DerivationType dt) {
     case DerivationType::InputAddressed: return false;
     case DerivationType::CAFixed: return true;
     case DerivationType::CAFloating: return false;
+    case DerivationType::DeferredInputAddressed: return false;
     };
     assert(false);
 }
@@ -180,6 +186,11 @@ static DerivationOutput parseDerivationOutput(const Store & store,
             };
         }
     } else {
+        if (pathS == "") {
+            return DerivationOutput {
+                .output = DerivationOutputDeferred { }
+            };
+        }
         validatePath(pathS);
         return DerivationOutput {
             .output = DerivationOutputInputAddressed {
@@ -325,6 +336,11 @@ string Derivation::unparse(const Store & store, bool maskOutputs,
                 s += ','; printUnquotedString(s, makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType));
                 s += ','; printUnquotedString(s, "");
             },
+            [&](DerivationOutputDeferred) {
+                s += ','; printUnquotedString(s, "");
+                s += ','; printUnquotedString(s, "");
+                s += ','; printUnquotedString(s, "");
+            }
         }, i.second.output);
         s += ')';
     }
@@ -389,7 +405,7 @@ std::string outputPathName(std::string_view drvName, std::string_view outputName
 
 DerivationType BasicDerivation::type() const
 {
-    std::set<std::string_view> inputAddressedOutputs, fixedCAOutputs, floatingCAOutputs;
+    std::set<std::string_view> inputAddressedOutputs, fixedCAOutputs, floatingCAOutputs, deferredIAOutputs;
     std::optional<HashType> floatingHashType;
     for (auto & i : outputs) {
         std::visit(overloaded {
@@ -408,22 +424,27 @@ DerivationType BasicDerivation::type() const
                         throw Error("All floating outputs must use the same hash type");
                 }
             },
+            [&](DerivationOutputDeferred _) {
+               deferredIAOutputs.insert(i.first);
+            },
         }, i.second.output);
     }
 
-    if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty()) {
+    if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) {
         throw Error("Must have at least one output");
-    } else if (! inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty()) {
+    } else if (! inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) {
         return DerivationType::InputAddressed;
-    } else if (inputAddressedOutputs.empty() && ! fixedCAOutputs.empty() && floatingCAOutputs.empty()) {
+    } else if (inputAddressedOutputs.empty() && ! fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) {
         if (fixedCAOutputs.size() > 1)
             // FIXME: Experimental feature?
             throw Error("Only one fixed output is allowed for now");
         if (*fixedCAOutputs.begin() != "out")
             throw Error("Single fixed output must be named \"out\"");
         return DerivationType::CAFixed;
-    } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && ! floatingCAOutputs.empty()) {
+    } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && ! floatingCAOutputs.empty() && deferredIAOutputs.empty()) {
         return DerivationType::CAFloating;
+    } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && !deferredIAOutputs.empty()) {
+        return DerivationType::DeferredInputAddressed;
     } else {
         throw Error("Can't mix derivation output types");
     }
@@ -476,7 +497,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
     /* Return a fixed hash for fixed-output derivations. */
     switch (drv.type()) {
     case DerivationType::CAFloating:
-        throw Error("Regular input-addressed derivations are not yet allowed to depend on CA derivations");
+        return UnknownHashes {};
     case DerivationType::CAFixed: {
         std::map<std::string, Hash> outputHashes;
         for (const auto & i : drv.outputs) {
@@ -491,12 +512,15 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
     }
     case DerivationType::InputAddressed:
         break;
+    case DerivationType::DeferredInputAddressed:
+        break;
     }
 
     /* For other derivations, replace the inputs paths with recursive
        calls to this function. */
     std::map<std::string, StringSet> inputs2;
     for (auto & i : drv.inputDrvs) {
+        bool hasUnknownHash = false;
         const auto & res = pathDerivationModulo(store, i.first);
         std::visit(overloaded {
             // Regular non-CA derivation, replace derivation
@@ -514,7 +538,13 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
                         justOut);
                 }
             },
+            [&](UnknownHashes) {
+                hasUnknownHash = true;
+            },
         }, res);
+        if (hasUnknownHash) {
+            return UnknownHashes {};
+        }
     }
 
     return hashString(htSHA256, drv.unparse(store, maskOutputs, &inputs2));
@@ -620,6 +650,11 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr
                     << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType))
                     << "";
             },
+            [&](DerivationOutputDeferred) {
+                out << ""
+                    << ""
+                    << "";
+            },
         }, i.second.output);
     }
     worker_proto::write(store, out, drv.inputSrcs);
@@ -645,7 +680,6 @@ std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath
 }
 
 
-// N.B. Outputs are left unchanged
 static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites) {
 
     debug("Rewriting the derivation");
@@ -666,6 +700,21 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String
         newEnv.emplace(envName, envValue);
     }
     drv.env = newEnv;
+
+    auto hashModulo = hashDerivationModulo(store, Derivation(drv), true);
+    for (auto & [outputName, output] : drv.outputs) {
+        if (std::holds_alternative<DerivationOutputDeferred>(output.output)) {
+            Hash h = std::get<Hash>(hashModulo);
+            auto outPath = store.makeOutputPath(outputName, h, drv.name);
+            drv.env[outputName] = store.printStorePath(outPath);
+            output = DerivationOutput {
+                .output = DerivationOutputInputAddressed {
+                    .path = std::move(outPath),
+                },
+            };
+        }
+    }
+
 }
 
 

src/libstore/derivations.hh

@@ -41,12 +41,18 @@ struct DerivationOutputCAFloating
     HashType hashType;
 };
 
+/* Input-addressed output which depends on a (CA) derivation whose hash isn't
+ * known atm
+ */
+struct DerivationOutputDeferred {};
+
 struct DerivationOutput
 {
     std::variant<
         DerivationOutputInputAddressed,
         DerivationOutputCAFixed,
-        DerivationOutputCAFloating
+        DerivationOutputCAFloating,
+        DerivationOutputDeferred
     > output;
     std::optional<HashType> hashAlgoOpt(const Store & store) const;
     /* Note, when you use this function you should make sure that you're passing
@@ -72,6 +78,7 @@ typedef std::map<string, string> StringPairs;
 
 enum struct DerivationType : uint8_t {
     InputAddressed,
+    DeferredInputAddressed,
     CAFixed,
     CAFloating,
 };
@@ -167,9 +174,12 @@ std::string outputPathName(std::string_view drvName, std::string_view outputName
 // whose output hashes are always known since they are fixed up-front.
 typedef std::map<std::string, Hash> CaOutputHashes;
 
+struct UnknownHashes {};
+
 typedef std::variant<
     Hash, // regular DRV normalized hash
-    CaOutputHashes
+    CaOutputHashes, // Fixed-output derivation hashes
+    UnknownHashes // Deferred hashes for floating outputs drvs and their dependencies
 > DrvHashModulo;
 
 /* Returns hashes with the details of fixed-output subderivations

src/libstore/local-store.cc

@@ -573,6 +573,8 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat
             [&](DerivationOutputCAFloating _) {
                 /* Nothing to check */
             },
+            [&](DerivationOutputDeferred) {
+            },
         }, i.second.output);
     }
 }
@@ -817,7 +819,7 @@ std::map<std::string, std::optional<StorePath>> LocalStore::queryPartialDerivati
     }
     /* can't just use else-if instead of `!haveCached` because we need to unlock
        `drvPathResolutions` before it is locked in `Derivation::resolve`. */
-    if (!haveCached && drv.type() == DerivationType::CAFloating) {
+    if (!haveCached && (drv.type() == DerivationType::CAFloating || drv.type() == DerivationType::DeferredInputAddressed)) {
         /* Try resolve drv and use that path instead. */
         auto attempt = drv.tryResolve(*this);
         if (!attempt)

src/nix/show-derivation.cc

@@ -82,6 +82,7 @@ struct CmdShowDerivation : InstallablesCommand
                         [&](DerivationOutputCAFloating dof) {
                             outputObj.attr("hashAlgo", makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType));
                         },
+                        [&](DerivationOutputDeferred) {},
                     }, output.output);
                 }
             }

tests/content-addressed.nix

@@ -15,7 +15,7 @@ rec {
     '';
   };
   rootCA = mkDerivation {
-    name = "dependent";
+    name = "rootCA";
     outputs = [ "out" "dev" ];
     buildCommand = ''
       echo "building a CA derivation"
@@ -51,4 +51,13 @@ rec {
     outputHashMode = "recursive";
     outputHashAlgo = "sha256";
   };
+  dependentNonCA = mkDerivation {
+    name = "dependent-non-ca";
+    buildCommand = ''
+      echo "Didn't cut-off"
+      echo "building dependent-non-ca"
+      mkdir -p $out
+      echo ${rootCA}/non-ca-hello > $out/dep
+    '';
+  };
 }

tests/content-addressed.sh

@@ -5,23 +5,49 @@ source common.sh
 drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 1)
 nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1
 
-testDerivation () {
+buildAttr () {
     local derivationPath=$1
-    local commonArgs=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "$derivationPath" "--no-out-link")
+    shift
+    local args=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "$derivationPath" "--no-out-link")
+    args+=("$@")
+    nix-build "${args[@]}"
+}
+
+testRemoteCache () {
+    clearCache
+    local outPath=$(buildAttr dependentNonCA)
+    nix copy --to file://$cacheDir $outPath
+    clearStore
+    buildAttr dependentNonCA --option substituters file://$cacheDir --no-require-sigs |& (! grep "building dependent-non-ca")
+}
+
+testDeterministicCA () {
+    [[ $(buildAttr rootCA) = $(buildAttr rootCA) ]]
+}
+
+testCutoffFor () {
     local out1 out2
-    out1=$(nix-build "${commonArgs[@]}" --arg seed 1)
+    out1=$(buildAttr $1)
-    out2=$(nix-build "${commonArgs[@]}" --arg seed 2 "${secondSeedArgs[@]}")
+    # The seed only changes the root derivation, and not it's output, so the
+    # dependent derivations should only need to be built once.
+    out2=$(buildAttr $1 -j0)
     test "$out1" == "$out2"
 }
 
-testDerivation rootCA
+testCutoff () {
-# The seed only changes the root derivation, and not it's output, so the
+    # Don't directly build depenentCA, that way we'll make sure we dodn't rely on
-# dependent derivations should only need to be built once.
+    # dependent derivations always being already built.
-secondSeedArgs=(-j0)
+    #testDerivation dependentCA
-# Don't directly build depenentCA, that way we'll make sure we dodn't rely on
+    testCutoffFor transitivelyDependentCA
-# dependent derivations always being already built.
+    testCutoffFor dependentNonCA
-#testDerivation dependentCA
+}
-testDerivation transitivelyDependentCA
 
-nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 5
+testGC () {
-nix-collect-garbage --experimental-features ca-derivations --option keep-derivations true
+    nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 5
+    nix-collect-garbage --experimental-features ca-derivations --option keep-derivations true
+}
+
+testRemoteCache
+testDeterministicCA
+testCutoff
+testGC