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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/nh: init #294923

Merged
merged 1 commit into from Apr 15, 2024
Merged

nixos/nh: init #294923

merged 1 commit into from Apr 15, 2024

Conversation

viperML
Copy link
Contributor

@viperML viperML commented Mar 11, 2024

Description of changes

Mostly a copy of the upstream module: https://github.com/viperML/nh/blob/master/module.nix

Marking as draft to get reactions about it. I need to add the release notes.

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

@viperML
Copy link
Contributor Author

viperML commented Mar 11, 2024

Idk why the bot adds the has module (update) tag

drupol
drupol previously requested changes Mar 11, 2024
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
@viperML viperML force-pushed the nh-module branch 2 times, most recently from 4daaf6a to f02b017 Compare March 11, 2024 09:10
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
@viperML
Copy link
Contributor Author

viperML commented Mar 11, 2024

Fixed some issues resulting of not using the programs. prefix. Now nixos evals, so should be ready.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/3619

@viperML viperML requested a review from drupol March 12, 2024 08:10
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi mate,

I'm not that old yet to review NixOS module correctly. I need more XP. I'll leave this to one of the other maintainer.

Copy link
Member

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

From my view:

  • Usually in program modules, there's a let-binding at the top with
let
  cfg = config.programs.nh;
in

which helps avoid redundancy / improve readability

  • The other mdDoc usages can be removed as well

Otherwise seems ok to me

@viperML
Copy link
Contributor Author

viperML commented Mar 12, 2024

The other mdDoc usages can be removed as well

All of them?

config = {
warnings =
if (!(cfg.clean.enable -> !config.nix.gc.automatic)) then [
"programs.nh.clean.enable and nix.gc.automatic are both enabled. Please use one or the other to avoid conflict."
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
"programs.nh.clean.enable and nix.gc.automatic are both enabled. Please use one or the other to avoid conflict."
"programs.nh.clean.enable and nix.gc.automatic conflict with each other, please decide for one of them."

Copy link
Member

Choose a reason for hiding this comment

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

This is less correct than the original; what was the intention behind this suggestion?

Comment on lines +55 to +56
warnings =
if (!(cfg.clean.enable -> !config.nix.gc.automatic)) then [
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be an assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was suggested by @eclairevoyant before: #294923 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to arbitrarily block settings if they don't actually conflict. Warning seems better IMO.

Comment on lines +62 to +65
{
assertion = cfg.clean.enable -> cfg.enable;
message = "programs.nh.clean.enable requires programs.nh.enable";
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not set this option to true?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think other modules work this way.

@viperML
Copy link
Contributor Author

viperML commented Apr 9, 2024

(oops)

Copy link
Member

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Seems fine otherwise

nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
nixos/modules/programs/nh.nix Outdated Show resolved Hide resolved
@eclairevoyant eclairevoyant merged commit 3947220 into NixOS:master Apr 15, 2024
21 checks passed
@viperML
Copy link
Contributor Author

viperML commented Apr 15, 2024

thanks

viperML added a commit to viperML/nh that referenced this pull request Apr 18, 2024
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.

None yet

7 participants