Merge pull request #13176 from obsidiansystems/revert-incomplete-closure-feature

Revert #77

doc/manual/rl-next/revert-77.md

@@ -0,0 +1,17 @@
+---
+synopsis: Revert incomplete closure mixed download and build feature
+issues: [77, 12628]
+prs: [13176]
+---
+
+Since Nix 1.3 (299141ecbd08bae17013226dbeae71e842b4fdd7 in 2013) Nix has attempted to mix together upstream fresh builds and downstream substitutions when remote substuters contain an "incomplete closure" (have some store objects, but not the store objects they reference).
+This feature is now removed.
+
+Worst case, removing this feature could cause more building downstream, but it should not cause outright failures, since this is not happening for opaque store objects that we don't know how to build if we decide not to substitute.
+In practice, however, we doubt even the more building is very likely to happen.
+Remote stores that are missing dependencies in arbitrary ways (e.g. corruption) don't seem to be very common.
+
+On the contrary, when remote stores fail to implement the [closure property](@docroot@/store/store-object.md#closure-property), it is usually an *intentional* choice on the part of the remote store, because it wishes to serve as an "overlay" store over another store, such as `https://cache.nixos.org`.
+If an "incomplete closure" is encountered in that situation, the right fix is not to do some sort of "franken-building" as this feature implemented, but instead to make sure both substituters are enabled in the settings.
+
+(In the future, we should make it easier for remote stores to indicate this to clients, to catch settings that won't work in general before a missing dependency is actually encountered.)

src/libstore/build/derivation-goal.cc

@@ -285,40 +285,13 @@ Goal::Co DerivationGoal::haveDerivation()
 
     assert(!drv->type().isImpure());
 
-    if (nrFailed > 0 && nrFailed > nrNoSubstituters + nrIncompleteClosure && !settings.tryFallback) {
+    if (nrFailed > 0 && nrFailed > nrNoSubstituters && !settings.tryFallback) {
         co_return done(BuildResult::TransientFailure, {},
             Error("some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ",
                 worker.store.printStorePath(drvPath)));
     }
 
-    /*  If the substitutes form an incomplete closure, then we should
-        build the dependencies of this derivation, but after that, we
-        can still use the substitutes for this derivation itself.
-
-        If the nrIncompleteClosure != nrFailed, we have another issue as well.
-        In particular, it may be the case that the hole in the closure is
-        an output of the current derivation, which causes a loop if retried.
-     */
-    {
-        bool substitutionFailed =
-            nrIncompleteClosure > 0 &&
-            nrIncompleteClosure == nrFailed;
-        switch (retrySubstitution) {
-        case RetrySubstitution::NoNeed:
-            if (substitutionFailed)
-                retrySubstitution = RetrySubstitution::YesNeed;
-            break;
-        case RetrySubstitution::YesNeed:
-            // Should not be able to reach this state from here.
-            assert(false);
-            break;
-        case RetrySubstitution::AlreadyRetried:
-            debug("substitution failed again, but we already retried once. Not retrying again.");
-            break;
-        }
-    }
-
-    nrFailed = nrNoSubstituters = nrIncompleteClosure = 0;
+    nrFailed = nrNoSubstituters = 0;
 
     if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) {
         needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
@@ -456,11 +429,6 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
         co_return done(BuildResult::DependencyFailed, {}, Error(msg));
     }
 
-    if (retrySubstitution == RetrySubstitution::YesNeed) {
-        retrySubstitution = RetrySubstitution::AlreadyRetried;
-        co_return haveDerivation();
-    }
-
     /* Gather information necessary for computing the closure and/or
        running the build hook. */
 

src/libstore/build/drv-output-substitution-goal.cc

@@ -139,7 +139,7 @@ Goal::Co DrvOutputSubstitutionGoal::realisationFetched(Goals waitees, std::share
 
     if (nrFailed > 0) {
         debug("The output path of the derivation output '%s' could not be substituted", id.to_string());
-        co_return amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed);
+        co_return amDone(nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed);
     }
 
     worker.store.registerDrvOutput(*outputInfo);

src/libstore/build/goal.cc

@@ -151,7 +151,7 @@ Goal::Done Goal::amDone(ExitCode result, std::optional<Error> ex)
     trace("done");
     assert(top_co);
     assert(exitCode == ecBusy);
-    assert(result == ecSuccess || result == ecFailed || result == ecNoSubstituters || result == ecIncompleteClosure);
+    assert(result == ecSuccess || result == ecFailed || result == ecNoSubstituters);
     exitCode = result;
 
     if (ex) {
@@ -170,12 +170,10 @@ Goal::Done Goal::amDone(ExitCode result, std::optional<Error> ex)
 
             goal->trace(fmt("waitee '%s' done; %d left", name, goal->waitees.size()));
 
-            if (result == ecFailed || result == ecNoSubstituters || result == ecIncompleteClosure) ++goal->nrFailed;
+            if (result == ecFailed || result == ecNoSubstituters) ++goal->nrFailed;
 
             if (result == ecNoSubstituters) ++goal->nrNoSubstituters;
 
-            if (result == ecIncompleteClosure) ++goal->nrIncompleteClosure;
-
             if (goal->waitees.empty()) {
                 worker.wakeUp(goal);
             } else if (result == ecFailed && !settings.keepGoing) {

src/libstore/build/substitution-goal.cc

@@ -169,7 +169,7 @@ Goal::Co PathSubstitutionGoal::tryToRun(StorePath subPath, nix::ref<Store> sub,
 
     if (nrFailed > 0) {
         co_return done(
-            nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed,
+            nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed,
             BuildResult::DependencyFailed,
             fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath)));
     }

src/libstore/include/nix/store/build/derivation-goal.hh

@@ -73,32 +73,6 @@ struct DerivationGoal : public Goal
      */
     NeedRestartForMoreOutputs needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
 
-    /**
-     * See `retrySubstitution`; just for that field.
-     */
-    enum RetrySubstitution {
-        /**
-         * No issues have yet arose, no need to restart.
-         */
-        NoNeed,
-        /**
-         * Something failed and there is an incomplete closure. Let's retry
-         * substituting.
-         */
-        YesNeed,
-        /**
-         * We are current or have already retried substitution, and whether or
-         * not something goes wrong we will not retry again.
-         */
-        AlreadyRetried,
-    };
-
-    /**
-     * Whether to retry substituting the outputs after building the
-     * inputs. This is done in case of an incomplete closure.
-     */
-    RetrySubstitution retrySubstitution = RetrySubstitution::NoNeed;
-
     /**
      * The derivation stored at drvPath.
      */

src/libstore/include/nix/store/build/goal.hh

@@ -61,7 +61,7 @@ private:
     Goals waitees;
 
 public:
-    typedef enum {ecBusy, ecSuccess, ecFailed, ecNoSubstituters, ecIncompleteClosure} ExitCode;
+    typedef enum {ecBusy, ecSuccess, ecFailed, ecNoSubstituters} ExitCode;
 
     /**
      * Backlink to the worker.
@@ -85,12 +85,6 @@ public:
      */
     size_t nrNoSubstituters = 0;
 
-    /**
-     * Number of substitution goals we are/were waiting for that
-     * failed because they had unsubstitutable references.
-     */
-    size_t nrIncompleteClosure = 0;
-
     /**
      * Name of this goal for debugging purposes.
      */

tests/functional/binary-cache.sh

@@ -151,8 +151,11 @@ nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -
 grepQuiet "don't know how to build" "$TEST_ROOT/log"
 grepQuiet "building.*input-1" "$TEST_ROOT/log"
 grepQuiet "building.*input-2" "$TEST_ROOT/log"
-grepQuiet "copying path.*input-0" "$TEST_ROOT/log"
-grepQuiet "copying path.*top" "$TEST_ROOT/log"
+
+# Removed for now since 299141ecbd08bae17013226dbeae71e842b4fdd7 / issue #77 is reverted
+
+#grepQuiet "copying path.*input-0" "$TEST_ROOT/log"
+#grepQuiet "copying path.*top" "$TEST_ROOT/log"
 
 
 # Create a signed binary cache.