testers.testBuildFailure: fix structuredAttrs support

[MattSturgeon]

I noticed that testers.testBuildFailure was not working with __structuredAttrs.

The main issues are, when structuredAttrs is enabled:

• $__structuredAttrs is not set
• $outputs is not set
• $outputs defined by NIX_ATTRS_SH_FILE is an associative array
  • This means it is unordered, so we cannot determine the "default" output

As per:

nixpkgs/pkgs/stdenv/generic/default-builder.sh

Lines 1 to 3 in ecd810c

	if [ -e "$NIX_ATTRS_SH_FILE" ]; then . "$NIX_ATTRS_SH_FILE"; elif [ -f .attrs.sh ]; then . .attrs.sh; fi
	source $stdenv/setup

• $outputs not being set is fixed by sourcing $NIX_ATTRS_SH_FILE
• $__structuredAttrs not being set is fixed by sourcing setup.sh
• $outputs being unordered is worked-around by passing in head orig.outputs using replaceVars

Test using:

nix-build -A tests.testers.testBuildFailure

Original Description

The main issue was that when structuredAttrs is enabled, none of the attr-vars get sourced. Therefore accessing any of them results in "unbound variable" errors.

This is fixed by sourcing $NIX_ATTRS_SH_FILE, as-per 8bc5104

nixpkgs/pkgs/stdenv/generic/default-builder.sh

Line 1 in ecd810c

if [ -e "$NIX_ATTRS_SH_FILE" ]; then . "$NIX_ATTRS_SH_FILE"; elif [ -f .attrs.sh ]; then . .attrs.sh; fi

However $__structuredAttrs itself is still unbound, as it is not declared by that file. To work around this, I injected the value using replaceVars.

Once the main issue was resolved, I still had to re-work some implementation details due to differences in the $outputs variable in structured & non-structured attrs.

Without structuredAttrs, $outputs is an array of variable-names, which should be dereferenced to access the actual path. However, with structuredAttrs, $outputs is an associative array:

declare -A outputs=(['out']='/nix/store/2z4mnrzxi1sb4k7zq49gibvvly7gs9rl-example' )

Tested using:

{
  structured = pkgs.testers.testBuildFailure (
    pkgs.runCommandLocal "example" { __structuredAttrs = true; } ''
      exit 1
    ''
  );
  unstructured = pkgs.testers.testBuildFailure (
    pkgs.runCommandLocal "example" { __structuredAttrs = false; } ''
      exit 1
    ''
  );
  unspecified = pkgs.testers.testBuildFailure (
    pkgs.runCommandLocal "example" { } ''
      exit 1
    ''
  );
}

Closes #355953

Also, uncomments a neovim test that was disabled in #352727 due to this bug

cc @wolfgangwalther @teto

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[teto]

ty for the fix

[wolfgangwalther]

I will do a a real review later.

[teto]

I forgot to mention it in my first message but your github ticket has a nice description of the issue, please copy it in the commit message. It helps when running git blame to know why the code is here.

[teto]

I defer the review to the real boss @wolfgangwalther who made such huge contributions to structured attrs (thanks again). Happy with this PR goal.

[wolfgangwalther]

I'd say we should just do 2/3 of what default-builder.sh does at this stage, namely the loading of the attrs.sh file and then sourcing of stdenv/setup. Of course, running genericBuild doesn't make sense.

I assume this could simplify the logic in here quite a bit.

But, we have this comment further up:

# Requirements:
# This runs before, without and after stdenv. Do not modify the environment;
# especially not before invoking the original builder. For example, use
# "@" substitutions instead of PATH.
# Do not export any variables.

I don't quite understand what "before, without and after stdenv" means. Going through the code, I think it would be fine to source stdenv at this stage, i.e. after the original builder ran.

@roberth, you introduced this file and comment - could you give your input here?

[MattSturgeon]

Do not export any variables.

My current impl is definitely violating this. It is at least happening after invoking the original builder.

[MattSturgeon]

I guess we could just encapsulate sourcing the vars in a subshell though

[wolfgangwalther]

The question: What exactly will come after the original builder has already been run and we finish the expect-failure.sh script? Nothing anymore, right? So why would sourcing stdenv/setup hurt at this stage?

[MattSturgeon]

Thanks for the feedback and guidance.

I've pushed this as a fixup commit. I can squash once @roberth has had chance to chime in.

I like the approach now, let's wait for more feedback.

[MattSturgeon]

Ideally, I'd add a regression test for this. But I don't see any existing tests to add to; tests.testers is just an alias for testers...

Looking at the stdenv tests, that has a bunch of stuff I don't currently understand relating to buildPackages and getting an "early" instance of pkgs. Is any of that also required for testing testers?

Or is there a test file somewhere I missed?

[wolfgangwalther]

You could maybe run all tests for replaceVars twice - once with and once without structuredAttrs. They use testBuildFailure, too. And replaceVars is kind of the way forward from substituteAll, which doesn't work nicely with structuredAttrs, so that would be kind of a fit :)

[MattSturgeon]

Ideally, I'd add a regression test for this. But I don't see any existing tests to add to; tests.testers is just an alias for testers...

Or is there a test file somewhere I missed?

Turns out I'm blind. tests.testers is not an alias, but is an actual test file.

I've added some basic tests that piggy-back on existing tests. However the multiOutput test is failing with structuredAttrs...

$ nix log /nix/store/b5x26d73b0nxxlh6anph4xj50f9lpnzi-testBuildFailure-multiOutput.drv
grep: /nix/store/3siwh00k6n76maya58scqizahw1pm31b-fail-structuredAttrs-dev/testBuildFailure.log: No such file or directory

I haven't had chance to debug yet, but my initial thoughts are:
Maybe the default output is not guaranteed to be the first output in the structured associative array?

EDIT: yes, with __structuredAttrs, the $outputs associative-array is unordered, so ${outputs[0]} might not be the "default" output.

EDIT2: I'm wondering if it'd be best to simply write the states&logs to all outputs, instead of attempting to detect the "default"?

EDIT3: reading through the docs, it does seem like the output order is significant. Therefore it seems like an oversight in structuredAttrs that the order is not preserved 🤔

EDIT4: For now, I've solved this by passing in the "default" output name using replaceVars. I'd rather find a neater way, but at least the new tests are passing now.

[wolfgangwalther]

with __structuredAttrs, the $outputs associative-array is unordered, so ${outputs[0]} might not be the "default" output.
[..]
it does seem like the output order is significant. Therefore it seems like an oversight in structuredAttrs that the order is not preserved 🤔
[..]
I've solved this by passing in the "default" output name using replaceVars. I'd rather find a neater way, but at least the new tests are passing now.

So the problem here seems to be that bash associative arrays are hash-ordered and thus the order is not preserved.

attrs.json should have the same information with the right order. Not sure how to best access this, yet. It'd seem possibly useful to have stdenv set a variable to point at the default output, even for structuredAttrs. But I'm not sure whether we can use jq in stdenv, probably not.

But you should be able to use it here, I guess. Extract outputs with jq for structuredAttrs, then take the first. Might not actually end up nicer than what you have right now, though :D

[MattSturgeon]

associative arrays are hash-ordered and thus the order is not preserved.
attrs.json should have the same information with the right order.

Yeah, I thought about that, but figured it'd be better to inject the head-output from the drv-args, rather than inject jq and parse the json.

It'd seem possibly useful to have stdenv set a variable to point at the default output, even for structuredAttrs.

Agreed. This would be the cleanest solution. It seems a bit beyond the scope of this PR, but maybe it'd be worth it...