Get rid of impureOutputHash

I do not believe there is any problem with computing `hashDerivationModulo` the normal way with impure derivations. Conversely, the way this used to work is very suspicious because two almost-equal derivations that only differ in depending on different impure derivations could have the same drv hash modulo. That is very suspicious because there is no reason to think those two different impure derivations will end up producing the same content-addressed data! Co-authored-by: Alain Zscheile <[email protected]>
NixOS · Jan 16, 2023 · 643e782 · 643e782
1 parent 1df3d62
commit 643e782
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 38 deletions.
diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
@@ -201,29 +201,6 @@ void DerivationGoal::haveDerivation()
     if (!drv->type().hasKnownOutputPaths())
         settings.requireExperimentalFeature(Xp::CaDerivations);
 
-    if (!drv->type().isPure()) {
-        settings.requireExperimentalFeature(Xp::ImpureDerivations);
-
-        for (auto & [outputName, output] : drv->outputs) {
-            auto randomPath = StorePath::random(outputPathName(drv->name, outputName));
-            assert(!worker.store.isValidPath(randomPath));
-            initialOutputs.insert({
-                outputName,
-                InitialOutput {
-                    .wanted = true,
-                    .outputHash = impureOutputHash,
-                    .known = InitialOutputStatus {
-                        .path = randomPath,
-                        .status = PathStatus::Absent
-                    }
-                }
-            });
-        }
-
-        gaveUpOnSubstitution();
-        return;
-    }
-
     for (auto & i : drv->outputsAndOptPaths(worker.store))
         if (i.second.second)
             worker.store.addTempRoot(*i.second.second);
@@ -238,6 +215,15 @@ void DerivationGoal::haveDerivation()
             }
         });
 
+    if (!drv->type().isPure()) {
+        settings.requireExperimentalFeature(Xp::ImpureDerivations);
+
+        /* We don't yet have any safe way to cache an impure derivation at
+           this step. */
+        gaveUpOnSubstitution();
+        return;
+    }
+
     /* Check what outputs paths are not already valid. */
     auto [allValid, validOutputs] = checkPathValidity();
 

diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
@@ -627,16 +627,6 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut
         };
     }
 
-    if (!type.isPure()) {
-        std::map<std::string, Hash> outputHashes;
-        for (const auto & [outputName, _] : drv.outputs)
-            outputHashes.insert_or_assign(outputName, impureOutputHash);
-        return DrvHash {
-            .hashes = outputHashes,
-            .kind = DrvHash::Kind::Deferred,
-        };
-    }
-
     auto kind = std::visit(overloaded {
         [](const DerivationType::InputAddressed & ia) {
             /* This might be a "pesimistically" deferred output, so we don't
@@ -649,7 +639,7 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut
                 : DrvHash::Kind::Deferred;
         },
         [](const DerivationType::Impure &) -> DrvHash::Kind {
-            assert(false);
+            return DrvHash::Kind::Deferred;
         }
     }, drv.type().raw());
 
@@ -888,6 +878,4 @@ std::optional<BasicDerivation> Derivation::tryResolve(
     return resolved;
 }
 
-const Hash impureOutputHash = hashString(htSHA256, "impure");
-
 }
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
@@ -317,6 +317,4 @@ std::string hashPlaceholder(const std::string_view outputName);
    dependency which is a CA derivation. */
 std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName);
 
-extern const Hash impureOutputHash;
-
 }