From b8f7177a7b2e884cbfb8bbe3c1ee8d586159fbb3 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 19 May 2021 16:19:46 +0200 Subject: Properly fail when trying to register an incoherent realisation --- src/libstore/local-store.cc | 138 +++++++++++++++++++++++++++++++------------- src/libstore/local-store.hh | 2 + src/libstore/realisation.cc | 6 ++ src/libstore/realisation.hh | 2 + 4 files changed, 108 insertions(+), 40 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c2256635a..8df1d55b9 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -53,6 +53,7 @@ struct LocalStore::State::Stmts { SQLiteStmt InvalidatePath; SQLiteStmt AddDerivationOutput; SQLiteStmt RegisterRealisedOutput; + SQLiteStmt UpdateRealisedOutput; SQLiteStmt QueryValidDerivers; SQLiteStmt QueryDerivationOutputs; SQLiteStmt QueryRealisedOutput; @@ -345,6 +346,15 @@ LocalStore::LocalStore(const Params & params) values (?, ?, (select id from ValidPaths where path = ?), ?) ; )"); + state->stmts->UpdateRealisedOutput.create(state->db, + R"( + update Realisations + set signatures = ? + where + drvPath = ? and + outputName = ? + ; + )"); state->stmts->QueryRealisedOutput.create(state->db, R"( select Realisations.id, Output.path, Realisations.signatures from Realisations @@ -710,14 +720,41 @@ void LocalStore::registerDrvOutput(const Realisation & info) settings.requireExperimentalFeature("ca-derivations"); retrySQLite([&]() { auto state(_state.lock()); - state->stmts->RegisterRealisedOutput.use() - (info.id.strHash()) - (info.id.outputName) - (printStorePath(info.outPath)) - (concatStringsSep(" ", info.signatures)) - .exec(); + if (auto oldR = queryRealisation_(*state, info.id)) { + if (info.isCompatibleWith(*oldR)) { + auto combinedSignatures = oldR->signatures; + combinedSignatures.insert(info.signatures.begin(), + info.signatures.end()); + state->stmts->UpdateRealisedOutput.use() + (concatStringsSep(" ", combinedSignatures)) + (info.id.strHash()) + (info.id.outputName) + .exec(); + } else { + throw Error("Trying to register a realisation of '%s', but we already " + "have another one locally", + info.id.to_string()); + } + } else { + state->stmts->RegisterRealisedOutput.use() + (info.id.strHash()) + (info.id.outputName) + (printStorePath(info.outPath)) + (concatStringsSep(" ", info.signatures)) + .exec(); + } uint64_t myId = state->db.getLastInsertedRowId(); - for (auto & [outputId, _] : info.dependentRealisations) { + for (auto & [outputId, depPath] : info.dependentRealisations) { + auto localRealisation = queryRealisationCore_(*state, outputId); + if (!localRealisation) + throw Error("unable to register the derivation '%s' as it " + "depends on the non existent '%s'", + info.id.to_string(), outputId.to_string()); + if (localRealisation->second.outPath != depPath) + throw Error("unable to register the derivation '%s' as it " + "depends on a realisation of '%s' that doesn’t" + "match what we have locally", + info.id.to_string(), outputId.to_string()); state->stmts->AddRealisationReference.use() (myId) (outputId.strHash()) @@ -1734,46 +1771,67 @@ void LocalStore::createUser(const std::string & userName, uid_t userId) } } -std::optional LocalStore::queryRealisation( - const DrvOutput& id) { - typedef std::optional Ret; - return retrySQLite([&]() -> Ret { - auto state(_state.lock()); - auto useQueryRealisedOutput(state->stmts->QueryRealisedOutput.use() +std::optional> LocalStore::queryRealisationCore_( + LocalStore::State & state, + const DrvOutput & id) +{ + auto useQueryRealisedOutput( + state.stmts->QueryRealisedOutput.use() (id.strHash()) (id.outputName)); - if (!useQueryRealisedOutput.next()) - return std::nullopt; - auto realisationDbId = useQueryRealisedOutput.getInt(0); - auto outputPath = parseStorePath(useQueryRealisedOutput.getStr(1)); - auto signatures = - tokenizeString(useQueryRealisedOutput.getStr(2)); - - std::map dependentRealisations; - auto useRealisationRefs( - state->stmts->QueryRealisationReferences.use() - (realisationDbId)); - while (useRealisationRefs.next()) { - auto depHash = useRealisationRefs.getStr(0); - auto depOutputName = useRealisationRefs.getStr(1); - auto useQueryRealisedOutput(state->stmts->QueryRealisedOutput.use() - (depHash) - (depOutputName)); - assert(useQueryRealisedOutput.next()); - auto outputPath = parseStorePath(useQueryRealisedOutput.getStr(1)); - auto depId = DrvOutput { Hash::parseAnyPrefixed(depHash), depOutputName }; - dependentRealisations.insert({depId, outputPath}); - } - - return Ret{Realisation{ + if (!useQueryRealisedOutput.next()) + return std::nullopt; + auto realisationDbId = useQueryRealisedOutput.getInt(0); + auto outputPath = parseStorePath(useQueryRealisedOutput.getStr(1)); + auto signatures = + tokenizeString(useQueryRealisedOutput.getStr(2)); + + return {{ + realisationDbId, + Realisation{ .id = id, .outPath = outputPath, .signatures = signatures, - .dependentRealisations = dependentRealisations, - }}; - }); + } + }}; } +std::optional LocalStore::queryRealisation_( + LocalStore::State & state, + const DrvOutput & id) +{ + auto maybeCore = queryRealisationCore_(state, id); + if (!maybeCore) + return std::nullopt; + auto [realisationDbId, res] = *maybeCore; + + std::map dependentRealisations; + auto useRealisationRefs( + state.stmts->QueryRealisationReferences.use() + (realisationDbId)); + while (useRealisationRefs.next()) { + auto depId = DrvOutput { + Hash::parseAnyPrefixed(useRealisationRefs.getStr(0)), + useRealisationRefs.getStr(1), + }; + auto dependentRealisation = queryRealisationCore_(state, depId); + assert(dependentRealisation); // Enforced by the db schema + auto outputPath = dependentRealisation->second.outPath; + dependentRealisations.insert({depId, outputPath}); + } + + res.dependentRealisations = dependentRealisations; + + return { res }; +} + +std::optional +LocalStore::queryRealisation(const DrvOutput &id) { + return retrySQLite>([&]() { + auto state(_state.lock()); + return queryRealisation_(*state, id); + }); +} FixedOutputHash LocalStore::hashCAPath( const FileIngestionMethod & method, const HashType & hashType, diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 15c7fc306..a01d48c4b 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -203,6 +203,8 @@ public: void registerDrvOutput(const Realisation & info, CheckSigsFlag checkSigs) override; void cacheDrvOutputMapping(State & state, const uint64_t deriver, const string & outputName, const StorePath & output); + std::optional queryRealisation_(State & state, const DrvOutput & id); + std::optional> queryRealisationCore_(State & state, const DrvOutput & id); std::optional queryRealisation(const DrvOutput&) override; private: diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index 0d9d4b433..76aec74ce 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -140,6 +140,12 @@ StorePath RealisedPath::path() const { return std::visit([](auto && arg) { return arg.getPath(); }, raw); } +bool Realisation::isCompatibleWith(const Realisation & other) const +{ + assert (id == other.id); + return outPath == other.outPath; +} + void RealisedPath::closure( Store& store, const RealisedPath::Set& startPaths, diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 7fdb65acd..05d2bc44f 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -47,6 +47,8 @@ struct Realisation { static std::set closure(Store &, const std::set &); static void closure(Store &, const std::set &, std::set& res); + bool isCompatibleWith(const Realisation & other) const; + StorePath getPath() const { return outPath; } GENERATE_CMP(Realisation, me->id, me->outPath); -- cgit v1.2.3 From d32cf0c17a3e5e139c384375084d9eb6aa428c3b Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 19 May 2021 16:27:09 +0200 Subject: Gracefully ignore a substituter if it holds an incompatible realisation --- src/libstore/build/drv-output-substitution-goal.cc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 1703e845d..ec3a8d758 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -17,6 +17,13 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal(const DrvOutput& id, Worker void DrvOutputSubstitutionGoal::init() { trace("init"); + + /* If the derivation already exists, we’re done */ + if (worker.store.queryRealisation(id)) { + amDone(ecSuccess); + return; + } + subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); tryNext(); } @@ -53,9 +60,18 @@ void DrvOutputSubstitutionGoal::tryNext() return; } - for (const auto & [drvOutputDep, _] : outputInfo->dependentRealisations) { - if (drvOutputDep != id) { - addWaitee(worker.makeDrvOutputSubstitutionGoal(drvOutputDep)); + for (const auto & [depId, depPath] : outputInfo->dependentRealisations) { + if (depId != id) { + if (auto localOutputInfo = worker.store.queryRealisation(depId); + localOutputInfo && localOutputInfo->outPath != depPath) { + warn( + "substituter '%s' has an incompatible realisation for '%s', ignoring", + sub->getUri(), + depId.to_string()); + tryNext(); + return; + } + addWaitee(worker.makeDrvOutputSubstitutionGoal(depId)); } } -- cgit v1.2.3 From 40f925b2dacb481b62d325fb41641804524a5dc8 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:42:23 +0200 Subject: Fix indentation --- src/libstore/local-store.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 8df1d55b9..064f7a432 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1826,11 +1826,12 @@ std::optional LocalStore::queryRealisation_( } std::optional -LocalStore::queryRealisation(const DrvOutput &id) { - return retrySQLite>([&]() { - auto state(_state.lock()); - return queryRealisation_(*state, id); - }); +LocalStore::queryRealisation(const DrvOutput & id) +{ + return retrySQLite>([&]() { + auto state(_state.lock()); + return queryRealisation_(*state, id); + }); } FixedOutputHash LocalStore::hashCAPath( -- cgit v1.2.3 From 16fb7d8d95a8bc81e7df885ab4167c8a03f1dddf Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:46:29 +0200 Subject: Display the diverging paths in case of a realisation mismatch --- src/libstore/build/drv-output-substitution-goal.cc | 9 +++++++-- src/libstore/local-store.cc | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index ec3a8d758..be270d079 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -65,9 +65,14 @@ void DrvOutputSubstitutionGoal::tryNext() if (auto localOutputInfo = worker.store.queryRealisation(depId); localOutputInfo && localOutputInfo->outPath != depPath) { warn( - "substituter '%s' has an incompatible realisation for '%s', ignoring", + "substituter '%s' has an incompatible realisation for '%s', ignoring.\n" + "Local: %s\n" + "Remote: %s", sub->getUri(), - depId.to_string()); + depId.to_string(), + worker.store.printStorePath(localOutputInfo->outPath), + worker.store.printStorePath(depPath) + ); tryNext(); return; } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 064f7a432..d7c7f8e1d 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -732,8 +732,13 @@ void LocalStore::registerDrvOutput(const Realisation & info) .exec(); } else { throw Error("Trying to register a realisation of '%s', but we already " - "have another one locally", - info.id.to_string()); + "have another one locally.\n" + "Local: %s\n" + "Remote: %s", + info.id.to_string(), + printStorePath(oldR->outPath), + printStorePath(info.outPath) + ); } } else { state->stmts->RegisterRealisedOutput.use() -- cgit v1.2.3 From c878cee8954151aaa1054af7ef3746a979b05832 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:50:28 +0200 Subject: Assert that compatible realisations have the same dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should always hold, but that’s not necessarily obvious, so better enforce it --- src/libstore/realisation.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src/libstore') diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index 76aec74ce..eadec594c 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -143,7 +143,11 @@ StorePath RealisedPath::path() const { bool Realisation::isCompatibleWith(const Realisation & other) const { assert (id == other.id); - return outPath == other.outPath; + if (outPath == other.outPath) { + assert(dependentRealisations == other.dependentRealisations); + return true; + } + return false; } void RealisedPath::closure( -- cgit v1.2.3