diff --git a/src/libstore/build/substituter-set.cc b/src/libstore/build/substituter-set.cc new file mode 100644 index 000000000..c3fc02741 --- /dev/null +++ b/src/libstore/build/substituter-set.cc @@ -0,0 +1,135 @@ +#include "substituter-set.hh" +#include "store-api.hh" + +namespace nix { + + SubstituterSet::SubstituterSet(std::list> substituters, std::optional contentAddress, + std::optional storePath, Store *baseStore) : contentAddress( + contentAddress), storePath(storePath), + baseStore(baseStore) { + for (const auto &x: substituters) { + subResults.push_back({.store = x}); + } + } + + size_t SubstituterSet::remaining() const { + return subResults.size(); + } + + // FIXME: Does this behave correctly with a missing result? + // I think we'll need these things to return nothing on an empty list + ref SubstituterSet::getSubstituter() { + return subResults[currentSubstituter].store; + } + + std::shared_ptr + SubstituterSet::getInfo() const { + return subResults[currentSubstituter].result->pathInfo; + } + + std::optional + SubstituterSet::getSubPath() const { + return subResults[currentSubstituter].result->subPath; + } + + // FIXME: maybe return Ok() or Errors here? Then we use getSubstituter to access, and can log the errors better + std::optional> SubstituterSet::nextSubstituter() { + if (subResults.empty()) { + return {}; + } + // What if results.size == 0? + if (currentSubstituter >= subResults.size()) { + // What if we continue calling this? + currentSubstituter = 0; + } else { + currentSubstituter++; + if (currentSubstituter == subResults.size()) { + // none left + return {}; + } + } + if (!subResults[currentSubstituter].result) { + // Wait for the future if necessary + // FIXME: choose first available substituter with matching priority + if (subResults[currentSubstituter].future.valid()) { + subResults[currentSubstituter].result = subResults[currentSubstituter].future.get(); + } else { + // Future was never initialized + // FIXME: maybe call init()? risk is some bug causing infinite looping + assert(false); + } + } + if (!subResults[currentSubstituter].result->valid) { + return nextSubstituter(); + } + return getSubstituter(); + } + + // Checks whether a substituter may to have the store path. + // This function queries a single substituter and returns the result + // FIXME: intended to be thread-safe, need to test + // This is a non-member function because it's called async + SubstituterSet::SubstituterResult substituterValidate(ref substituter, std::optional contentAddress, Store* baseStore, std::optional storePath) { + // Explicitly mark the result as false until we finish all the checks + auto result = SubstituterSet::SubstituterResult{.valid = false}; + + if (contentAddress) { + result.subPath = substituter->makeFixedOutputPathFromCA(storePath->name(), *contentAddress); + if (substituter->storeDir == baseStore->storeDir) + assert(result.subPath == storePath); + } else if (substituter->storeDir != baseStore->storeDir) { + result.valid = false; + return result; + } + + try { + result.pathInfo = substituter->queryPathInfo(result.subPath ? *result.subPath : *storePath); + } catch (const InvalidPath &e) { + result.error = e; + return result; + } catch (const SubstituterDisabled &e) { + result.error = e; + return result; + } catch (Error &e) { + // FIXME: Handle this error message better? + // Or combine with the others? + logError(e.info()); + result.error = e; + return result; + } + + if (result.pathInfo->path != storePath) { + if (result.pathInfo->isContentAddressed(*substituter) && result.pathInfo->references.empty()) { + auto info2 = std::make_shared(*result.pathInfo); + info2->path = *storePath; + result.pathInfo = info2; + } else { + printError("asked '%s' for '%s' but got '%s'", + substituter->getUri(), baseStore->printStorePath(*storePath), + substituter->printStorePath(result.pathInfo->path)); + return result; + } + } + + /* Bail out early if this substituter lacks a valid + signature. LocalStore::addToStore() also checks for this, but + only after we've downloaded the path. */ + if (!substituter->isTrusted && baseStore->pathInfoIsUntrusted(*result.pathInfo)) { + warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'", + baseStore->printStorePath(*storePath), substituter->getUri()); + return result; + } + + result.valid = true; + return result; + } + + void SubstituterSet::init() { + for (auto &sub: subResults) { + if (!sub.result && !sub.future.valid()) { + sub.future = std::async(std::launch::async, substituterValidate, sub.store, contentAddress, baseStore, storePath); + } + } + } + +} diff --git a/src/libstore/build/substituter-set.hh b/src/libstore/build/substituter-set.hh new file mode 100644 index 000000000..a7d6a4a15 --- /dev/null +++ b/src/libstore/build/substituter-set.hh @@ -0,0 +1,64 @@ +#pragma once + +#include "store-api.hh" +#include +#include + +namespace nix { + + struct SubstituterSet { + struct SubstituterResult { + // FIXME: there's probably a better way to set this and the error messages + bool valid = false; + std::optional error; + // FIXME: is this the correct name? + std::shared_ptr pathInfo; + std::optional subPath; + }; + + struct SubstituterInfo { + ref store; + std::optional result; + std::future future; + }; + + /* Content address for recomputing store path */ + std::optional contentAddress; + + /* The store path that should be realised through a substitute. */ + std::optional storePath; + + // The store associated with the current job? + // FIXME: no bare pointers + Store* baseStore = nullptr; + + // Saved list of substituters and the query results + std::vector subResults; + + // Reference to the substituter in use. + // FIXME: this really shouldn't be an index + size_t currentSubstituter = std::numeric_limits::max(); + + public: + SubstituterSet() = default; + + SubstituterSet(std::list> substituters, std::optional ca, + std::optional storePath, Store* baseStore); + + // Perform initialization steps to query the substituters. + void init(); + + size_t remaining() const; + + // Returns the current substituter. + // FIXME: safety + ref getSubstituter(); + + std::shared_ptr getInfo() const; + std::optional getSubPath() const; + + // Invalidates the current substituter and returns the next valid one. + // To be used when the current substituter fails. + std::optional> nextSubstituter(); + }; +} diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 2af105b4d..e5e658a83 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -59,7 +59,10 @@ void PathSubstitutionGoal::init() if (settings.readOnlyMode) throw Error("cannot substitute path '%s' - no write access to the Nix store", worker.store.printStorePath(storePath)); - subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); + substituters = SubstituterSet(settings.useSubstitutes ? getDefaultSubstituters() : std::list>() + , ca, storePath, &worker.store); + + substituters.init(); tryNext(); } @@ -71,7 +74,9 @@ void PathSubstitutionGoal::tryNext() cleanup(); - if (subs.size() == 0) { + if (auto next = substituters.nextSubstituter(); next.has_value()) { + sub = next.value(); + } else { /* None left. Terminate this goal and let someone else deal with it. */ @@ -91,56 +96,10 @@ void PathSubstitutionGoal::tryNext() return; } - sub = subs.front(); - subs.pop_front(); - - if (ca) { - subPath = sub->makeFixedOutputPathFromCA(storePath.name(), *ca); - if (sub->storeDir == worker.store.storeDir) - assert(subPath == storePath); - } else if (sub->storeDir != worker.store.storeDir) { - tryNext(); - return; - } - - try { - // FIXME: make async - info = sub->queryPathInfo(subPath ? *subPath : storePath); - } catch (InvalidPath &) { - tryNext(); - return; - } catch (SubstituterDisabled &) { - if (settings.tryFallback) { - tryNext(); - return; - } - throw; - } catch (Error & e) { - if (settings.tryFallback) { - logError(e.info()); - tryNext(); - return; - } - throw; - } - - if (info->path != storePath) { - if (info->isContentAddressed(*sub) && info->references.empty()) { - auto info2 = std::make_shared(*info); - info2->path = storePath; - info = info2; - } else { - printError("asked '%s' for '%s' but got '%s'", - sub->getUri(), worker.store.printStorePath(storePath), sub->printStorePath(info->path)); - tryNext(); - return; - } - } - /* Update the total expected download size. */ - auto narInfo = std::dynamic_pointer_cast(info); + auto narInfo = std::dynamic_pointer_cast(substituters.getInfo()); - maintainExpectedNar = std::make_unique>(worker.expectedNarSize, info->narSize); + maintainExpectedNar = std::make_unique>(worker.expectedNarSize, substituters.getInfo()->narSize); maintainExpectedDownload = narInfo && narInfo->fileSize @@ -149,20 +108,9 @@ void PathSubstitutionGoal::tryNext() worker.updateProgress(); - /* Bail out early if this substituter lacks a valid - signature. LocalStore::addToStore() also checks for this, but - only after we've downloaded the path. */ - if (!sub->isTrusted && worker.store.pathInfoIsUntrusted(*info)) - { - warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'", - worker.store.printStorePath(storePath), sub->getUri()); - tryNext(); - return; - } - /* To maintain the closure invariant, we first have to realise the paths referenced by this one. */ - for (auto & i : info->references) + for (auto & i : substituters.getInfo()->references) if (i != storePath) /* ignore self-references */ addWaitee(worker.makePathSubstitutionGoal(i)); @@ -185,7 +133,7 @@ void PathSubstitutionGoal::referencesValid() return; } - for (auto & i : info->references) + for (auto & i : substituters.getInfo()->references) if (i != storePath) /* ignore self-references */ assert(worker.store.isValidPath(i)); @@ -223,7 +171,7 @@ void PathSubstitutionGoal::tryToRun() PushActivity pact(act.id); copyStorePath(*sub, worker.store, - subPath ? *subPath : storePath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs); + substituters.getSubPath() ? *substituters.getSubPath(): storePath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs); promise.set_value(); } catch (...) { diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index a73f8e666..c556bdb9e 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -2,6 +2,7 @@ #include "lock.hh" #include "store-api.hh" +#include "substituter-set.hh" #include "goal.hh" namespace nix { @@ -13,12 +14,8 @@ struct PathSubstitutionGoal : public Goal /* The store path that should be realised through a substitute. */ StorePath storePath; - /* The path the substituter refers to the path as. This will be - different when the stores have different names. */ - std::optional subPath; - - /* The remaining substituters. */ - std::list> subs; + /* The set of substituters in use. */ + SubstituterSet substituters; /* The current substituter. */ std::shared_ptr sub; @@ -26,9 +23,6 @@ struct PathSubstitutionGoal : public Goal /* Whether a substituter failed. */ bool substituterFailed = false; - /* Path info returned by the substituter's query info operation. */ - std::shared_ptr info; - /* Pipe for the substituter's standard output. */ Pipe outPipe; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c9a466ee8..2caa7a887 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1121,6 +1121,7 @@ StorePathSet LocalStore::querySubstitutablePaths(const StorePathSet & paths) // FIXME: move this, it's not specific to LocalStore. +// FIXME: use SubstituterSet void LocalStore::querySubstitutablePathInfos(const StorePathCAMap & paths, SubstitutablePathInfos & infos) { if (!settings.useSubstitutes) return;