Skip to content

Commit

Permalink
libstore: fix "illegal reference specifier 'man'"-error in postgresql_14
Browse files Browse the repository at this point in the history
Nixpkgs issues / PRs:
* NixOS/nixpkgs#368091
* NixOS/nixpkgs#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
  • Loading branch information
Ma27 committed Jan 5, 2025
1 parent 5c7ea4f commit a1c09be
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 6 deletions.
14 changes: 9 additions & 5 deletions lix/libstore/build/local-derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2031,6 +2031,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
OutputPathMap finalOutputs;

std::vector<std::pair<Path, std::optional<Path>>> nondeterministic;
std::map<std::string, StorePath> alreadyRegisteredOutputs;

for (auto & outputName : sortedOutputNames) {
auto output = get(drv->outputs, outputName);
Expand All @@ -2054,6 +2055,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
std::optional<StorePathSet> referencesOpt = std::visit(overloaded {
[&](const AlreadyRegistered & skippedFinalPath) -> std::optional<StorePathSet> {
finish(skippedFinalPath.path);
alreadyRegisteredOutputs.insert_or_assign(outputName, skippedFinalPath.path);
return std::nullopt;
},
[&](const PerhapsNeedToRegister & r) -> std::optional<StorePathSet> {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2438,13 +2440,13 @@ void LocalDerivationGoal::signRealisation(Realisation & realisation)
}


void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo> & outputs)
void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo> & newlyBuiltOutputs, const std::map<std::string, StorePath> & alreadyRegisteredOutputs)
{
std::map<Path, const ValidPathInfo &> 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;

Expand Down Expand Up @@ -2510,8 +2512,10 @@ void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo
for (auto & i : *value) {
if (worker.store.isStorePath(i))
spec.insert(worker.store.parseStorePath(i));
else if (auto output = get(outputs, i))
else if (auto output = get(newlyBuiltOutputs, i))
spec.insert(output->path);
else if (auto storePath = get(alreadyRegisteredOutputs, i))
spec.insert(*storePath);
else
throw BuildError("derivation contains an illegal reference specifier '%s'", i);
}
Expand Down
2 changes: 1 addition & 1 deletion lix/libstore/build/local-derivation-goal.hh
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ struct LocalDerivationGoal : public DerivationGoal
* 'outputChecks' attribute (or the legacy
* '{allowed,disallowed}{References,Requisites}' attributes).
*/
void checkOutputs(const std::map<std::string, ValidPathInfo> & outputs);
void checkOutputs(const std::map<std::string, ValidPathInfo> & outputs, const std::map<std::string, StorePath> & alreadyRegisteredOutputs);

/**
* Close the read side of the logger pipe.
Expand Down
1 change: 1 addition & 0 deletions tests/functional/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 12 additions & 0 deletions tests/functional/regression-reference-checks.nix
Original file line number Diff line number Diff line change
@@ -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]}
'';
}
9 changes: 9 additions & 0 deletions tests/functional/regression-reference-checks.sh
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit a1c09be

Please sign in to comment.