nixos/modular-services: add portable process.environment - #518860
Conversation
ed9202c to
b168777
Compare
process.environment and process.environmentFileprocess.environment
b168777 to
540a74f
Compare
540a74f to
201526e
Compare
201526e to
e50372c
Compare
|
Something to consider for this pr is handling secrets securely. |
Adds modular service option `process.environment` to pass an attrset of env vars to the service manager. `null` values actively unset the variable before the process starts. Values are `coercedTo (either path package) str` via interpolation, mirroring `pathOrStr`, so paths and packages render to store-path strings with string context preserved. The type is `lazyAttrsOf`, allowing one entry to reference another (recursive env definitions). The systemd backend unsets entries using `unexport` in `ExecStart`, so the variable is absent even when `Environment=` or the inherited environment would otherwise supply it. The systemd backend lifts non-null entries onto the primary unit wrapped per-key with `lib.mkDefault` so they merge with the existing priority-100 `environment.PATH` binding in `nixos/lib/systemd-lib.nix` while still letting explicit `systemd.service.environment.<k>` overrides win. The systemd extra-root modules are loaded via `importApply`, closing `pkgs` over `systemd/service.nix` as a non-module argument (matching the portable `lib/services/service.nix` convention) instead of passing a redundant `pkgs` specialArg. The docs eval threads `pkgs = throw` accordingly. Portable coverage lives in `testers.modularServiceCompliance`: an eval-level check that a set value round-trips and a `null` value is preserved, plus an integration test that records the service's own `/proc/$$/environ` and asserts the set variable is present and the null variable is absent. The systemd-specific grep assertions in `systemd/test.nix` cover how systemd achieves this (`Environment=` rendering, null filtering, the `unexport` wrapper, and override precedence). Assisted-by: Claude:claude-opus-4-8
roberth
left a comment
There was a problem hiding this comment.
LGTM!
Tests pass after rebase ✔️
|
Since this merge, I have this: error: function 'anonymous lambda' called with unexpected argument 'lib'
at «github:nixos/nixpkgs/ec830498ebdf7052e3ce91646d09ae7b43313384?narHash=sha256-A2KfwKftvyG1%2BE%2Bmr1Fh/Q0rM6tQbCq4pB7sRbd/Q5c%3D»/nixos/modules/system/service/systemd/service.nix:4:1:
3| # This makes service modules self-contained, allowing mixing of Nixpkgs versions.
4| { pkgs }:
| ^
5| |
| # Non-module arguments | ||
| # These are separate from the module arguments to avoid implicit dependencies. | ||
| # This makes service modules self-contained, allowing mixing of Nixpkgs versions. | ||
| { pkgs }: |
There was a problem hiding this comment.
It seems that this breaks home-manager at this location. https://github.com/nix-community/home-manager/blob/master/modules/services-modular/service.nix
Changing the import in the home-manager service.nix file to the following appears to fix it. I'm not sure if there are many other consumers of this file?
(import (nixpkgsPath + "/nixos/modules/system/service/systemd/service.nix") { inherit pkgs; })There was a problem hiding this comment.
Maybe lib.modules.importApply instead of import, since that's what they use in this merge? But yes, the problem remains, because this might not be the only place where this import appears
There was a problem hiding this comment.
Is the only use of pkgs unsetting environment variables with pkgs.execline in generated systemd units? Can UnsetEnvironment= be used?
There was a problem hiding this comment.
thanks - i filed a fix based on UnsetEnvironment now at #545521.
|
after this commit it stopped building |
|
OK so we clearly need more integration tests for modular service consumers, at the very least. Reverting for now. |
Switch the approach to unsetting environment variables in NixOS's modular services from `pkgs.execline` to `UnsetEnvironment`, removing the closure that broke usage for downstream consumers depending on the prior interface. Post-merge comments at NixOS#518860 revealed that the newly-introduced closure at `nixos/modules/system/service/systemd/service.nix`, introducing a `pkgs` argument to unset environment variables using `pkgs.execline`, broke downstream consumption such as by Home Manager, which so far imports this file by path. Note that, while a proper fix to making downstream consumption more robust is introduced at NixOS#507052, the current change renders unsetting of environment variables more idiomatic to systemd, while simplifying the interface again. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: cinereal <cinereal@riseup.net>
…etEnvironment` Adds modular service option `process.environment` to pass an attrset of env vars to the service manager. `null` values actively unset the variable before the process starts. The systemd backend unsets entries using `UnsetEnvironment`, so the variable is absent even when `Environment=` or the inherited environment would otherwise supply it. Differs from the ealier approach from NixOS#518860, which instead unset environment variables using `pkgs.execline`, removing its `pkgs` closure that broke usage for downstream consumers depending on the prior interface. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: cinereal <cinereal@riseup.net>
…etEnvironment` Adds modular service option `process.environment` to pass an attrset of env vars to the service manager. `null` values actively unset the variable before the process starts. The systemd backend unsets entries using `UnsetEnvironment`, so the variable is absent even when `Environment=` or the inherited environment would otherwise supply it. Differs from the ealier approach from NixOS#518860, which instead unset environment variables using `pkgs.execline`, removing its `pkgs` closure that broke usage for downstream consumers depending on the prior interface. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: cinereal <cinereal@riseup.net>
|
Thank you @K900 for the revert. The root cause here is a missing public interface for the systemd integration, which was built against NixOS, not against systemd in isolation. This also means that we should soon merge
And of course we need to refactor out a systemd integration that isn't as coupled to NixOS, and give that a documented and tested interface that HM can use. |
…etEnvironment` Adds modular service option `process.environment` to pass an attrset of env vars to the service manager. `null` values actively unset the variable before the process starts. The systemd backend unsets entries using `UnsetEnvironment`, so the variable is absent even when `Environment=` or the inherited environment would otherwise supply it. Differs from the ealier approach from NixOS#518860, which instead unset environment variables using `pkgs.execline`, removing its `pkgs` closure that broke usage for downstream consumers depending on the prior interface. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: cinereal <cinereal@riseup.net>
Adds new modular service option
process.environmentfor declaring env vars.Disclaimer: I used a coding agent in the creation of this patch.
Things done
passthru.tests.nix-instantiate --eval lib/services/test.nixnixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.