`nix flake check`: skip derivations for foreign systems #7759

peterbecich · 2023-02-05T23:54:46Z

Context

nix flake check breaks on IFD in multi-platform flake
#4265

This PR changes the default behavior of nix flake check.

The same change was already approved for nix flake show: #6988

Motivation

nix flake show now skips derivations for foreign systems: #6988

This PR borrows from that PR to implement the same behavior for nix flake check.

Here is a Flake project which I believe uses IFD: https://github.com/srid/haskell-template

Using Nix 2.12.0 on a x86_64-linux, the result of nix flake check is:

$ nix flake check
warning: Git tree '/home/peterbecich/haskell/libraries/haskell-template' is dirty
error: a 'aarch64-linux' with features {} is required to build 
'/nix/store/jlcg48nsm1dpblvg9wj6zq19c3mswg40-cabal2nix-haskell-template.drv',
 but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test, uid-range}
(use '--show-trace' to show detailed location information)

With this PR, nix flake check produces:

~/nix/nix/outputs/out/bin/nix flake check
warning: Git tree '/home/peterbecich/haskell/libraries/haskell-template' is dirty
warning: The check omitted these incompatible systems (use '--all-systems' to check all):
warning: aarch64-darwin
warning: aarch64-linux
warning: armv5tel-linux
warning: armv6l-linux
warning: armv7l-linux
warning: i686-linux
warning: mipsel-linux
warning: powerpc64le-linux
warning: riscv64-linux
warning: x86_64-darwin

With this PR and the --all-systems flag, it produces an error like before:

 ~/nix/nix/outputs/out/bin/nix flake check --all-systems
warning: Git tree '/home/peterbecich/haskell/libraries/haskell-template' is dirty
error:
       … while checking flake output 'packages'

         at «none»:0: (source not available)

       … while checking the derivation 'packages.aarch64-linux.default'

         at «none»:0: (source not available)

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: a 'aarch64-linux' with features {} is required to build 
'/nix/store/jlcg48nsm1dpblvg9wj6zq19c3mswg40-cabal2nix-haskell-template.drv',
 but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test, uid-range}

The current system is acquired this way, copied from #6988:

nix/src/nix/flake.cc

Line 300 in 513d8a1

auto localSystem = std::string(settings.thisSystem.get());

For a Nix system with remote builders, could this change to the default behavior of nix flake check be a problem?

This PR copies the existing check checkSystemName.
Wherever checkSystemName is applied in nix flake check, the system type is now also checked by checkSystemType. I have not thought very carefully about where these checks are placed. Example:

nix/src/nix/flake.cc

Lines 528 to 542 in 8be71bc

    
           checkSystemName(attr_name, attr.pos); 
        
           if (checkSystemType(attr_name, attr.pos)) { 
        
               state->forceAttrs(*attr.value, attr.pos, ""); 
        
               for (auto & attr2 : *attr.value->attrs) { 
        
                   auto drvPath = checkDerivation( 
        
                       fmt("%s.%s.%s", name, attr_name, state->symbols[attr2.name]), 
        
                       *attr2.value, attr2.pos); 
        
                   if (drvPath && attr_name == settings.thisSystem.get()) { 
        
                       drvPaths.push_back(DerivedPath::Built { 
        
                               .drvPath = *drvPath, 
        
                                   .outputs = OutputsSpec::All { }, 
        
                                   }); 
        
                   } 
        
               } 
        
           }

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

yajo · 2023-02-06T09:51:21Z

If you want better backwards compatibility, you can do this opt-in and instead add the flag --current-system-only or --skip-foreign-systems or similar.

peterbecich · 2023-02-07T04:27:30Z

That's a good idea, I don't have an opinion one way or the other.

Can somebody make a decision:

check all systems by default; --current-system-only flag or similar
- optionally, we could consider altering nix flake show to be consistent with this

or

check only current system by default; --all-systems flag
- consistent with nix flake show --all-systems

I assume the nix flake show fix will be included in the 2.14 release. It would be cool if this PR were also included in 2.14.

NobbZ · 2023-02-07T07:48:37Z

Personally I am in the "change the default behaviour" camp and only check the current system.

This usually makes it easier to run a check on the terminal.

Keeping the current behaviour as the default would usually end up in a workflow like the following:

run the check,
see it fail because of IFD
be confused
google
try check with --current-system-only (or whatever name would be agreed upon)
see an error that the flag wasn't known as nix is too old

Optionally replace 6 with "get a suceeding check".

By changing the default, we can get this down to:

run the check
see it pass/fail

In worst case people had an older nix and they would google and should see, oh, new behaviour introduced in nix 2.30, I'm only on 2.15, so I have to update.

CI that indeed relies on multi-arch checks would need to update their scripts, as they update their nix version. I think this is fine. At least they are relying on experimental software which is allowed to break or change its behaviour in subtle ways.

We just need to be clear in the release notes that this is intended behaviour.

Break things now!

Once 3.0 got released we can not as easily!

PS: I am a big fan of --system … to be able to override the current system to play with aarch64 and i686 builds on my x86_64 box, so that should definitely be respected as "current system"

peterbecich · 2023-02-08T07:58:30Z

Okay, I will keep the current behavior then, default is current system only.

PS: I am a big fan of --system … to be able to override the current system to play with aarch64 and i686 builds on my x86_64 box, so that should definitely be respected as "current system"

This is a good idea; can we review this PR now and implement that later in a subsequent PR? It can be done for both nix flake show and nix flake check. I don't know when the date is for the next release; hoping to merge this PR before.

nixos-discourse · 2023-02-20T14:58:40Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-17-nix-team-meeting-minutes-33/25624/1

nixos-discourse · 2023-03-11T02:49:04Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-17-nix-team-meeting-minutes-33/25624/2

fricklerhandwerk · 2023-03-22T20:11:29Z

Discussed in the Nix team meeting 2023-03-17:

@peterbecich to answer your question from Discourse on behalf of the team (although I was not there, just inferring from the meeting notes): Yes, default behavior should stay the same. We could either have a --keep-going flag to delay the error, or alternatively a --current-system-only flag like suggested in #7759 (comment). Just have to make it consistent with --all-systems in nix flake show.

Currently nix flake check works fine without IFD, but breaks on most IFD flakes (because evaluates the outputs for all systems, which will break if there's no builder for all the systems).

There was no clear decision on the priority and how to proceed though, and I suppose we will talk about it again some time soon, and post an update again.

nixos-discourse · 2023-03-22T20:18:37Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-17-nix-team-meeting-minutes-41/26614/1

peterbecich · 2023-03-26T19:27:04Z

@fricklerhandwerk okay, thank you. I will keep the default behavior and implement --current-system-only.

IMO --current-system-only is better than --keep-going with deferred error.

This will help nix flake check --current-system-only in GitHub Actions. If there is a deferred error code thrown by nix flake check --keep-going, the GitHub Action will fail.

Just have to make it consistent with --all-systems in nix flake show.

To make it consistent with nix flake show, it needs to change the default behavior.

If I'm not mistaken:

default behavior of nix flake show is current system only
default behavior of nix flake check is all systems

The choice to change the default behavior got 9 upvotes: #7759 (comment)

I have no opinion.

peterbecich · 2023-03-26T20:29:03Z

Here are tests;

default behavior maintained:

$ ./result/bin/nix flake check ~/haskell/libraries/purescript-bridge2                                                                                                1 
...
error: a 'aarch64-linux' with features {} is required to build '/nix/store/d0w4jfn7ldnr7j1xipfzp3rmk7mwwf1j-cabal2nix-example.drv', 
but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test, uid-range}

The --current-system-only flag:

$ ./result/bin/nix flake check --current-system-only ~/haskell/libraries/purescript-bridge2                                                                          warning: unknown flake output 'haskellFlakeProjectModules'
warning: The check omitted these incompatible systems:
warning: aarch64-darwin
warning: aarch64-linux
warning: armv5tel-linux
warning: armv6l-linux
warning: armv7l-linux
warning: i686-linux
warning: mipsel-linux
warning: powerpc64le-linux
warning: riscv64-linux
warning: x86_64-darwin

nixos-discourse · 2023-03-26T20:31:47Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-17-nix-team-meeting-minutes-41/26614/3

fricklerhandwerk · 2023-03-27T12:14:14Z

Discussed in Nix team meeting:

@peterbecich we talked about this again and agree on changing the default behavior, and making --all-systems work exactly as in nix flake show. Thanks for your patience!

@thufschmitt will shepherd the PR to the finish line.

nixos-discourse · 2023-03-27T13:44:50Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-27-nix-team-meeting-minutes-44/26759/1

peterbecich · 2023-04-06T07:35:03Z

Great, thank you. ~~This last commit d00082f should be deleted.~~ I have reverted that commit.

peterbecich · 2023-04-29T01:36:10Z

The PR implements the behavior everyone has agreed to: #7759 (comment)

peterbecich · 2023-05-19T08:26:47Z

Can this be merged? Thank you

thufschmitt

Sorry for the delay @peterbecich, this fell down the cracks. Just one small comment on the message that gets printed, but good otherwise.

src/nix/flake.cc

`nix flake show` now skips derivations for foreign systems: NixOS#6988 This commit borrows from that to implement the same behavior for `nix flake check`. See "nix flake check breaks on IFD in multi-platform flake" NixOS#4265

Flake with two systems intentionally fails with `nix flake check --all-systems`; the same flake succeeds with `nix flake check`. Previously, this flake intentionally failed with `nix flake check`.

…ystem-only` flag

…urrent-system-only` flag" This reverts commit d00082f.

Co-authored-by: Théophane Hufschmitt <[email protected]>

peterbecich · 2023-05-22T18:13:15Z

@thufschmitt , thank you, I have checked and made a small fix to your commit, looks good to me.

$ nix flake check ~/haskell/libraries/hackage-server
warning: unknown flake output 'haskellFlakeProjectModules'
trace: WARNING[haskell-flake]: Multiple haskell overlays are applied in arbitrary order.
warning: The check omitted these incompatible systems: aarch64-darwin, x86_64-darwin
Use '--all-systems' to check all.

thufschmitt · 2023-05-23T04:59:06Z

Awesome, thanks

peterbecich · 2023-05-27T22:34:57Z

This PR did not fix nix flake check for multi-platform Flakes using IFD: #4265 (comment)

peterbecich marked this pull request as ready for review February 7, 2023 04:22

peterbecich requested review from edolstra and thufschmitt as code owners February 7, 2023 04:22

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch 2 times, most recently from ab46217 to b2e5a8a Compare February 11, 2023 07:38

adrian-gierakowski mentioned this pull request Feb 15, 2023

Does not work on platforms aside from x86_64-linux hall/kubenix#12

Closed

fricklerhandwerk changed the title ~~nix flake check skips derivations for foreign systems~~ nix flake check: skip derivations for foreign systems Feb 17, 2023

thufschmitt added the new-cli Relating to the "nix" command label Feb 27, 2023

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from b2e5a8a to 15d7393 Compare March 11, 2023 02:53

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from 15d7393 to 581084d Compare March 26, 2023 19:28

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Mar 26, 2023

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from 1e63fe3 to d00082f Compare March 26, 2023 20:19

thufschmitt self-assigned this Mar 27, 2023

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from d00082f to df8133f Compare April 29, 2023 01:33

thufschmitt suggested changes May 22, 2023

View reviewed changes

src/nix/flake.cc Outdated Show resolved Hide resolved

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from c3ebe84 to 96470b3 Compare May 22, 2023 17:23

peterbecich added 4 commits May 22, 2023 10:43

nix flake check skips derivations for foreign systems

5a678fc

`nix flake show` now skips derivations for foreign systems: NixOS#6988 This commit borrows from that to implement the same behavior for `nix flake check`. See "nix flake check breaks on IFD in multi-platform flake" NixOS#4265

fix functional tests for nix flake check --all-systems

8e2c281

Flake with two systems intentionally fails with `nix flake check --all-systems`; the same flake succeeds with `nix flake check`. Previously, this flake intentionally failed with `nix flake check`.

default behavior of nix flake check unchanged; provide `--current-s…

03782ef

…ystem-only` flag

Revert "default behavior of nix flake check unchanged; provide `--c…

25e4864

…urrent-system-only` flag" This reverts commit d00082f.

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from 96470b3 to a395e48 Compare May 22, 2023 17:43

Update src/nix/flake.cc

567cb9b

Co-authored-by: Théophane Hufschmitt <[email protected]>

peterbecich force-pushed the skip-nix-flake-check-for-incompatible-systems branch from a395e48 to 567cb9b Compare May 22, 2023 18:11

peterbecich requested a review from thufschmitt May 22, 2023 21:11

thufschmitt approved these changes May 23, 2023

View reviewed changes

thufschmitt merged commit a420ccc into NixOS:master May 23, 2023

peterbecich deleted the skip-nix-flake-check-for-incompatible-systems branch May 27, 2023 20:10

cleeyv mentioned this pull request Jul 28, 2023

CI builds for darwin ngi-nix/ngipkgs#24

Closed

This was referenced Aug 22, 2023

still necessary with Nix 2.16+? srid/check-flake#1

Open

behavior of show and check changed in Nix recently nix-systems/nix-systems#9

Open

henrirosten mentioned this pull request Nov 2, 2023

Nix flake check fails on Ghaf mainline tiiuae/ghaf#296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`nix flake check`: skip derivations for foreign systems #7759

`nix flake check`: skip derivations for foreign systems #7759

peterbecich commented Feb 5, 2023 •

edited

Loading

yajo commented Feb 6, 2023

peterbecich commented Feb 7, 2023 •

edited

Loading

NobbZ commented Feb 7, 2023 •

edited

Loading

peterbecich commented Feb 8, 2023

nixos-discourse commented Feb 20, 2023

nixos-discourse commented Mar 11, 2023

fricklerhandwerk commented Mar 22, 2023 •

edited

Loading

nixos-discourse commented Mar 22, 2023

peterbecich commented Mar 26, 2023 •

edited

Loading

peterbecich commented Mar 26, 2023

nixos-discourse commented Mar 26, 2023

fricklerhandwerk commented Mar 27, 2023 •

edited

Loading

nixos-discourse commented Mar 27, 2023

peterbecich commented Apr 6, 2023 •

edited

Loading

peterbecich commented Apr 29, 2023 •

edited

Loading

peterbecich commented May 19, 2023

thufschmitt left a comment

peterbecich commented May 22, 2023 •

edited

Loading

thufschmitt commented May 23, 2023

peterbecich commented May 27, 2023 •

edited

Loading

	checkSystemName(attr_name, attr.pos);
	if (checkSystemType(attr_name, attr.pos)) {
	state->forceAttrs(*attr.value, attr.pos, "");
	for (auto & attr2 : *attr.value->attrs) {
	auto drvPath = checkDerivation(
	fmt("%s.%s.%s", name, attr_name, state->symbols[attr2.name]),
	*attr2.value, attr2.pos);
	if (drvPath && attr_name == settings.thisSystem.get()) {
	drvPaths.push_back(DerivedPath::Built {
	.drvPath = *drvPath,
	.outputs = OutputsSpec::All { },
	});
	}
	}
	}

nix flake check: skip derivations for foreign systems #7759

nix flake check: skip derivations for foreign systems #7759

Conversation

peterbecich commented Feb 5, 2023 • edited Loading

Context

Motivation

Checklist for maintainers

yajo commented Feb 6, 2023

peterbecich commented Feb 7, 2023 • edited Loading

NobbZ commented Feb 7, 2023 • edited Loading

peterbecich commented Feb 8, 2023

nixos-discourse commented Feb 20, 2023

nixos-discourse commented Mar 11, 2023

fricklerhandwerk commented Mar 22, 2023 • edited Loading

nixos-discourse commented Mar 22, 2023

peterbecich commented Mar 26, 2023 • edited Loading

peterbecich commented Mar 26, 2023

nixos-discourse commented Mar 26, 2023

fricklerhandwerk commented Mar 27, 2023 • edited Loading

nixos-discourse commented Mar 27, 2023

peterbecich commented Apr 6, 2023 • edited Loading

peterbecich commented Apr 29, 2023 • edited Loading

peterbecich commented May 19, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

peterbecich commented May 22, 2023 • edited Loading

thufschmitt commented May 23, 2023

peterbecich commented May 27, 2023 • edited Loading

`nix flake check`: skip derivations for foreign systems #7759

`nix flake check`: skip derivations for foreign systems #7759

peterbecich commented Feb 5, 2023 •

edited

Loading

peterbecich commented Feb 7, 2023 •

edited

Loading

NobbZ commented Feb 7, 2023 •

edited

Loading

fricklerhandwerk commented Mar 22, 2023 •

edited

Loading

peterbecich commented Mar 26, 2023 •

edited

Loading

fricklerhandwerk commented Mar 27, 2023 •

edited

Loading

peterbecich commented Apr 6, 2023 •

edited

Loading

peterbecich commented Apr 29, 2023 •

edited

Loading

peterbecich commented May 22, 2023 •

edited

Loading

peterbecich commented May 27, 2023 •

edited

Loading