Forbid drvPath in strictDerivation outputs attribute

[tie]

Motivation

builtins.strictDerivation returns an attribute set with drvPath and output paths. For some reason, current implementation forbids drv instead of drvPath.

Context

n/a

Priorities and Process

Add 👍 to pull requests you find important.

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

[cole-h]

I may be wrong, but I think this code is less about forbidding the name of an attribute, and more forbidding the name of an output (i.e. the dev, bin, etc you can come across in Nixpkgs).

Looks like (something like) this check (for the drv output) has existed since at least Nix 1.0 (d329c3e)!

[tie]

Thanks for pointing that out. Looks like the drv was not allowed before for a reason (see below), but the check is out of date now 😅

nix/src/libexpr/primops.cc

Lines 522 to 528 in d329c3e

	mkString(*state.allocAttr(v, state.sDrvPath), drvPath, singleton<PathSet>("=" + drvPath));
	foreach (DerivationOutputs::iterator, i, drv.outputs) {
	/* The output path of an output X is ‘<X>Path’,
	e.g. ‘outPath’. */
	mkString(*state.allocAttr(v, state.symbols.create(i->first + "Path")),
	i->second.path, singleton<PathSet>(drvPath));
	}

Notice line 526. That is, back then, the resulting attribute set contained "${outputName}Path" attributes for each output. Now it’s just ${outputName}, so we should reject drvPath instead of drv.

nix/src/libexpr/primops.cc

Lines 1447 to 1451 in 081faed

	result.alloc(state.sDrvPath).mkString(drvPathS, {
	NixStringContextElem::DrvDeep { .drvPath = drvPath },
	});
	for (auto & i : drv.outputs)
	mkOutputString(state, result, drvPath, i);

[roberth]

Maybe with dynamic derivations, drv is a sensible output name, so removing the check sounds ok.
However, we shouldn't need to return drvPath in the first place, so making "new" rules about it seems a bit unnecessary to me.
More of this complexity could also be removed altogether with a new builtin.

[tie]

@roberth, I don’t think this is making new rules. Currently, Nix returns an error for

builtins.derivationStrict { outputs = [ "out" "drv" ]; name = "x"; system = "x"; builder = "x"; }

error: invalid derivation output name 'drv'

Presumably because it didn’t make sense when that check was added since the resulting attribute set contained ${outputName}Path keys, so output named drv would conflict with drvPath (see code references above). However, this check is out-of-date right now. For example, the following succeeds:

builtins.derivationStrict { outputs = [ "out" "drvPath" ]; name = "x"; system = "x"; builder = "x"; }

and returns

{ drvPath = "/nix/store/yv8hx0r7bk0b3k9cza6m5kfplnjnz30i-x.drv"; out = "/nix/store/pjgwjdg81dw1h0pwgckns19kydsxd8g7-x"; }

🤷

Perhaps we can remove the check with dynamic derivations, or a new builtin. That’s not the point of this PR though.
First, it refactors tests that were commented out before. Second, it adjust the check to forbid drvPath since that’s what we actually don’t want to be in outputs right now.

[roberth]

This seems to be the right behavior for now, for builtins.strictDerivation.
I don't think we need to perpetuate this limitation, but that's of later concern.
Thank you for the improvement and enabling those test!