From a5df669bc685834b16de0ab57723ff734c10d2f7 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 19 May 2021 13:35:46 +0200 Subject: [PATCH 1/6] =?UTF-8?q?Add=20a=20test=20for=20the=20=E2=80=9Ctwo?= =?UTF-8?q?=20glibc=E2=80=9D=20issue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/ca/duplicate-realisation-in-closure.sh | 26 +++++++++++++++ tests/ca/nondeterministic.nix | 35 ++++++++++++++++++++ tests/local.mk | 1 + 3 files changed, 62 insertions(+) create mode 100644 tests/ca/duplicate-realisation-in-closure.sh create mode 100644 tests/ca/nondeterministic.nix diff --git a/tests/ca/duplicate-realisation-in-closure.sh b/tests/ca/duplicate-realisation-in-closure.sh new file mode 100644 index 000000000..bfe2a4e08 --- /dev/null +++ b/tests/ca/duplicate-realisation-in-closure.sh @@ -0,0 +1,26 @@ +source ./common.sh + +sed -i 's/experimental-features .*/& ca-derivations ca-references/' "$NIX_CONF_DIR"/nix.conf + +export REMOTE_STORE_DIR="$TEST_ROOT/remote_store" +export REMOTE_STORE="file://$REMOTE_STORE_DIR" + +rm -rf $REMOTE_STORE_DIR +clearStore + +# Build dep1 and push that to the binary cache. +# This entails building (and pushing) current-time. +nix copy --to "$REMOTE_STORE" -f nondeterministic.nix dep1 +clearStore +sleep 2 # To make sure that `$(date)` will be different +# Build dep2. +# As we’ve cleared the cache, we’ll have to rebuild current-time. And because +# the current time isn’t the same as before, this will yield a new (different) +# realisation +nix build -f nondeterministic.nix dep2 + +# Build something that depends both on dep1 and dep2. +# If everything goes right, we should rebuild dep2 rather than fetch it from +# the cache (because that would mean duplicating `current-time` in the closure), +# and have `dep1 == dep2`. +nix build --substituters "$REMOTE_STORE" -f nondeterministic.nix toplevel --no-require-sigs diff --git a/tests/ca/nondeterministic.nix b/tests/ca/nondeterministic.nix new file mode 100644 index 000000000..d6d099a3e --- /dev/null +++ b/tests/ca/nondeterministic.nix @@ -0,0 +1,35 @@ +with import ./config.nix; + +let mkCADerivation = args: mkDerivation ({ + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; +} // args); +in + +rec { + currentTime = mkCADerivation { + name = "current-time"; + buildCommand = '' + mkdir $out + echo $(date) > $out/current-time + ''; + }; + dep = seed: mkCADerivation { + name = "dep"; + inherit seed; + buildCommand = '' + echo ${currentTime} > $out + ''; + }; + dep1 = dep 1; + dep2 = dep 2; + toplevel = mkCADerivation { + name = "toplevel"; + buildCommand = '' + test ${dep1} == ${dep2} + touch $out + ''; + }; +} + diff --git a/tests/local.mk b/tests/local.mk index 0d594cda0..e7161d298 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -47,6 +47,7 @@ nix_tests = \ compute-levels.sh \ ca/build.sh \ ca/build-with-garbage-path.sh \ + ca/duplicate-realisation-in-closure.sh \ ca/substitute.sh \ ca/signatures.sh \ ca/nix-run.sh \ From b8f7177a7b2e884cbfb8bbe3c1ee8d586159fbb3 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 19 May 2021 16:19:46 +0200 Subject: [PATCH 2/6] Properly fail when trying to register an incoherent realisation --- src/libstore/local-store.cc | 136 +++++++++++++++++++++++++----------- src/libstore/local-store.hh | 2 + src/libstore/realisation.cc | 6 ++ src/libstore/realisation.hh | 2 + 4 files changed, 107 insertions(+), 39 deletions(-) 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)); + 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{ + 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); From d32cf0c17a3e5e139c384375084d9eb6aa428c3b Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 19 May 2021 16:27:09 +0200 Subject: [PATCH 3/6] Gracefully ignore a substituter if it holds an incompatible realisation --- .../build/drv-output-substitution-goal.cc | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) 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)); } } From 40f925b2dacb481b62d325fb41641804524a5dc8 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:42:23 +0200 Subject: [PATCH 4/6] Fix indentation --- src/libstore/local-store.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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( From 16fb7d8d95a8bc81e7df885ab4167c8a03f1dddf Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:46:29 +0200 Subject: [PATCH 5/6] 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(-) 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() From c878cee8954151aaa1054af7ef3746a979b05832 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 22 Jun 2021 10:50:28 +0200 Subject: [PATCH 6/6] 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(-) 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(