local-derivation-goal: improve "illegal reference" error

[trofi]

Motivation

Before the change "illegal reference" was hard to interpret as it did not mention what derivation actually hits it.

Today's nixpkgs example:

Before the change:

$ nix build --no-link -f. postgresql_14
...
error: derivation contains an illegal reference specifier 'man'

After the change:

$ nix build --no-link -f. postgresql_14
...
error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])

Context

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[wolfgangwalther]

Would it be possible to also print the file that contains that reference?

[wolfgangwalther]

As for the postgresql_14 example itself: This could be a nix bug in the first place - see efforts in NixOS/nixpkgs#368091 to track this down.

[trofi]

Would it be possible to also print the file that contains that reference?

Commit message shows the new error message as:

error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])

Do you mean something else than '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' derivation file?

[wolfgangwalther]

Do you mean something else than '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' derivation file?

Yeah, I mean the file inside the output. So in this case nix finds a file inside the lib output that has a reference to the man output, right? But which file inside ...-lib/* contains this reference?

[trofi]

As for the postgresql_14 example itself: This could be a nix bug in the first place - see efforts in NixOS/nixpkgs#368091 to track this down.

Yes, I believe it's a nix bug. But I would say fixing it out of scope of this PR (I did not start debugging it yet).

[wolfgangwalther]

But I would say fixing it out of scope of this PR (I did not start debugging it yet).

Yes, 100%.

[wolfgangwalther]

Yeah, I mean the file inside the output. So in this case nix finds a file inside the lib output that has a reference to the man output, right? But which file inside ...-lib/* contains this reference?

Maybe I misunderstand the error, though...

[trofi]

Do you mean something else than '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' derivation file?

Yeah, I mean the file inside the output. So in this case nix finds a file inside the lib output that has a reference to the man output, right? But which file inside ...-lib/* contains this reference?

No, I think nix believes that man output is not present in those derivation at all:

error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])

Note how man is not in the list of expected identifiers. The specific nix code is:

nix/src/libstore/unix/build/local-derivation-goal.cc

Line 2931 in 8a3fc27

throw BuildError("derivation contains an illegal reference specifier '%s'", i);

                for (auto & i : *value) {
                    if (worker.store.isStorePath(i))
                        spec.insert(worker.store.parseStorePath(i));
                    else if (auto output = get(outputs, i))
                        spec.insert(output->path);
                    else
                        throw BuildError("derivation contains an illegal reference specifier '%s'", i);
                }

I think auto output = get(outputs, i) is expected to return something here, but it does not. I would expect some ortdering problem where the map is corrupted, or missing prerequisite problem when only a subset of outputs fetched from the cache.

[wolfgangwalther]

No, I think nix believes that man output is not present in those derivation at all:

Ah, ok. Now I got it.

Could we change the error message as follows then?

-error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])
+error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output check for 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])

(is that correct?)

[trofi]

Yeah, that matches my understanding. Updated the PR with your suggestion as:

--- a/src/libstore/unix/build/local-derivation-goal.cc
+++ b/src/libstore/unix/build/local-derivation-goal.cc
@@ -2936,3 +2936,3 @@ void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo
                         }
-                        throw BuildError("derivation '%s' output '%s' contains an illegal reference specifier '%s',"
+                        throw BuildError("derivation '%s' output check for '%s' contains an illegal reference specifier '%s',"
                             " expected store path or output name (one of [%s])",