diff --git a/doc/manual/rl-next/revert-77.md b/doc/manual/rl-next/revert-77.md new file mode 100644 index 000000000..c2cef74d3 --- /dev/null +++ b/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.) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 4d4371128..393157938 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/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. */ diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index c553eeedb..80a6e060e 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/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); diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index d2feb34c7..c9294a1a4 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -151,7 +151,7 @@ Goal::Done Goal::amDone(ExitCode result, std::optional 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 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) { diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 2b4b8ac25..b745548a1 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -169,7 +169,7 @@ Goal::Co PathSubstitutionGoal::tryToRun(StorePath subPath, nix::ref 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))); } diff --git a/src/libstore/include/nix/store/build/derivation-goal.hh b/src/libstore/include/nix/store/build/derivation-goal.hh index 485a34ec4..f6f4da249 100644 --- a/src/libstore/include/nix/store/build/derivation-goal.hh +++ b/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. */ diff --git a/src/libstore/include/nix/store/build/goal.hh b/src/libstore/include/nix/store/build/goal.hh index 9be27f6b3..399b5f82f 100644 --- a/src/libstore/include/nix/store/build/goal.hh +++ b/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. */ diff --git a/tests/functional/binary-cache.sh b/tests/functional/binary-cache.sh index ff39ab3b7..2c102df07 100755 --- a/tests/functional/binary-cache.sh +++ b/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.