Skip to content

refactor: overhaul flake - #37

Merged
ToRvaLDz merged 6 commits into
ToRvaLDz:mainfrom
linusammon:main
Jul 19, 2026
Merged

refactor: overhaul flake#37
ToRvaLDz merged 6 commits into
ToRvaLDz:mainfrom
linusammon:main

Conversation

@linusammon

@linusammon linusammon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

This PR overhauls the flake to be more maintainable and follow current best practices.

  • Split package, NixOS module, and devshell into their own files under nix/
  • Dropped flake-utils dependency in favor of a small helper
  • Version is now extracted from pyproject.toml at build time instead of being hardcoded
  • Added a package option to the NixOS module so users can override which package gets installed
  • Moved the polkit rule to a dedicated file via environment.etc for controllable ordering and to avoid interference with 10-nixos.rules
  • Dev shell now uses inputsFrom to pull build inputs from the package, avoiding duplication
  • Translated all option descriptions to English
  • Switched nixpkgs input to the channel tarball URL as it is faster, has no rate limits and is more reliable

Tested nix run, nix develop, as well as using the NixOS module in my own config.

@ToRvaLDz

Copy link
Copy Markdown
Owner

Thanks for this, it's a solid cleanup and the split into nix/ is a clear improvement. Extracting the version from pyproject.toml also fixes a real drift bug: the flake was still pinned at 0.5.0 while the project is at 0.6.7.

One change needed before merge:

forEachSystem iterates over lib.platforms.linux. That list contains ~30 doubles (m68k-linux, s390-linux, mips64-linux, microblaze-linux, ...), but nixpkgs.legacyPackages is only exposed for the systems in lib.systems.flakeExposed. For everything else nixpkgs.legacyPackages.${system} is a missing attribute. Since attrset values are lazy, nix build .#default works fine (which is why it passed your testing), but anything that forces all outputs, e.g. nix flake show or nix flake check, fails to evaluate. Could you switch to an explicit list?

forEachSystem =
  perSystem:
  lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (
    system: perSystem nixpkgs.legacyPackages.${system} system
  );

One small correction to the PR description, no code change needed: security.polkit.extraConfig is types.lines, so multiple definitions are concatenated rather than overriding each other. The move to environment.etc is still the better approach (dedicated file, controllable ordering, no interference with 10-nixos.rules), so I'm happy to keep it, just wanted to note the rationale.

Optional, entirely up to you since you're already in that file: adding security.polkit.enable = lib.mkDefault true; inside the enablePolkit branch would make the rule effective even on setups where polkit isn't pulled in by something else.

Everything else I spotted is minor polish (pyproject = true instead of the deprecated format = "pyproject", pytest/ruff in the dev shell, enabling doCheck now that the test suite exists, documenting the new programs.monique.package option in the README). I'll take care of those in a follow-up after merge, no need for you to do it here.

- Split package, NixOS module, and devshell into their own files
- Drop flake-utils in favor of a small helper
- Extract version from pyproject.toml instead of hardcoding
- Add package option to NixOS module
- Use environment.etc for polkit configuration
- Use inputsFrom in devshell to avoid duplicating build inputs
- Translate option descriptions to English
- Switch nixpkgs input to channel tarball URL
@linusammon

linusammon commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

forEachSystem iterates over lib.platforms.linux. That list contains ~30 doubles (m68k-linux, s390-linux, mips64-linux, microblaze-linux, ...), but nixpkgs.legacyPackages is only exposed for the systems in lib.systems.flakeExposed. For everything else nixpkgs.legacyPackages.${system} is a missing attribute. Since attrset values are lazy, nix build .#default works fine (which is why it passed your testing), but anything that forces all outputs, e.g. nix flake show or nix flake check, fails to evaluate. Could you switch to an explicit list?

Ah I didn't test with --all-systems, changed forEachSystem to an explicit list, and also updated meta.platforms on the package itself.

One small correction to the PR description, no code change needed: security.polkit.extraConfig is types.lines, so multiple definitions are concatenated rather than overriding each other. The move to environment.etc is still the better approach (dedicated file, controllable ordering, no interference with 10-nixos.rules), so I'm happy to keep it, just wanted to note the rationale.

Thanks for the correction, my bad, I assumed it was a string. Updated the PR description for future reference.

Optional, entirely up to you since you're already in that file: adding security.polkit.enable = lib.mkDefault true; inside the enablePolkit branch would make the rule effective even on setups where polkit isn't pulled in by something else.

I did consider that but decided against it, as it seems to me like enablePolkit is really only needed for specific use cases - enabling polkit by default for everyone using the NixOS module didn't seem ideal. That said, perhaps defaulting enablePolkit to false but enabling polkit when set to true would be a better approach?

@ToRvaLDz

Copy link
Copy Markdown
Owner

Thanks for the quick turnaround, the explicit system list and the meta.platforms update look right.

On polkit. I'd rather not flip the default. enablePolkit is true today, so switching it to false would silently take the rule away from everyone who already has programs.monique.enable = true: the password prompt would come back on the next rebuild with nothing explaining why. It's also the right default on the merits, since writing the SDDM Xsetup script and the greetd config is a core part of what Monique does, not an opt-in extra.

I think lib.mkDefault already gives you the safety you're looking for. It sits at priority 1000, so any explicit security.polkit.enable = false; in the user's config (priority 100) wins over it. It isn't forcing polkit on anybody: it only says "if you have no opinion, and you asked for the polkit rule, then you need polkit running". Anyone who wants neither sets enablePolkit = false and nothing gets enabled.

The concrete problem with the current state is that without security.polkit.enable the rule is an inert file in /etc/polkit-1/rules.d: the user enables the module, tries to write Xsetup, gets a password prompt, and has no way to work out why. So:

config = lib.mkIf cfg.enable {
  security.polkit.enable = lib.mkDefault cfg.enablePolkit;
  ...
};

If you'd still rather not touch security.polkit.enable at all, the idiomatic fallback is a warning:

warnings = lib.optional (cfg.enablePolkit && !config.security.polkit.enable)
  "programs.monique.enablePolkit is set but security.polkit.enable is false, so the rule will have no effect.";

Either is fine by me. Silence is the one option I'd like to avoid.

Two more things before merge, both small.

  1. programs.monique.package is types.nullOr types.package with a null default, guarded by lib.optional (cfg.package != null). Through nixosModules.default this always works because the flake wrapper supplies the mkDefault. But anyone importing nix/nixos-module.nix directly gets enable = true installing nothing at all, with no error. Plain types.package with no default keeps the flake path working and turns the direct-import case into an explicit evaluation error.

  2. The package installs $out/lib/systemd/user/moniqued.service, but NixOS doesn't pick unit files up from packages, so systemctl --user enable moniqued currently fails on NixOS. systemd.packages = [ cfg.package ]; in the config block fixes it. This is a pre-existing bug rather than something you introduced, so if you'd rather not widen the PR, say so and I'll handle it in the follow-up.

Nit, no action needed if you'd rather I do it: the description in both flake.nix and nix/package.nix still says "for Hyprland and Sway", but Niri is supported now.

The rest of my earlier list (pyproject = true, re-enabling doCheck now that the tests exist and import no gi, pytest/ruff in the dev shell, documenting programs.monique.package in the README) stays with me after merge.

@linusammon

linusammon commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up.

I'd rather not flip the default. enablePolkit is true today, so switching it to false would silently take the rule away from everyone who already has programs.monique.enable = true

Fair point, went with your suggestion: lib.mkDefault cfg.enablePolkit.

programs.monique.package is types.nullOr types.package...

Changed to plain types.package.

The package installs $out/lib/systemd/user/moniqued.service, but NixOS doesn't pick unit files up from packages...

Added systemd.packages = [ cfg.package ] so the unit file gets picked up.

the description in both flake.nix and nix/package.nix still says "for Hyprland and Sway", but Niri is supported now.

Updated both to include Niri.

@ToRvaLDz

Copy link
Copy Markdown
Owner

Thanks for the thorough back and forth on this, and for picking up the systemd.packages fix even though it was a pre-existing bug rather than something your PR introduced.

The split into nix/, the version extraction from pyproject.toml and the first committed flake.lock are all clear improvements, and dropping flake-utils takes the flake down from 121 lines to 43.

Merging. The remaining polish stays with me as agreed: pyproject = true instead of the deprecated format = "pyproject", re-enabling doCheck now that the test suite exists and imports no gi, pytest/ruff in the dev shell, and documenting programs.monique.package in the README.

@ToRvaLDz
ToRvaLDz merged commit 29fb361 into ToRvaLDz:main Jul 19, 2026
10 checks passed
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.

2 participants