Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix flake show: don't evaluate derivations for foreign systems by default #6988

Merged
merged 7 commits into from
Jan 30, 2023

Conversation

max-privatevoid
Copy link
Contributor

Simple implementation of the idea, based on how legacyPackages is handled already. Doesn't cover hydraJobs, unfortunately.

Fixes #6985

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea, although it conflicts a bit with the idea that the system hierarchy layer should be special-cased as least as can be. But probably that's worth it.

src/nix/flake.cc Outdated
@@ -957,6 +957,7 @@ struct CmdFlakeArchive : FlakeCommand, MixJSON, MixDryRun
struct CmdFlakeShow : FlakeCommand, MixJSON
{
bool showLegacy = false;
bool showForeign = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on is having showForeign false by default, because it would be bad for backwards-compatibility – esp. for the json output which is designed to be used in scripts and is more dangerous to break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand your sentiments, though having it defaulting to true is a major annoyance for the human.

  1. Evaluation of nix flake show will take much longer than needed
  2. Evaluation of nix flake show will show a lot of redundant information
  3. Evaluation of nix flake show will fail for a majority of flakes

We can avoid all of these 3 problems by defaulting it to false.

At the same time, breaking the interface is fine, as long as we do it now. After 3.0 has been released we can not do it anymore.

Back when 2.4 got released, no one cared that it broke a lot of scripts. Back then it has been pointed out that its at your own risk to use an experimental API. So… If someone complaints, why did they use an experimental API if they do not want things to change?

Also, it is much easier to add --foreign to a handful of scripts, but annoying to have to remember to add it to each and every invocation to shave off minutes of eval time and annoying IFD errors…

I'd even suggest to make this a nix.conf option, such that every user can decide on their very own default in the systems or users nix.conf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely not be a nix.conf option, as tempting as that is, since it's a bad idea for scripting to have options that fundamentally alter what the output of a command is. You can end up with a script that unexpectedly fails on another machine or user account because foreign is set to a different value...

@NobbZ
Copy link
Contributor

NobbZ commented Sep 5, 2022

I like this idea, although it conflicts a bit with the idea that the system hierarchy layer should be special-cased as least as can be. But probably that's worth it.

I think we can easily agree on the fact that the way the system is currently handled has a lot of flaws, and doing special handling it will be the only way around it (besides rewriting "flakespec" from scratch).

@NobbZ
Copy link
Contributor

NobbZ commented Sep 5, 2022

And I also want to add:

Specialcasing the system within nix' source code wasn't necessary that mouch if we were doing checks and shows in nix expression language, as briefly suggested in #6453 and #6454.

Of course I see, that we would still need to tell the nix expressions what the current system is.

@edolstra
Copy link
Member

edolstra commented Sep 6, 2022

--foreign is not very descriptive as an option name. Maybe --all-systems or something like that?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/deduplicating-a-package-definition-between-nixpkgs-and-a-flake/21460/4

@yajo
Copy link
Contributor

yajo commented Sep 22, 2022

Or maybe --systems=all (default value for flag: --systems=local). That would let us also give a specific list of systems to eval (--systems=x86_64-linux,aarch64-linux).

@L-as
Copy link
Member

L-as commented Oct 26, 2022

Is there any reason why we need this instead of just catching errors that happen due to not being able to build a drv and omitting those parts of the output?

@yajo
Copy link
Contributor

yajo commented Oct 26, 2022

For nix flake show, probably it would be the same. However, for nix flake check we would need a way to check the flake only for the current arch. Otherwise an IFS would always become an error, even if it's not.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/announcing-stacklock2nix-easily-build-a-haskell-project-that-contains-a-stack-yaml-lock-file/23563/5

@domenkozar
Copy link
Member

Lost ~hour today due to this unexpected behavor while working on cachix/cachix#476

@adrian-gierakowski
Copy link

What is stopping this fix from being merged?

@thufschmitt
Copy link
Member

What is stopping this fix from being merged?

Nothing really.

@max-privatevoid I've merged master back into this and added a few tests. Let me know whether that looks good to you

@thufschmitt
Copy link
Member

Oh also: I've noticed that this doesn't apply to legacyPackages. Is it intentional?

(I don't think it should be a blocker, and I think it makes sense either way, just wants to make sure it's not an overlook)

@max-privatevoid
Copy link
Contributor Author

Thanks for writing the tests. I've added the functionality to legacyPackages as well now. I definitely meant to have the flag work there too, just forgot about it.

Don't hardcode “x86_64-linux” as this won't work too nicely on other
platforms
@thufschmitt
Copy link
Member

Awesome, thanks. I've just pushed a fix for the tests because they were broken on non-x86_64-linux. If the CI is OK, it'll just merge

Things leading to another...
@thufschmitt thufschmitt merged commit 575d0ae into NixOS:master Jan 30, 2023
peterbecich added a commit to peterbecich/nix that referenced this pull request Feb 5, 2023
`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
@peterbecich
Copy link
Contributor

Thanks for this PR, I am trying to use it to implement the same behavior for nix flake check: #7759

peterbecich added a commit to peterbecich/nix that referenced this pull request Feb 9, 2023
`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
peterbecich added a commit to peterbecich/nix that referenced this pull request Feb 11, 2023
`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
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-2-14-0-released/25900/2

peterbecich added a commit to peterbecich/nix that referenced this pull request Mar 11, 2023
`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
peterbecich added a commit to peterbecich/nix that referenced this pull request Mar 26, 2023
`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
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

peterbecich added a commit to peterbecich/nix that referenced this pull request Apr 29, 2023
`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
peterbecich added a commit to peterbecich/nix that referenced this pull request May 22, 2023
`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
thufschmitt pushed a commit that referenced this pull request May 23, 2023
`nix flake show` now skips derivations for foreign systems: #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" #4265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

nix flake show should be filterable by the current system
10 participants