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.

Sign up for GitHub

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
Closed

support externally extensible systems #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions modules/perSystem.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,21 @@ let
in
{
options = {
systems = mkOption {
systems = mkOption ({
description = ''
All the system types to enumerate in the flake output subattributes.

In other words, all valid values for `system` in e.g. `packages.<system>.foo`.

If the `systems` flake input exists, it will be imported as the
default. See <https://github.com/nix-systems/nix-systems> for a
description of the pattern.
'';
type = types.listOf types.str;
};
} // (lib.optionalAttrs self.inputs?systems) {
# Load the systems input by default if it has been declared.
default = import self.inputs.systems;
});
Comment on lines +73 to +76
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 •
edited
Loading

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 •
edited
Loading

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.


perInput = mkOption {
description = ''
Expand Down
3 changes: 2 additions & 1 deletion template/default/flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

inputs = {
nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
# The list of systems to use
systems.url = "github:nix-systems/default";
};

outputs = inputs@{ flake-parts, ... }:
Expand All @@ -14,7 +16,6 @@
# 3. Add here: foo.flakeModule

];
systems = [ "x86_64-linux" "aarch64-darwin" ];
perSystem = { config, self', inputs', pkgs, system, ... }: {
# Per-system attributes can be defined here. The self' and inputs'
# module parameters provide easy access to attributes of the same
Expand Down