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

flake.nix expose forAllSystems #208020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

flake.nix expose forAllSystems #208020

wants to merge 1 commit into from

Conversation

domenkozar
Copy link
Member

@domenkozar domenkozar commented Dec 27, 2022

Description of changes

This allows flake users to use nixpkgs.lib.forAllSystems

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@domenkozar domenkozar requested a review from roberth December 27, 2022 21:01
This allows flake users to use nixpkgs.lib.forAllSystems
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 27, 2022
@drupol
Copy link
Contributor

drupol commented Dec 27, 2022

I guess that once this is in, it will make the project https://github.com/numtide/flake-utils obsolete?

Copy link
Member

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

the same function is also used in release-lib.nix, I think we can just move it to lib to reduce duplicate code

@domenkozar
Copy link
Member Author

the same function is also used in release-lib.nix, I think we can just move it to lib to reduce duplicate code

That one has a different set of systems if I read the code correctly.

@roberth
Copy link
Member

roberth commented Dec 28, 2022

Since systems.flakeExposed is arbitrary and the remaining logic is lib.genAttrs, I don't think combining the two into a single function is the right thing to do. The configuration and the generic logic shouldn't be coupled like that.

Other people's objections also still apply, see:

Please use genAttrs systems.flakeExposed instead of forAllSystems. I'm sorry.

@roberth roberth closed this Dec 28, 2022
@roberth
Copy link
Member

roberth commented Dec 28, 2022

To everyone who doesn't care about the set of systems, I 100% agree, but that's a problem to be fixed in Nix itself, not in Nixpkgs. The flakes schema requires us to think about the set of systems. If you disagree with this design choice, help out with https://github.com/NixOS/nix instead of papering over it with hacks like this.

@domenkozar
Copy link
Member Author

Since systems.flakeExposed is arbitrary

Can you explain why it's arbitrary? It's based on the RFC how we define tiers for nixpkgs support.

I don't think combining the two into a single function is the right thing to do.

But that's literally what nixpkgs does and anything that consumes nixpkgs will at most suppport this kind of systems. Why is it OK to be used in nixpkgs but not the consumers of nixpkgs?

Please use genAttrs systems.flakeExposed instead of forAllSystems. I'm sorry.

So all flakes out there are using this composition and we should instruct them to use it, but we should forbid making it one function?

@domenkozar
Copy link
Member Author

domenkozar commented Dec 28, 2022

I'll phrase it differently, for someone that comes newly to Nix and wants to write a flake, what logic do we recommend them to use for systems? And hopefully the answer is not a PhD thesis paper.

@roberth
Copy link
Member

roberth commented Dec 28, 2022

Can you explain why it's arbitrary?

iirc @edolstra's stance on the system for which to add attrs, is just the explicitly supported (by reason or by tests?) systems, to be determined by the flake author. This should therefore usually be a smaller set.
If we choose to list more than explicitly supported systems, many packages may even work on a larger set, provided that the flake instantiates their own nixpkgs via the function instead of legacyPackages.

I don't think combining the two into a single function is the right thing to do.

But that's literally what nixpkgs does

Other flakes aren't Nixpkgs and are supposed to consider for themselves what to support. By letting the flake author write their own let binding for this, they are set up for the future, where they likely will diverge from Nixpkgs' choice, if that's even a good first choice in the first place..

I'll phrase it differently, for someone that comes newly to Nix and wants to write a flake, what logic do we recommend them to use for systems? And hopefully the answer is not a PhD thesis paper.

Assuming they don't use any framework like flake-utils, flake-parts, etc, and if I understand Eelco's design correctly, it should be

{
  # ...
  outputs = { nixpkgs, ... }:
  let inherit (nixpkgs) lib;
      supportedSystems = [ "x86_64-linux" ];
      forSystems = lib.genAttrs supportedSystems;
  in {
    # ...
  };
}

As for my own opinion, I agree that supportedSystems isn't something most flake authors should have to think about at all. The current flake design requires authors to, but the solution is not to hide it away (and still require it) but to split the outputs along the proven roles of what used to be default.nix vs release.nix. This has been discussed extensively in the nix repo, but little action has been taken. I hope the nix team can make decide a direction, but as far as I'm aware, we don't have a lot of resources for feature development. We're still recovering from a period of rather haphazard development from before the team was formed (and I don't mean that in a judging way; I'm confident that all contributors and maintainers have done their best given their constraints).
I'll see what I can do in the team.

@domenkozar
Copy link
Member Author

domenkozar commented Dec 29, 2022

Whole nixpkgs works on the principle of having attributes for all platforms and builds failing if it a derivation doesn't build.

So the whole idea that lack of an attribute means platform isn't support won't work since the user will learn quickly it's a broken promise.

But flakes makes system in the outputs turns a build error that contains relevant information what went wrong into

error: attribute 'armv6l-linux' missing

Creating a door for the user to be confused about: Did I forget something? Did I mistype something?

Now they need to ask all flakes they depend on to specify that platforms so that you can even try to see if it builds.

All this complexity costs for what exactly?

@domenkozar
Copy link
Member Author

Here's exactly what happens when a new user is faced with this, to back up my claims: https://discord.com/channels/1036369714731036712/1036369758024650792/1064497129030176799

@roberth
Copy link
Member

roberth commented Jan 16, 2023

@domenkozar I have 0 doubt about your claims. I only disagree about the particular solution.
The link seems to point to a private channel. Could you quote it here?

We'll be discussing the system handling in the flake schema today in the Nix team. I wish we could have done that sooner, but it got delayed because of the holidays.

@domenkozar
Copy link
Member Author

2023-01-16-114630_1714x523_escrotum

It's not a private channel, but probably you need to join the server to see it.

@infinisil infinisil mentioned this pull request Jan 16, 2023
13 tasks
@roberth
Copy link
Member

roberth commented Jan 16, 2023

@domenkozar A forAllSystems function does not solve their problem, which appears to be a misunderstanding of the schema of the phps flake, or a misunderstanding of the schema suggested by the Nix CLI, or a guess informed by some example somewhere that's "unintuitive" and therefore not translatable to the use case at hand. It does illustrate that we have a general problem with system handling, where "plain" flakes are bad examples. The normal thing, to operate with a context that has an implied system, should be easy. No introductory tutorial should have to deal with system more than once (or not at all).

@roberth
Copy link
Member

roberth commented Jan 16, 2023

The Promised Update

I'll see what I can do in the team.

The team has not made a decision today to make the pragmatic choice to implement NixOS/nix#6773 or a similar change in the short term. However, the good news is that we've had a productive and positive discussion about the stabilization of these experimental features, which makes me hopeful about the future of these features, albeit in the long term.

This also means that I have to make good on my implicit promise. (I guess I was hesitant to spell it out)

So the answer is that system handling won't change in the short term. I also see the reality that many people have committed to flakes as if they were stable. It would have been unfortunate to have people change their flakes shortly before Nix itself would vastly improve the situation, but that won't be the case for probably multiple months.

Things being what they are, I'll go ahead and allow incremental improvements to happen. We'll have a function that will be made obsolete. So be it, if it helps users today.

So...

I'm reopening this PR to allow a hopefully temporary workaround to be added to the code that's already part of an experimental feature. Do not be surprised when it is deprecated. Or be positively surprised by the alternative.

🎉

@roberth roberth reopened this Jan 16, 2023
@domenkozar
Copy link
Member Author

@domenkozar A forAllSystems function does not solve their problem, which appears to be a misunderstanding of the schema of the phps flake, or a misunderstanding of the schema suggested by the Nix CLI, or a guess informed by some example somewhere that's "unintuitive" and therefore not translatable to the use case at hand. It does illustrate that we have a general problem with system handling, where "plain" flakes are bad examples. The normal thing, to operate with a context that has an implied system, should be easy. No introductory tutorial should have to deal with system more than once (or not at all).

It does not, but it proves the point if that was indeed a system attribute not supported by a flake.

I'm happy to hear this is moving further 🥳

Thanks for the hard work!

@roberth
Copy link
Member

roberth commented Jan 16, 2023

I think this is close to done. An item in the release notes would be appropriate.

@fricklerhandwerk do you still think experimental features should not be documented in the manuals? Perhaps we could have a section or at least something? I feel like we're wasting a lot of potential by postponing it until the very end of the stabilization process.

@fricklerhandwerk
Copy link
Contributor

I never actually thought we shouldn't, and just followed what I perceived as an existing convention, agreeing with what @edolstra essentially argued today that using experimental features in examples will lead to confusion and churn.

By all means we should have a section that acknowledges experimental features and the extent to which Nixpkgs supports them. If there is much more than that, we should possibly set up a convention to mark functions as relying on experimental features.

@SuperSandro2000 SuperSandro2000 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 23, 2023
@gytis-ivaskevicius
Copy link
Contributor

gytis-ivaskevicius commented Feb 6, 2023

Just my 2 nix'es:
I believe that we should not encourage people to depend on nixpkgs and this should be resolved in nix itself.

In the future, once flakes are stabilized and are feature complete I'd expect to see simple flakes look something like this: https://github.com/gytis-ivaskevicius/nix-patterns/tree/master/07-blueprints-no-nixpkgs

This has several advantages:

  • You have an option to depend on developer-provided nixpkgs if you feel like it
  • Not to depend on nixpkgs and be sure of it (no nixpkgs input - no chances of shooting yourself in a foot with multiple nixpkgs imports. Otherwise, you don't know what the developer is doing, he might import nixpkgs for you which would mean secondary nixpkgs evaluation no matter if you overwrite nixpkgs or not)
  • Not to depend on developer-provided <system>'s
  • No longer a need to use drv.override {}
  • No more developers forgetting to use pkgs.callPackage
  • No more overlays - unless you feel like it ;)

@aakropotkin
Copy link
Contributor

Just my 2 nix'es:
I believe that we should not encourage people to depend on nixpkgs and this should be resolved in nix itself.

In the future, once flakes are stabilized and are feature complete I'd expect to see simple flakes look something like this: https://github.com/gytis-ivaskevicius/nix-patterns/tree/master/07-blueprints-no-nixpkgs

This has several advantages:

  • You have an option to depend on developer-provided nixpkgs if you feel like it
  • Not to depend on nixpkgs and be sure of it (no nixpkgs input - no chances of shooting yourself in a foot with multiple nixpkgs imports. Otherwise, you don't know what the developer is doing, he might import nixpkgs for you which would mean secondary nixpkgs evaluation no matter if you overwrite nixpkgs or not)
  • Not to depend on developer-provided <system>'s
  • No longer a need to use drv.override {}
  • No more developers forgetting to use pkgs.callPackage
  • No more overlays - unless you feel like it ;)

This sounds like a problem that's covered by overlays outputs in flakes.

I understand the wart, and agree that forcing deep follows is annoying ( you basically have to use a modified callFlake ).

What I'm not really following is how avoiding a Nixpkgs input resolves these issues. Unless you completely avoided stdenv and bootstrap a set of flakes from derivation, you're still going to end up referring to it with something like fetchurl.

Maybe I'm misunderstanding your pitch though.

@gytis-ivaskevicius
Copy link
Contributor

This sounds like a problem that's covered by overlays outputs in flakes.

You are not wrong, but here are the main points:

  • Blueprints remove a need for .overrides
  • Blueprints can be very easily turned into overlays or packages with simple mapAttrs
  • I've seen several instances of overlays importing nixpkgs. Blueprints are foolproof in this regard
  • I've seen a lot of instances of callPackage not being used. Blueprints are foolproof in this regard
  • Overlays are disliked by a large chunk of the community due to the billion nixpkgs issue (I assume that's why?)

@roberth
Copy link
Member

roberth commented Feb 6, 2023

Blueprints are ok, but they're only one solution in a spectrum of ways you can work with dependencies.

  • blueprints: everything injectable; some wiring not reused
  • function from nixpkgs to attribute set of packages: adopt the upstream wiring, but avoid an extra nixpkgs
  • flake packages attribute, and no follows: use the package exactly as tested by upstream, incur the extra nixpkgs

Each of these is a valid way of presenting and consuming packages, but none of them have a finalized design or cli integration

  • blueprints: better to be simplified into modules (drv-parts?)
  • function from nixpkgs to attribute set of packages: a possible generalization of the flake schema
  • even if we don't apply such a generalization, the need for enumeration is a problem, as is the inability to hint/suggest/require follows as needed, and an important potential benefit hasn't been realised yet: inter-flake evaluation cache (as opposed to cli <-> evaluator boundary level cache)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2022-01-05-documentation-team-meeting-notes-22/25535/1

@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-systems-externally-extensible-flake-systems/27110/8

@AndersonTorres
Copy link
Member

Is there anything worth looking? I am very confused.

@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 1 This PR was reviewed and approved by one reputable person significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.