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

nixos/guix: init module #264323

Closed
wants to merge 1 commit into from
Closed

nixos/guix: init module #264323

wants to merge 1 commit into from

Conversation

viperML
Copy link
Contributor

@viperML viperML commented Oct 30, 2023

Description of changes

Guix was recently merged #246975 . This module would configure the guix-daemon, and add guix to path, similarly to the nix-daemon module.

This is a draft PR and I've added a VM configuration to be deleted, so that changes can be quickly tested.

Todo:

  • Configuring relevant daemon options.
  • Tests
  • ? Boostrapping channels
  • ? Binfmt cross-compilation

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

I hope you don't mind a few comments, even though this PR is marked as draft. This looks neat!

Comment on lines +13 to +20
package = mkOption {
type = with types; package;
default = pkgs.guix;
defaultText = literalExpression "pkgs.guix";
description = mdDoc ''
This option specifies the Guix package to be used in the system.
'';
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
package = mkOption {
type = with types; package;
default = pkgs.guix;
defaultText = literalExpression "pkgs.guix";
description = mdDoc ''
This option specifies the Guix package to be used in the system.
'';
};
package = mkPackageOptionMD pkgs "guix" { };

};

cores = mkOption {
type = with types; int;
Copy link
Member

Choose a reason for hiding this comment

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

Using with types; for everything is a bit overkill. Would you mind using types. when it's a simple type?

Suggested change
type = with types; int;
type = types.ints.unsigned;

};

maxJobs = mkOption {
type = with types; int;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type = with types; int;
type = types.ints.unsigned;

environment.systemPackages = [cfg.package];

systemd.services.guix-daemon = {
path = [cfg.package];
Copy link
Member

Choose a reason for hiding this comment

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

Does this program call itself at some point?

};

extraArgs = mkOption {
type = with types; listOf (coercedTo anything (x: "${x}") str);
Copy link
Member

Choose a reason for hiding this comment

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

This type is a bit weird. I see what you're going for with it being easier to write ints and so on, but I think there are better ways to do this. It doesn't make sense to put bools, attrsets and probably not lists either here, unless guix can read nix syntax in its arguments. anything will also accept types.function, which errors out on string interpolation.

Also, AFAIK string interpolation just invokes the same logic that is used in toString, meaning (x: "${x}") can be replaced with toString.

Maybe we could just keep it to listOf str? Alternatively you could look into using lib.cli.toGNUCommandLineShell.

Comment on lines +9 to +10
options = with lib; {
guix = {
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be flattend down, as I'm assuming you're not planning to make options outside of guix?

Suggested change
options = with lib; {
guix = {
options.guix = with lib; {

@h7x4 h7x4 mentioned this pull request Oct 30, 2023
13 tasks
@viperML viperML closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants