refactor(overlays): split custom-packages-overlay into per-app gated modules#196
Conversation
…modules The combined overlay in modules/base/custom-packages-overlay.nix loaded all 28 entries on every host regardless of whether the matching app was enabled. Splits it into per-package files under modules/custom-overlays/, each gated on programs.<name>.extended.enable of the corresponding flake.nixosModules.apps.<name>. Hosts wire them in via a single auto- discovering aggregator (modules/<host>/custom-overlays-base.nix), mirroring the existing modules/<host>/apps-base.nix pattern that uses lib.attrValues over a flake-output namespace. Adds the option flake.customOverlays (attrsOf deferredModule, declared in modules/meta/flake-output.nix) so overlay submodules survive merge as first- class entries instead of collapsing under flake.nixosModules.* like the existing comment on flake.csec already documents. Adds four new app modules (snixembed, i3-focus-or-launch, i3-scratchpad-show-or-create, monitor-query) so the i3wm helpers fit the gating contract; both hosts now enable them in apps-enable.nix because both run i3wm. The monitor-query module intentionally omits environment.systemPackages because the package is a writeText shell library sourced via pkgs.monitor-query, not a derivation with bin/ outputs. Renames modules/system76/custom-packages-overlay.nix to system76-power-overlay.nix because its only remaining content is the system76-power skip-non-alpm-scsi-hosts.patch override, unrelated to the splitter aggregate. Deletes modules/base/custom-packages-overlay.nix and modules/tpnix/custom-packages-overlay.nix; the flake.lib.overlays.customPackages symbol is no longer defined or referenced. Validation: nix fmt; pre-commit run --all-files --hook-stage manual passes all hooks; nix flake check --accept-flake-config --no-build is clean; nix build of nixosConfigurations.system76 and nixosConfigurations.tpnix toplevel both succeed; nix eval confirms snixembed carries the icon- resolution patch and librepods reports version 0.2.5 once the matching app modules are enabled.
|
Claude finished @Bad3r's task in 5m 43s —— View job ReviewNo issues found. The strict-rules bar requires concrete defects or measurable enhancements; the actionable concerns on this PR (i3-helper gating, |
There was a problem hiding this comment.
Code Review
This pull request refactors the monolithic customPackages overlay into granular, per-app overlays located in modules/custom-overlays/. These overlays are now conditionally applied to nixpkgs.overlays only when the corresponding app module is enabled via programs.<name>.extended.enable. The changes also include an auto-import mechanism for these overlays in host configurations. Feedback highlights two risks: first, core i3 utilities are now gated, which could cause evaluation failures if the i3wm module references them while the app modules are disabled; second, the snixembed service enablement is not synchronized with its new gated overlay, potentially resulting in the use of an unpatched version of the package.
…osts
The previous commit gated four overlays (snixembed, i3-focus-or-launch,
i3-scratchpad-show-or-create, monitor-query) on
`programs.<name>.extended.enable` and listed them under each host's
`apps-enable.nix`. That setup had two foot-guns flagged in the PR review:
1. modules/apps/i3wm/{config,scratchpad,services}.nix reference
`pkgs.<helper>` directly. Disabling any helper on an i3 host would
remove the overlay and surface as "attribute missing" during eval,
not as a clear "you turned off something i3 needs".
2. The snixembed systemd user unit in modules/apps/i3wm/services.nix is
gated only on `xsession.windowManager.i3.enable`. With the overlay
gated on the app-module enable, a host that disabled snixembed but
kept i3 on would launch the upstream nixpkgs snixembed without the
ARGB stride / SVG-loader / icon-resolution patches, producing blank
i3bar tray icons.
Couples the four helpers to i3 itself by setting
`programs.<helper>.extended.enable = lib.mkDefault true` inside the i3
NixOS module (modules/apps/i3wm/nixos.nix). Priority 1000 (mkDefault)
beats the 1100 used in apps-enable.nix, so the four redundant entries in
modules/system76/apps-enable.nix and modules/tpnix/apps-enable.nix are
removed. Hosts can still opt out with no-modifier or `lib.mkForce false`.
Validation: nix flake check --no-build clean; both host closures rebuild;
nix eval confirms each helper resolves to enable=true on system76 and
tpnix via the i3 mkDefault path.
|
@gemini-code-assist Thanks for the review. Both threads addressed in What was acceptedBoth inline comments. They flagged the same underlying issue under different surfaces:
What was rejectedNothing in the substance was rejected. Of the two implementation options you proposed, neither was used directly:
How the accepted ones were implementedA third path: bind the dependency at the module that owns it. programs = {
"i3-focus-or-launch".extended.enable = lib.mkDefault true;
"i3-scratchpad-show-or-create".extended.enable = lib.mkDefault true;
"monitor-query".extended.enable = lib.mkDefault true;
snixembed.extended.enable = lib.mkDefault true;
};Why this shape:
Validation
|
|
The approach of binding dependencies at the module level using |
The previous commit removed the four i3 helper entries (`snixembed`, `i3-focus-or-launch`, `i3-scratchpad-show-or-create`, `monitor-query`) from each host's `apps-enable.nix` because `modules/apps/i3wm/nixos.nix` now sets them via `lib.mkDefault true`. The `apps-catalog-sync` pre-push hook treats every `modules/apps/*.nix` file as a required catalog entry and rejected the push. Re-adds the entries as `lib.mkOverride 1100 false`, the conventional "catalog default off" value for entries managed elsewhere. Priority 1100 is lower than the i3 module's `mkDefault` (1000), so the four helpers still resolve to `true` on i3 hosts. On a hypothetical non-i3 host that imported the app modules directly, the catalog default would correctly leave them off.
…block
`modules/apps/monitor-query.nix` previously declared
`programs.monitor-query.extended.package` via `lib.mkPackageOption` and an
empty `config = lib.mkIf cfg.enable { }` block. Neither served anything:
* `cfg.package` is never read. The overlay in
modules/custom-overlays/monitor-query.nix builds the package itself
via `import lib/shell/monitor-query.nix`, and the only consumer
(modules/apps/i3wm/scratchpad.nix) sources `pkgs.monitor-query`
directly. No call site goes through the option.
* `lib.mkIf cfg.enable { }` is a no-op regardless of `cfg.enable`.
Reduces the module to its actual job: declaring
`programs.monitor-query.extended.enable` so the gated overlay submodule
has something to read. No behavior change on either host (verified with
`nix build` for system76 and tpnix).
Flagged in PR review by claude[bot] on PR #196.
|
@claude Thanks for catching the dead code. Addressed in What was acceptedYour enhancement on
How it was implementedReduced the module to its actual contract — declaring Validation: Pushing now. |
|
Claude finished @Bad3r's task in 1m 5s —— View job Verified
|
…modules (#196) * refactor(overlays): split custom-packages-overlay into per-app gated modules The combined overlay in modules/base/custom-packages-overlay.nix loaded all 28 entries on every host regardless of whether the matching app was enabled. Splits it into per-package files under modules/custom-overlays/, each gated on programs.<name>.extended.enable of the corresponding flake.nixosModules.apps.<name>. Hosts wire them in via a single auto- discovering aggregator (modules/<host>/custom-overlays-base.nix), mirroring the existing modules/<host>/apps-base.nix pattern that uses lib.attrValues over a flake-output namespace. Adds the option flake.customOverlays (attrsOf deferredModule, declared in modules/meta/flake-output.nix) so overlay submodules survive merge as first- class entries instead of collapsing under flake.nixosModules.* like the existing comment on flake.csec already documents. Adds four new app modules (snixembed, i3-focus-or-launch, i3-scratchpad-show-or-create, monitor-query) so the i3wm helpers fit the gating contract; both hosts now enable them in apps-enable.nix because both run i3wm. The monitor-query module intentionally omits environment.systemPackages because the package is a writeText shell library sourced via pkgs.monitor-query, not a derivation with bin/ outputs. Renames modules/system76/custom-packages-overlay.nix to system76-power-overlay.nix because its only remaining content is the system76-power skip-non-alpm-scsi-hosts.patch override, unrelated to the splitter aggregate. Deletes modules/base/custom-packages-overlay.nix and modules/tpnix/custom-packages-overlay.nix; the flake.lib.overlays.customPackages symbol is no longer defined or referenced. Validation: nix fmt; pre-commit run --all-files --hook-stage manual passes all hooks; nix flake check --accept-flake-config --no-build is clean; nix build of nixosConfigurations.system76 and nixosConfigurations.tpnix toplevel both succeed; nix eval confirms snixembed carries the icon- resolution patch and librepods reports version 0.2.5 once the matching app modules are enabled. * fix(i3wm): auto-enable helper apps so gated overlays register on i3 hosts The previous commit gated four overlays (snixembed, i3-focus-or-launch, i3-scratchpad-show-or-create, monitor-query) on `programs.<name>.extended.enable` and listed them under each host's `apps-enable.nix`. That setup had two foot-guns flagged in the PR review: 1. modules/apps/i3wm/{config,scratchpad,services}.nix reference `pkgs.<helper>` directly. Disabling any helper on an i3 host would remove the overlay and surface as "attribute missing" during eval, not as a clear "you turned off something i3 needs". 2. The snixembed systemd user unit in modules/apps/i3wm/services.nix is gated only on `xsession.windowManager.i3.enable`. With the overlay gated on the app-module enable, a host that disabled snixembed but kept i3 on would launch the upstream nixpkgs snixembed without the ARGB stride / SVG-loader / icon-resolution patches, producing blank i3bar tray icons. Couples the four helpers to i3 itself by setting `programs.<helper>.extended.enable = lib.mkDefault true` inside the i3 NixOS module (modules/apps/i3wm/nixos.nix). Priority 1000 (mkDefault) beats the 1100 used in apps-enable.nix, so the four redundant entries in modules/system76/apps-enable.nix and modules/tpnix/apps-enable.nix are removed. Hosts can still opt out with no-modifier or `lib.mkForce false`. Validation: nix flake check --no-build clean; both host closures rebuild; nix eval confirms each helper resolves to enable=true on system76 and tpnix via the i3 mkDefault path. * chore(catalog): list i3 helper apps as catalog defaults The previous commit removed the four i3 helper entries (`snixembed`, `i3-focus-or-launch`, `i3-scratchpad-show-or-create`, `monitor-query`) from each host's `apps-enable.nix` because `modules/apps/i3wm/nixos.nix` now sets them via `lib.mkDefault true`. The `apps-catalog-sync` pre-push hook treats every `modules/apps/*.nix` file as a required catalog entry and rejected the push. Re-adds the entries as `lib.mkOverride 1100 false`, the conventional "catalog default off" value for entries managed elsewhere. Priority 1100 is lower than the i3 module's `mkDefault` (1000), so the four helpers still resolve to `true` on i3 hosts. On a hypothetical non-i3 host that imported the app modules directly, the catalog default would correctly leave them off. * refactor(monitor-query): drop dead `package` option and no-op config block `modules/apps/monitor-query.nix` previously declared `programs.monitor-query.extended.package` via `lib.mkPackageOption` and an empty `config = lib.mkIf cfg.enable { }` block. Neither served anything: * `cfg.package` is never read. The overlay in modules/custom-overlays/monitor-query.nix builds the package itself via `import lib/shell/monitor-query.nix`, and the only consumer (modules/apps/i3wm/scratchpad.nix) sources `pkgs.monitor-query` directly. No call site goes through the option. * `lib.mkIf cfg.enable { }` is a no-op regardless of `cfg.enable`. Reduces the module to its actual job: declaring `programs.monitor-query.extended.enable` so the gated overlay submodule has something to read. No behavior change on either host (verified with `nix build` for system76 and tpnix). Flagged in PR review by claude[bot] on PR #196.
Summary
modules/base/custom-packages-overlay.nixinto 28 per-package files undermodules/custom-overlays/, each gated onprograms.<name>.extended.enableof the matchingflake.nixosModules.apps.<name>.flake.customOverlays(attrsOf deferredModule) inmodules/meta/flake-output.nixso overlay submodules survive merge as first-class entries instead of collapsing underflake.nixosModules.*(same rationale already documented onflake.csec).snixembed,i3-focus-or-launch,i3-scratchpad-show-or-create,monitor-query) and enables them on both hosts viaapps-enable.nix.monitor-query's app module deliberately omitsenvironment.systemPackagessince the package is awriteTextshell library sourced viapkgs.monitor-query.modules/system76/custom-overlays-base.nixandmodules/tpnix/custom-overlays-base.nixthat auto-discover overlays vialib.attrValues (config.flake.customOverlays or { }), mirroring the existingapps-base.nixpattern.modules/system76/custom-packages-overlay.nixtosystem76-power-overlay.nix(only thesystem76-powerskip-non-alpm-scsi-hosts.patchoverride survives). Deletesmodules/base/custom-packages-overlay.nixandmodules/tpnix/custom-packages-overlay.nix;flake.lib.overlays.customPackagesis no longer defined or referenced.Test plan
nix fmtnix develop -c pre-commit run --all-files --hook-stage manual(all hooks pass)nix flake check --accept-flake-config --no-build(clean)nix build .#nixosConfigurations.system76.config.system.build.toplevelnix build .#nixosConfigurations.tpnix.config.system.build.toplevelnix evalconfirmssnixembedcarriesicon-resolution.patchandlibrepodsreports version0.2.5i3-scratchpad-show-or-createafter activation