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

support externally extensible systems #133

Closed
wants to merge 1 commit into from

Conversation

zimbatm
Copy link
Contributor

@zimbatm zimbatm commented Apr 9, 2023

With this change, it becomes possible to modify the list of systems that
a flake is using, using only the flake override mechanisms.

See numtide/treefmt#228 and
https://github.com/nix-systems/nix-systems.

With this change, it becomes possible to modify the list of systems that
a flake is using, using only the flake override mechanisms.

See <numtide/treefmt#228> and
<https://github.com/nix-systems/nix-systems>.
Comment on lines +73 to +76
} // (lib.optionalAttrs self.inputs?systems) {
# Load the systems input by default if it has been declared.
default = import self.inputs.systems;
});
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really fit the architecture, because we shouldn't be making assumptions about the user's flake inputs; certainly not in the core, non-optional modules.

It'd be nicer to have nix-systems implement the flake-parts interface, by providing a module that just sets the systems option. In that case it doesn't need to be a default either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does flake-parts auto-import inputs' flakeModules?

Alternatively, I would propose extending the flake-parts "systems" type to support inherits systems; when systems is an input.

Copy link
Member

Choose a reason for hiding this comment

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

By default it does try to import all inputs, and I wouldn't encourage that, because that would defeat lazy fetching and any solution to #119
Accepting a flake (I presume?) for an arbitrary option does not seem like a sensible solution to me. It abbreviates the syntax, but that is not a goal. An option should have a meaningful type.

Copy link

@bb010g bb010g Apr 13, 2023

Choose a reason for hiding this comment

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

This doesn't really fit the architecture, because we shouldn't be making assumptions about the user's flake inputs; certainly not in the core, non-optional modules.

This already happens with the Nixpkgs module's _module.args.pkgs assuming inputs.nixpkgs. This is justified through pkgs being an important, special case. I do think the same could be said here for systems.

It'd be nicer to have nix-systems implement the flake-parts interface, by providing a module that just sets the systems option. In that case it doesn't need to be a default either.

github:nix-systems/default can't provide a module. All consumers have to assume that inputs.systems.flake = false; for the simple overrides this pattern allows to work properly. (inputs.systems.url = "file:./systems.nix"; is very intentionally valid.) github:nix-systems/nix-systems could provide a module, but it's essentially documentation for the pattern and, if the goal of github:nix-systems/default moving to github:NixOS/systems.default is met, should cease to exist at some point, or maybe it might just move to github:NixOS/systems.README. Point is, it's far heavier than the other system inputs are and I don't ever see a reason it would be a module input. The idea is to deal with Nix flake's lack of sane systems handling by making overriding systems as boring & smooth as possible. Specifying inputs.<name>.inputs.systems.follows = "systems"; will still be required for each input that uses systems, which is boilerplatey, but ideally this boilerplate is cut to the bare minimum. If flake-parts encourages using a systems flake input that is overridden instead of a lexical systems value stuck under outputs, only accessible to flake modules, then that's more of a barrier to systems being as widely integrated as possible.

systems is meant to be the special case flake input until this gets resolved in a cleaner way in Nix proper.

We're hoping github:nix-systems/default is assigned flake:systems in the flake registry, which would allow even less boilerplate for the happy path here.


I would recommend changing the fancy conditional attrset merge here with normal

mkOption {
  # …
  default = import inputs.systems or (throw "flake-parts: The flake does not have a `systems` input. Please add it, or set `systems` yourself.");
  defaultText = literalExpression ''import inputs.systems'';
}

like the Nixpkgs module does.

Copy link
Member

Choose a reason for hiding this comment

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

This already happens with the Nixpkgs module's _module.args.pkgs assuming inputs.nixpkgs.

That's a temporary measure and a bad precedent, and documented as such in the code. I will remove it.

"file:./systems.nix"

I wasn't aware of that requirement.

I think the standard way is good enough then. All this fancy system inheriting stuff is just a ridiculous workaround anyway.

systems = import inputs.systems;

Copy link

@bb010g bb010g Apr 13, 2023

Choose a reason for hiding this comment

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

I think the standard way is good enough then. All this fancy system inheriting stuff is just a ridiculous workaround anyway.

systems = import inputs.systems;

That's fair; the idea behind this would be to reduce boilerplate just a little bit more and push people towards just using inputs.systems instead of deciding to list the inputs out inline. It's a ridiculous workaround because it's a ridiculous problem to have so many hard-coded and hard to override/patch instances of systems lists in flakes today.

Copy link

Choose a reason for hiding this comment

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

This already happens with the Nixpkgs module's _module.args.pkgs assuming inputs.nixpkgs.

That's a temporary measure and a bad precedent, and documented as such in the code. I will remove it.

It's probably better to discuss this more in #137, but I think it would be a shame to see that module go before a capable inputs.nixpkgs.flakeModules.default exists. Virtually every flake will need Nixpkgs, and having to manually set up config._module.args.pkgs boilerplate or import a 3rd-party flake module for it will push people away from using flake-parts when they're first evaluating it. At the very least, some sort of inputs.nixpkgs-flake-parts.url = "github:hercules-ci/nixpkgs-flake-parts"; needs to exist first, before this is removed. I think the Nixpkgs module is an okay exception until flake-parts picks up more momentum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roberth what is your conclusion? I wouldn't mind re-opening the PR if it's possible to move forwards.

@zimbatm zimbatm closed this Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants