testers.testBuildFailure: fix structuredAttrs support by MattSturgeon · Pull Request #371337 · NixOS/nixpkgs

MattSturgeon · 2025-01-06T03:52:41Z

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

Add a 👍 reaction to pull requests you find important.

teto

ty for the fix

wolfgangwalther

I will do a a real review later.

pkgs/build-support/testers/default.nix

teto · 2025-01-06T09:26:53Z

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 · 2025-01-06T11:05:11Z

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

pkgs/build-support/testers/expect-failure.sh

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

MattSturgeon · 2025-01-06T19:23:54Z

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 · 2025-01-06T19:29:02Z

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 · 2025-01-06T21:37:20Z

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 · 2025-01-07T19:04:28Z

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 · 2025-01-07T19:23:32Z

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...
EDIT: I see this would actually require changes in nix (c++)

pkgs/build-support/testers/expect-failure.sh

wolfgangwalther

Just a nit, very nice approach now!

pkgs/build-support/testers/expect-failure.sh

Fixes `testers.testBuildFailure` compatibility with `__structuredAttrs`. The two main issues are, when `structuredAttrs` is enabled: - `$__structuredAttrs` is not set - `$outputs` is not set `$__structuredAttrs` not being set is fixed by checking for `$NIX_ATTRS_SH_FILE`, as per pkgs/stdenv/generic/setup.sh `$outputs` not being set is fixed by sourcing `$NIX_ATTRS_SH_FILE`, as-per pkgs/stdenv/generic/default-builder.sh

If `$stdenv/setup` is sourced by `expect-failure.sh`, then the original builder's hooks could be re-run, potentially leading to unintended side-effects.

MattSturgeon · 2025-01-08T22:51:36Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 371337

`x86_64-linux`

✅ 25 packages built:

tests.dotnet.structured-attrs.no-structured-attrs
tests.dotnet.use-dotnet-from-env.without-fallback
tests.replaceVars.replaceVars-fails-in-build-phase
tests.replaceVars.replaceVars-fails-in-check-phase
tests.replaceVars.replaceVars-fails-in-check-phase-with-exemption
tests.replaceVars.replaceVars-fails-on-directory
tests.testers.hasPkgConfigModules.miniz-versions-mismatch
tests.testers.hasPkgConfigModules.zlib-does-not-have-ylib
tests.testers.lycheeLinkCheck.fail
tests.testers.lycheeLinkCheck.fail-emptyDirectory
tests.testers.shellcheck.example-dir
tests.testers.shellcheck.example-file
tests.testers.testBuildFailure.happy
tests.testers.testBuildFailure.happyStructuredAttrs
tests.testers.testBuildFailure.helloDoesNotFail
tests.testers.testBuildFailure.multiOutput
tests.testers.testBuildFailure.multiOutputStructuredAttrs
tests.testers.testBuildFailure.sideEffectStructuredAttrs
tests.testers.testBuildFailure.sideEffects
tests.testers.testEqualContents.emptyFileAndDir
tests.testers.testEqualContents.fileDiff
tests.testers.testEqualContents.fileMissing
tests.testers.testEqualContents.nonExistentPath
tests.testers.testEqualContents.unequalExe
tests.testers.testEqualContents.unequalExeInDir

github-actions bot added 6.topic: vim Advanced text editor 6.topic: testing Tooling for automated testing of packages and modules labels Jan 6, 2025

MattSturgeon requested review from khaneliman, teto and wolfgangwalther January 6, 2025 03:53

nix-owners bot requested review from GaetanLepage, PerchunPak, figsoda, manveru, mrcjkb, philiptaron and rvolosatovs January 6, 2025 03:54

MattSturgeon mentioned this pull request Jan 6, 2025

expect-failure.sh: fix variabl existence check #355953

Closed

13 tasks

github-actions bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 6, 2025

teto approved these changes Jan 6, 2025

View reviewed changes

wolfgangwalther previously requested changes Jan 6, 2025

View reviewed changes

pkgs/build-support/testers/default.nix Outdated Show resolved Hide resolved

MattSturgeon force-pushed the fix-testFailure-structuredAttrs branch 2 times, most recently from 6852a9a to ef30e6d Compare January 6, 2025 10:22

MattSturgeon requested review from teto and wolfgangwalther January 6, 2025 10:29

MattSturgeon force-pushed the fix-testFailure-structuredAttrs branch from ef30e6d to ac4958b Compare January 6, 2025 11:10

MattSturgeon commented Jan 6, 2025

View reviewed changes

pkgs/build-support/testers/expect-failure.sh Outdated Show resolved Hide resolved

pkgs/build-support/testers/expect-failure.sh Outdated Show resolved Hide resolved

wolfgangwalther reviewed Jan 6, 2025

View reviewed changes

pkgs/build-support/testers/expect-failure.sh Outdated Show resolved Hide resolved

wolfgangwalther mentioned this pull request Jan 6, 2025

[Tracking Issue] __structuredAttrs TODOs and FIXMEs #205690

Open

4 tasks

roberth reviewed Jan 8, 2025

View reviewed changes

pkgs/build-support/testers/expect-failure.sh Outdated Show resolved Hide resolved

MattSturgeon force-pushed the fix-testFailure-structuredAttrs branch 2 times, most recently from d493a97 to 03e40ab Compare January 8, 2025 18:22

MattSturgeon requested review from roberth and wolfgangwalther January 8, 2025 18:28

wolfgangwalther approved these changes Jan 8, 2025

View reviewed changes

pkgs/build-support/testers/expect-failure.sh Outdated Show resolved Hide resolved

MattSturgeon added 4 commits January 8, 2025 19:19

tests.testers.testBuildFailure: assert hooks have no side-effects

fea1744

If `$stdenv/setup` is sourced by `expect-failure.sh`, then the original builder's hooks could be re-run, potentially leading to unintended side-effects.

tests.testers.testBuildFailure: test with structuredAttrs

68be5bb

neovim.tests: enable failing_check

e652e9c

MattSturgeon force-pushed the fix-testFailure-structuredAttrs branch from 03e40ab to e652e9c Compare January 8, 2025 19:20

MattSturgeon added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 8, 2025

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 8, 2025

wolfgangwalther merged commit 38a0a52 into NixOS:master Jan 10, 2025
30 checks passed

MattSturgeon deleted the fix-testFailure-structuredAttrs branch January 10, 2025 21:08

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

	source $stdenv/setup

Uh oh!

Conversation

MattSturgeon commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test using:

Things done

Uh oh!

teto left a comment

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teto commented Jan 6, 2025

Uh oh!

teto commented Jan 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MattSturgeon commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jan 6, 2025

Uh oh!

MattSturgeon commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jan 7, 2025

Uh oh!

MattSturgeon commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MattSturgeon commented Jan 8, 2025

nixpkgs-review result

x86_64-linux

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MattSturgeon commented Jan 6, 2025 •

edited

Loading

MattSturgeon commented Jan 6, 2025 •

edited

Loading

MattSturgeon commented Jan 6, 2025 •

edited

Loading

MattSturgeon commented Jan 7, 2025 •

edited

Loading

`nixpkgs-review` result

`x86_64-linux`