From 869fb1a2f6b8374feacb5ed730e0a0f6cbb195c4 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 31 Jan 2023 12:49:33 +0100 Subject: [PATCH 1/5] tests/post-hook: test to see if all outputs are passed fe5509df caused only wanted outputs to be passed to the post-build-hook, which resulted in paths being built without ever going into the hook. This commit adds a (currently failing) test for this. --- tests/post-hook.sh | 4 ++++ tests/push-to-store-old.sh | 6 +++++- tests/push-to-store.sh | 6 +++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/post-hook.sh b/tests/post-hook.sh index 0266eb15d..8b7838058 100644 --- a/tests/post-hook.sh +++ b/tests/post-hook.sh @@ -17,6 +17,9 @@ fi # Build the dependencies and push them to the remote store. nix-build -o $TEST_ROOT/result dependencies.nix --post-build-hook "$pushToStore" +# See if all outputs are passed to the post-build hook by only specifying one +export BUILD_HOOK_ONLY_OUT_PATHS=1 +nix-build -o $TEST_ROOT/result-mult multiple-outputs.nix -A a.first --post-build-hook "$pushToStore" clearStore @@ -24,3 +27,4 @@ clearStore # closure of what we've just built. nix copy --from "$REMOTE_STORE" --no-require-sigs -f dependencies.nix nix copy --from "$REMOTE_STORE" --no-require-sigs -f dependencies.nix input1_drv +nix copy --from "$REMOTE_STORE" --no-require-sigs -f multiple-outputs.nix a^second diff --git a/tests/push-to-store-old.sh b/tests/push-to-store-old.sh index b1495c9e2..4187958b2 100755 --- a/tests/push-to-store-old.sh +++ b/tests/push-to-store-old.sh @@ -7,4 +7,8 @@ set -e [ -n "$DRV_PATH" ] echo Pushing "$OUT_PATHS" to "$REMOTE_STORE" -printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +if [ -n "$BUILD_HOOK_ONLY_OUT_PATHS" ]; then + printf "%s" "$OUT_PATHS" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +else + printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +fi diff --git a/tests/push-to-store.sh b/tests/push-to-store.sh index 0b090e1b3..9e4e475e0 100755 --- a/tests/push-to-store.sh +++ b/tests/push-to-store.sh @@ -7,4 +7,8 @@ set -e [ -n "$DRV_PATH" ] echo Pushing "$OUT_PATHS" to "$REMOTE_STORE" -printf "%s" "$DRV_PATH"^'*' | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +if [ -n "$BUILD_HOOK_ONLY_OUT_PATHS" ]; then + printf "%s" "$OUT_PATHS" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +else + printf "%s" "$DRV_PATH"^'*' | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs +fi From 2ca2c80c4e08544cb64f4dc00d083fb9540c2b04 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 31 Jan 2023 12:51:12 +0100 Subject: [PATCH 2/5] libstore: also pass unwanted outputs to the post-build-hook --- src/libstore/build/derivation-goal.cc | 9 +++++---- src/libstore/build/derivation-goal.hh | 6 ++---- src/libstore/build/local-derivation-goal.cc | 3 +-- src/libstore/realisation.cc | 13 +++++++++++++ src/libstore/realisation.hh | 9 +++++++++ 5 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a4bb94b0e..2aaeaec6e 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1064,7 +1064,7 @@ void DerivationGoal::resolvedFinished() worker.store.registerDrvOutput(newRealisation); } outputPaths.insert(realisation.outPath); - builtOutputs.emplace(wantedOutput, realisation); + builtOutputs.emplace(outputName, realisation); } runPostBuildHook( @@ -1406,7 +1406,7 @@ std::pair DerivationGoal::checkPathValidity() ); } } - if (info.wanted && info.known && info.known->isValid()) + if (info.known && info.known->isValid()) validOutputs.emplace(i.first, Realisation { drvOutput, info.known->path }); } @@ -1457,8 +1457,9 @@ void DerivationGoal::done( mcRunningBuilds.reset(); if (buildResult.success()) { - assert(!builtOutputs.empty()); - buildResult.builtOutputs = std::move(builtOutputs); + auto wantedBuiltOutputs = filterDrvOutputs(wantedOutputs, std::move(builtOutputs)); + assert(!wantedBuiltOutputs.empty()); + buildResult.builtOutputs = std::move(wantedBuiltOutputs); if (status == BuildResult::Built) worker.doneBuilds++; } else { diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 7033b7a58..01a08d391 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -306,15 +306,13 @@ struct DerivationGoal : public Goal * Update 'initialOutputs' to determine the current status of the * outputs of the derivation. Also returns a Boolean denoting * whether all outputs are valid and non-corrupt, and a - * 'SingleDrvOutputs' structure containing the valid and wanted - * outputs. + * 'SingleDrvOutputs' structure containing the valid outputs. */ std::pair checkPathValidity(); /** * Aborts if any output is not valid or corrupt, and otherwise - * returns a 'SingleDrvOutputs' structure containing the wanted - * outputs. + * returns a 'SingleDrvOutputs' structure containing all outputs. */ SingleDrvOutputs assertPathValidity(); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 21cd6e7ee..2f545627b 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2701,8 +2701,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } - if (wantedOutputs.contains(outputName)) - builtOutputs.emplace(outputName, thisRealisation); + builtOutputs.emplace(outputName, thisRealisation); } return builtOutputs; diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index d63ec5ea2..93ddb5b20 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -136,6 +136,19 @@ size_t Realisation::checkSignatures(const PublicKeys & publicKeys) const return good; } + +SingleDrvOutputs filterDrvOutputs(const OutputsSpec& wanted, SingleDrvOutputs&& outputs) +{ + SingleDrvOutputs ret = std::move(outputs); + for (auto it = ret.begin(); it != ret.end(); ) { + if (!wanted.contains(it->first)) + it = ret.erase(it); + else + ++it; + } + return ret; +} + StorePath RealisedPath::path() const { return std::visit([](auto && arg) { return arg.getPath(); }, raw); } diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 3922d1267..2a093c128 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -12,6 +12,7 @@ namespace nix { class Store; +struct OutputsSpec; /** * A general `Realisation` key. @@ -93,6 +94,14 @@ typedef std::map SingleDrvOutputs; */ typedef std::map DrvOutputs; +/** + * Filter a SingleDrvOutputs to include only specific output names + * + * Moves the `outputs` input. + */ +SingleDrvOutputs filterDrvOutputs(const OutputsSpec&, SingleDrvOutputs&&); + + struct OpaquePath { StorePath path; From 12685ef45fa81e5a5ea550bf469d7722fe9c8709 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Fri, 24 Feb 2023 16:31:58 +0100 Subject: [PATCH 3/5] CA: rewrite hashes for all outputs, not just the wanted ones --- src/libstore/build/derivation-goal.cc | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 2aaeaec6e..23724b0d9 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1020,43 +1020,33 @@ void DerivationGoal::resolvedFinished() StorePathSet outputPaths; - // `wantedOutputs` might merely indicate “all the outputs” - auto realWantedOutputs = std::visit(overloaded { - [&](const OutputsSpec::All &) { - return resolvedDrv.outputNames(); - }, - [&](const OutputsSpec::Names & names) { - return static_cast>(names); - }, - }, wantedOutputs.raw()); - - for (auto & wantedOutput : realWantedOutputs) { - auto initialOutput = get(initialOutputs, wantedOutput); - auto resolvedHash = get(resolvedHashes, wantedOutput); + for (auto & outputName : resolvedDrv.outputNames()) { + auto initialOutput = get(initialOutputs, outputName); + auto resolvedHash = get(resolvedHashes, outputName); if ((!initialOutput) || (!resolvedHash)) throw Error( "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolvedFinished,resolve)", - worker.store.printStorePath(drvPath), wantedOutput); + worker.store.printStorePath(drvPath), outputName); auto realisation = [&]{ - auto take1 = get(resolvedResult.builtOutputs, wantedOutput); + auto take1 = get(resolvedResult.builtOutputs, outputName); if (take1) return *take1; /* The above `get` should work. But sateful tracking of outputs in resolvedResult, this can get out of sync with the store, which is our actual source of truth. For now we just check the store directly if it fails. */ - auto take2 = worker.evalStore.queryRealisation(DrvOutput { *resolvedHash, wantedOutput }); + auto take2 = worker.evalStore.queryRealisation(DrvOutput { *resolvedHash, outputName }); if (take2) return *take2; throw Error( "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolvedFinished,realisation)", - worker.store.printStorePath(resolvedDrvGoal->drvPath), wantedOutput); + worker.store.printStorePath(resolvedDrvGoal->drvPath), outputName); }(); if (drv->type().isPure()) { auto newRealisation = realisation; - newRealisation.id = DrvOutput { initialOutput->outputHash, wantedOutput }; + newRealisation.id = DrvOutput { initialOutput->outputHash, outputName }; newRealisation.signatures.clear(); if (!drv->type().isFixed()) newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath); From 5e332aa5030d2d6780db18c4cb042e3a6f178923 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Fri, 24 Feb 2023 16:33:28 +0100 Subject: [PATCH 4/5] tests: copying only the out paths is not enough information for CA --- tests/post-hook.sh | 3 ++- tests/push-to-store.sh | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/post-hook.sh b/tests/post-hook.sh index 8b7838058..c974ea1b0 100644 --- a/tests/post-hook.sh +++ b/tests/post-hook.sh @@ -18,7 +18,8 @@ fi # Build the dependencies and push them to the remote store. nix-build -o $TEST_ROOT/result dependencies.nix --post-build-hook "$pushToStore" # See if all outputs are passed to the post-build hook by only specifying one -export BUILD_HOOK_ONLY_OUT_PATHS=1 +# TODO: BUILD_HOOK_ONLY_OUT_PATHS does not work with CA tests +export BUILD_HOOK_ONLY_OUT_PATHS=$([ ! $NIX_TESTS_CA_BY_DEFAULT ]) nix-build -o $TEST_ROOT/result-mult multiple-outputs.nix -A a.first --post-build-hook "$pushToStore" clearStore diff --git a/tests/push-to-store.sh b/tests/push-to-store.sh index 9e4e475e0..b5f099fe2 100755 --- a/tests/push-to-store.sh +++ b/tests/push-to-store.sh @@ -9,6 +9,7 @@ set -e echo Pushing "$OUT_PATHS" to "$REMOTE_STORE" if [ -n "$BUILD_HOOK_ONLY_OUT_PATHS" ]; then printf "%s" "$OUT_PATHS" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs + printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs --derivation else printf "%s" "$DRV_PATH"^'*' | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs fi From d1ff33d2d6f966da4d7ee85918cf6b12b951135f Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 28 Feb 2023 10:56:12 +0100 Subject: [PATCH 5/5] tests/post-hook: remove TODO and --derivation upload --- tests/post-hook.sh | 2 +- tests/push-to-store.sh | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/post-hook.sh b/tests/post-hook.sh index c974ea1b0..752f8220c 100644 --- a/tests/post-hook.sh +++ b/tests/post-hook.sh @@ -18,7 +18,7 @@ fi # Build the dependencies and push them to the remote store. nix-build -o $TEST_ROOT/result dependencies.nix --post-build-hook "$pushToStore" # See if all outputs are passed to the post-build hook by only specifying one -# TODO: BUILD_HOOK_ONLY_OUT_PATHS does not work with CA tests +# We're not able to test CA tests this way export BUILD_HOOK_ONLY_OUT_PATHS=$([ ! $NIX_TESTS_CA_BY_DEFAULT ]) nix-build -o $TEST_ROOT/result-mult multiple-outputs.nix -A a.first --post-build-hook "$pushToStore" diff --git a/tests/push-to-store.sh b/tests/push-to-store.sh index b5f099fe2..9e4e475e0 100755 --- a/tests/push-to-store.sh +++ b/tests/push-to-store.sh @@ -9,7 +9,6 @@ set -e echo Pushing "$OUT_PATHS" to "$REMOTE_STORE" if [ -n "$BUILD_HOOK_ONLY_OUT_PATHS" ]; then printf "%s" "$OUT_PATHS" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs - printf "%s" "$DRV_PATH" | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs --derivation else printf "%s" "$DRV_PATH"^'*' | xargs nix copy --to "$REMOTE_STORE" --no-require-sigs fi