From d19593d00b33768abc5c04a876cce9859302ba0c Mon Sep 17 00:00:00 2001 From: Linus Heckemann Date: Wed, 7 May 2025 20:52:19 +0200 Subject: [PATCH] libstore/local-derivation-goal: better debuggability for FOD mismatches The expected and the obtained path are now printed as part of the error message, making comparing them easier when they're both at hand. The extra rethrow for the hash-mismatch exception in the bmCheck case has been removed, allowing the path to be registered as in the non-check case. This makes having both paths at hand a lot more likely! The determinism check logic was incorrect for content-addressed paths, since it only ever tried to compare the path produced, even if this was not the path expected (in the case of fixed-output derivations) or the path previously produced (in the case of non-fixed CA derivations). This made little sense, because that would always be the same path if it exists! The determinism check is therefore now bypassed for CA paths. Having a correct determinism check for non-fixed CA derivations and running the diff hook for fixed-output derivations would be nice, but feels out of scope and bypassing the inapplicable logic isn't a regression from the previous behaviour. Change-Id: I5fc14fb477c8c7d2f5bdedad5591af916f72b128 --- doc/manual/rl-next/mismatch-check-path.md | 14 ++++++++++ lix/libstore/build/local-derivation-goal.cc | 30 ++++++++++++--------- tests/functional/check.sh | 20 +++++++++++--- 3 files changed, 48 insertions(+), 16 deletions(-) create mode 100644 doc/manual/rl-next/mismatch-check-path.md diff --git a/doc/manual/rl-next/mismatch-check-path.md b/doc/manual/rl-next/mismatch-check-path.md new file mode 100644 index 000000000..a0e2ba95d --- /dev/null +++ b/doc/manual/rl-next/mismatch-check-path.md @@ -0,0 +1,14 @@ +--- +synopsis: Better debuggability on fixed-output hash mismatches +issues: [] +cls: [] +category: Improvements +credits: [lheckemann] +--- + +Fixed-output derivation hash mismatch error messages will now include the path that was +produced unexpectedly, and this path will be registered as valid even if `--check` +(`nix-store`, `nix-build`) or `--rebuild` (`nix build`) was passed. This makes comparing +the expected path with the obtained path easier, and is useful for debugging when +upstreams modify previously-published releases or when changes in fixed-output +derivations' dependencies affect their output unexpectedly. diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 79e63b149..1577b2476 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -1855,11 +1855,13 @@ try { // XXX: shameless layering violation hack that makes the hash mismatch error at least not utterly worthless auto guessedUrl = getOr(drv->env, "urls", getOr(drv->env, "url", "(unknown)")); delayedException = std::make_exception_ptr( - BuildError("hash mismatch in fixed-output derivation '%s':\n likely URL: %s\n specified: %s\n got: %s", + BuildError("hash mismatch in fixed-output derivation '%s':\n likely URL: %s\n specified: %s\n got: %s\n expected path: %s\n got path: %s", worker.store.printStorePath(drvPath), guessedUrl, wanted.to_string(Base::SRI, true), - got.to_string(Base::SRI, true))); + got.to_string(Base::SRI, true), + worker.store.printStorePath(dof.path(worker.store, drv->name, outputName)), + worker.store.printStorePath(newInfo0.path))); } if (!newInfo0.references.empty()) { std::string references; @@ -1918,9 +1920,10 @@ try { /* Path already exists, need to replace it */ replaceValidPath(worker.store.toRealPath(finalDestPath), actualPath); actualPath = worker.store.toRealPath(finalDestPath); - } else if (buildMode == bmCheck) { - /* Path already exists, and we want to compare, so we leave out - new path in place. */ + } else if (buildMode == bmCheck && TRY_AWAIT(worker.store.isValidPath(newInfo.path))) { + /* Path already exists, and we want to compare, so we + don't replace the previously existing output with + the new one. */ } else if (TRY_AWAIT(worker.store.isValidPath(newInfo.path))) { /* Path already exists because CA path produced by something else. No moving needed. */ @@ -1935,8 +1938,12 @@ try { auto & localStore = getLocalStore(); - if (buildMode == bmCheck) { + // Check determinism and run the diff hook for input-addressed + // paths if we're in check mode. + // TODO: implement this for content-addressed paths too. + if (buildMode == bmCheck && !newInfo.ca) { + // We can only do this if we have a previous output path to compare. if (!TRY_AWAIT(worker.store.isValidPath(newInfo.path))) continue; ValidPathInfo oldInfo(*TRY_AWAIT(worker.store.queryPathInfo(newInfo.path))); if (newInfo.narHash != oldInfo.narHash) { @@ -2007,12 +2014,11 @@ try { } throw NotDeterministic(msg.str()); } - /* In case of fixed-output derivations, if there are - mismatches on `--check` an error must be thrown as this is - also a source for non-determinism. */ - if (delayedException) - std::rethrow_exception(delayedException); - co_return TRY_AWAIT(assertPathValidity()); + /* In case of fixed-output derivations with hash mismatches, + we don't want to rethrow the exception until later so that + the unexpected path is still registered as valid. */ + if (!delayedException) + co_return TRY_AWAIT(assertPathValidity()); } /* Apply output checks. */ diff --git a/tests/functional/check.sh b/tests/functional/check.sh index 653563971..fc63b9e21 100644 --- a/tests/functional/check.sh +++ b/tests/functional/check.sh @@ -136,15 +136,27 @@ nix-build check.nix -A fetchurl --no-out-link --repair [[ $(cat $path) != foo ]] echo 'Hello World' > $TEST_ROOT/dummy -nix-build check.nix -A hashmismatch --no-out-link || status=$? +nix-build check.nix -A hashmismatch --no-out-link 2>mismatch-output || status=$? [ "$status" = "102" ] +obtained=$(grep "got path:" mismatch-output | awk '{ print $3 }') +# The path that actually came out should exist +[ -e "$obtained" ] +# and be registered as valid +nix-store -q --references "$obtained" >/dev/null echo -n > $TEST_ROOT/dummy -nix-build check.nix -A hashmismatch --no-out-link -echo 'Hello World' > $TEST_ROOT/dummy +successful_fod=$(nix-build check.nix -A hashmismatch --no-out-link) -nix-build check.nix -A hashmismatch --no-out-link --check || status=$? +echo 'Hello World 2' > $TEST_ROOT/dummy +nix-build check.nix -A hashmismatch --no-out-link --check 2>mismatch-output || status=$? +cat mismatch-output >&2 [ "$status" = "102" ] +grep -E "expected path:\s+$successful_fod" mismatch-output +obtained=$(grep "got path:" mismatch-output | awk '{ print $3 }') +# The path that actually came out should exist +[ -e "$obtained" ] +# and be registered as valid +nix-store -q --references "$obtained" >/dev/null # Multiple failures with --keep-going nix-build check.nix -A nondeterministic --no-out-link