From a1c09bed7b9831d65a569f3422750546d822e99d Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 4 Jan 2025 10:55:28 +0100 Subject: [PATCH] libstore: fix "illegal reference specifier 'man'"-error in postgresql_14 Nixpkgs issues / PRs: * https://github.com/NixOS/nixpkgs/pull/368091 * https://github.com/NixOS/nixpkgs/issues/369366 This can be triggered with the postgresql_14 derivation from nixpkgs rev 19305d94dacca226ca048b78e6de00f599c65858 (/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv on x86_64-linux): for reasons unknown to me, only the `man` and `lib` outputs are cached on cache.nixos.org: $ nix derivation show /nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv | jq '.[].outputs.[].path' -r | xargs nix path-info --store https://cache.nixos.org warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv^*' don't know how to build these paths: /nix/store/m9vb40xxr6gckjzpfxnqcmjqsks2gx03-postgresql-14.15 /nix/store/nm1415wa53iawar9axwxy0an6ximhayn-postgresql-14.15-dev /nix/store/v9vrvfhiw9gk8hj9895sb15fxvxnyylj-postgresql-14.15-debug /nix/store/zi12g1p99g2173i8093ixbqkfh9ng87b-postgresql-14.15-doc /nix/store/3i3fpz0xss9inampf51gp3pkx24ypxpj-postgresql-14.15-man /nix/store/db8797h2cp4rm1cnsqrf87apkkxwwdff-postgresql-14.15-lib error: path '/nix/store/m9vb40xxr6gckjzpfxnqcmjqsks2gx03-postgresql-14.15' does not exist in the store Also, the derivation uses the `outputChecks` feature (and thus `__structuredAttrs`) to make sure that e.g. the `out` output doesn't reference the `man` output: __structuredAttrs = true; outputs = [ "out" "dev" "doc" "lib" "man" ]; outputChecks.out.disallowedReferences = [ "dev" "doc" "man" ]; With all that in place, the following error was hit on all CppNix / Lix versions currently supported when trying to build the derivation above: error: derivation contains an illegal reference specifier 'man' The following happened here: * The `man` & `lib` outputs were substituted at some point. * When register outputs, the reference checks are made. * `LocalDerivationGoal::checkOutputs` gets a map of all outputs that were built and are NOT already registered in the store. In the example above this means `out`, `dev`, `debug` and `doc`. * `checkOutputs` tries to resolve the `man` output and fails to do so because it's a store-path that's already registered and thus not part of the map passed to `checkOutputs`. Since the map passed to `checkOutputs` is used in various other places that appear to assume that the paths aren't registered already, I didn't write the already registered paths into it. Instead, I created a second map that contains all already registered outputs and pass it as third argument to `checkOutputs`. If the other lookups fail, this map will be now checked before the "illegal reference specifier"-error is thrown. This fixes the problem with `postgresql_14` for me. Also wrote a small regression test that fails locally without the patch in place. Change-Id: Ieacca80c001fcfbebf6f5fe97e25c49d2724c3ff --- lix/libstore/build/local-derivation-goal.cc | 14 +++++++++----- lix/libstore/build/local-derivation-goal.hh | 2 +- tests/functional/meson.build | 1 + tests/functional/regression-reference-checks.nix | 12 ++++++++++++ tests/functional/regression-reference-checks.sh | 9 +++++++++ 5 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 tests/functional/regression-reference-checks.nix create mode 100644 tests/functional/regression-reference-checks.sh diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 794b85031..625f33cba 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -2031,6 +2031,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() OutputPathMap finalOutputs; std::vector>> nondeterministic; + std::map alreadyRegisteredOutputs; for (auto & outputName : sortedOutputNames) { auto output = get(drv->outputs, outputName); @@ -2054,6 +2055,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() std::optional referencesOpt = std::visit(overloaded { [&](const AlreadyRegistered & skippedFinalPath) -> std::optional { finish(skippedFinalPath.path); + alreadyRegisteredOutputs.insert_or_assign(outputName, skippedFinalPath.path); return std::nullopt; }, [&](const PerhapsNeedToRegister & r) -> std::optional { @@ -2383,7 +2385,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() } /* Apply output checks. */ - checkOutputs(infos); + checkOutputs(infos, alreadyRegisteredOutputs); /* Register each output path as valid, and register the sets of paths referenced by each of them. If there are cycles in the @@ -2438,13 +2440,13 @@ void LocalDerivationGoal::signRealisation(Realisation & realisation) } -void LocalDerivationGoal::checkOutputs(const std::map & outputs) +void LocalDerivationGoal::checkOutputs(const std::map & newlyBuiltOutputs, const std::map & alreadyRegisteredOutputs) { std::map outputsByPath; - for (auto & output : outputs) + for (auto & output : newlyBuiltOutputs) outputsByPath.emplace(worker.store.printStorePath(output.second.path), output.second); - for (auto & output : outputs) { + for (auto & output : newlyBuiltOutputs) { auto & outputName = output.first; auto & info = output.second; @@ -2510,8 +2512,10 @@ void LocalDerivationGoal::checkOutputs(const std::mappath); + else if (auto storePath = get(alreadyRegisteredOutputs, i)) + spec.insert(*storePath); else throw BuildError("derivation contains an illegal reference specifier '%s'", i); } diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index cfd31ef81..96f0c41a0 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/lix/libstore/build/local-derivation-goal.hh @@ -270,7 +270,7 @@ struct LocalDerivationGoal : public DerivationGoal * 'outputChecks' attribute (or the legacy * '{allowed,disallowed}{References,Requisites}' attributes). */ - void checkOutputs(const std::map & outputs); + void checkOutputs(const std::map & outputs, const std::map & alreadyRegisteredOutputs); /** * Close the read side of the logger pipe. diff --git a/tests/functional/meson.build b/tests/functional/meson.build index 891a755d7..e54daa1a3 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -197,6 +197,7 @@ functional_tests_scripts = [ 'extra-sandbox-profile.sh', 'substitute-truncated-nar.sh', 'regression-484.sh', + 'regression-reference-checks.sh', ] # Plugin tests require shared libraries support. diff --git a/tests/functional/regression-reference-checks.nix b/tests/functional/regression-reference-checks.nix new file mode 100644 index 000000000..771896e37 --- /dev/null +++ b/tests/functional/regression-reference-checks.nix @@ -0,0 +1,12 @@ +with import ./config.nix; +mkDerivation { + name = "test"; + __structuredAttrs = true; + outputs = [ "out" "man" ]; + outputChecks.out.disallowedReferences = [ "man" ]; + buildCommand = '' + source $NIX_ATTRS_SH_FILE + mkdir ''${outputs[out]} + mkdir ''${outputs[man]} + ''; +} diff --git a/tests/functional/regression-reference-checks.sh b/tests/functional/regression-reference-checks.sh new file mode 100644 index 000000000..3e07b164c --- /dev/null +++ b/tests/functional/regression-reference-checks.sh @@ -0,0 +1,9 @@ +source common.sh + +clearStore + +outpath="$(nix-build regression-reference-checks.nix -A out --no-out-link)" +manpage="$(nix-build regression-reference-checks.nix -A man --no-out-link)" + +nix-store --delete "$outpath" +nix-build regression-reference-checks.nix -A out --no-out-link