From 0f2b5146c79895ac10362b6da56b535fc3d963a4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 14 Feb 2023 13:25:55 -0500 Subject: [PATCH] Make restarting state machines explicit If my memory is correct, @edolstra objected to modifying `wantedOutputs` upon falling back to doing a build (as we did before), because we should only modify it in response to new requests --- *actual* wants --- and not because we are "incidentally" building all the outptus beyond what may have been requested. That's a fair point, and the alternative is to replace the boolean soup with proper enums: Instead of modifying `wantedOuputs` som more, we'll modify `needsRestart` to indicate we are passed the need. --- src/libstore/build/derivation-goal.cc | 49 +++++++++++++++++++------ src/libstore/build/derivation-goal.hh | 52 ++++++++++++++++++++++----- 2 files changed, 83 insertions(+), 18 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 606f9fd48..5bb664bff 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -145,8 +145,20 @@ void DerivationGoal::work() void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { auto newWanted = wantedOutputs.union_(outputs); - if (!newWanted.isSubsetOf(wantedOutputs)) - needRestart = true; + switch (needRestart) { + case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed: + if (!newWanted.isSubsetOf(wantedOutputs)) + needRestart = NeedRestartForMoreOutputs::OutputsAddedDoNeed; + break; + case NeedRestartForMoreOutputs::OutputsAddedDoNeed: + /* No need to check whether we added more outputs, because a + restart is already queued up. */ + break; + case NeedRestartForMoreOutputs::BuildInProgressWillNotNeed: + /* We are already building all outputs, so it doesn't matter if + we now want more. */ + break; + }; wantedOutputs = newWanted; } @@ -297,12 +309,29 @@ void DerivationGoal::outputsSubstitutionTried() 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. */ - if (nrIncompleteClosure > 0 && nrIncompleteClosure == nrFailed) retrySubstitution = true; + { + 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; - if (needRestart) { - needRestart = false; + if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) { + needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed; haveDerivation(); return; } @@ -330,9 +359,9 @@ void DerivationGoal::outputsSubstitutionTried() produced using a substitute. So we have to build instead. */ void DerivationGoal::gaveUpOnSubstitution() { - /* Make sure checkPathValidity() from now on checks all - outputs. */ - wantedOutputs = OutputsSpec::All { }; + /* At this point we are building all outputs, so if more are wanted there + is no need to restart. */ + needRestart = NeedRestartForMoreOutputs::BuildInProgressWillNotNeed; /* The inputs must be built before we can build this goal. */ inputDrvOutputs.clear(); @@ -455,8 +484,8 @@ void DerivationGoal::inputsRealised() return; } - if (retrySubstitution && !retriedSubstitution) { - retriedSubstitution = true; + if (retrySubstitution == RetrySubstitution::YesNeed) { + retrySubstitution = RetrySubstitution::AlreadyRetried; haveDerivation(); return; } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 3a6f0c2d9..9b5bd1805 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -78,22 +78,58 @@ struct DerivationGoal : public Goal */ std::map, StorePath> inputDrvOutputs; + /** + * See `needRestart`; just for that field. + */ + enum struct NeedRestartForMoreOutputs { + /** + * The goal state machine is progressing based on the current value of + * `wantedOutputs. No actions are needed. + */ + OutputsUnmodifedDontNeed, + /** + * `wantedOutputs` has been extended, but the state machine is + * proceeding according to its old value, so we need to restart. + */ + OutputsAddedDoNeed, + /** + * The goal state machine has progressed to the point of doing a build, + * in which case all outputs will be produced, so extensions to + * `wantedOutputs` no longer require a restart. + */ + BuildInProgressWillNotNeed, + }; + /** * Whether additional wanted outputs have been added. */ - bool needRestart = false; + 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. */ - bool retrySubstitution = false; - - /** - * Whether we've retried substitution, in which case we won't try - * again. - */ - bool retriedSubstitution = false; + RetrySubstitution retrySubstitution = RetrySubstitution::NoNeed; /** * The derivation stored at drvPath.