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.

Sign up for GitHub

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

Jump to bottom

nixos/systemd: Fix environment type -> allows overriding #38356

Merged
merged 1 commit into from Apr 20, 2018
Merged

nixos/systemd: Fix environment type -> allows overriding #38356

merged 1 commit into from Apr 20, 2018

Conversation

infinisil
Copy link
Member

Motivation for this change

A user on IRC tried to override an environment variable of a service, which didn't work even with mkForce. Turns out reason being is that the type of the environment option of a service was wrong.

Things done

I built my 3 nixos systems with this change and I didn't get an evaluation error.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

infinisil
infinisil commented Apr 2, 2018
View reviewed changes
nixos/modules/system/boot/systemd-unit-options.nix
@@ -217,7 +217,7 @@ in rec {

environment = mkOption {
default = {};
type = types.attrs; # FIXME
type = with types; attrsOf (nullOr (either str package));
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using str | package because str doesn't match derivations, which are used as values in nixpkgs, and can still be converted to strings.

Copy link
Member

Choose a reason for hiding this comment

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

I ran an evaluation of all of the tests, nothing failed because of the more restrictive type.

@infinisil
Copy link
Member Author

For reference, these values get converted to a string here:

nixpkgs/nixos/modules/system/boot/systemd.nix

Lines 321 to 324 in 00fc66d

${let env = cfg.globalEnvironment // def.environment;
in concatMapStrings (n:
let s = optionalString (env."${n}" != null)
"Environment=${builtins.toJSON "${n}=${env.${n}}"}\n";

LnL7
LnL7 approved these changes Apr 4, 2018
View reviewed changes
nixos/modules/system/boot/systemd-unit-options.nix
@@ -217,7 +217,7 @@ in rec {

environment = mkOption {
default = {};
type = types.attrs; # FIXME
type = with types; attrsOf (nullOr (either str package));
Copy link
Member

Choose a reason for hiding this comment

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

I ran an evaluation of all of the tests, nothing failed because of the more restrictive type.

@LnL7 LnL7 merged commit 58fd592 into NixOS:master Apr 20, 2018
@LnL7 LnL7 added the 8.has: port to stable A PR already has a backport to the stable release. label Apr 21, 2018
@dtzWill
Copy link
Member

dtzWill commented May 15, 2018

This seems to have been backported to 18.03 (df4eae6) but the follow-up fix on master (fb2a8d6) has not been backported.

I think this is why my box's config no longer evaluates on 18.03. Can folks involved take a look and backport that second commit if it's appropriate/okay/safe? cc @infinisil @oxij (committers).

FWIW the error I'm getting is this, which may not be related (but checking the follow-up commit seems warranted regardless):

error: The unique option `systemd.services.hydra-manual-setup.environment.PATH' is defined multiple times, in `/etc/nixos/hydra.nix' and `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/nixos/modules/system/boot/systemd.nix'.

Where nixos channel is: /nix/store/zrvfh80xckr2acbmfn34xkkaszsjd8yv-nixos-18.03.132336.ef74cafd3e5.

@LnL7
Copy link
Member

LnL7 commented May 15, 2018

The commit looks good to me, I didn't think about types.path. Guess just checking the tests was a bit too optimistic. 😞

@infinisil
Copy link
Member Author

Shouldn't this get fixed for globalEnvironment too then?

nixpkgs/nixos/modules/system/boot/systemd.nix

Line 518 in f3fcf1b

type = with types; attrsOf (nullOr (either str package));

@infinisil infinisil deleted the fix/environment branch May 16, 2018 19:39
@infinisil
Copy link
Member Author

Shouldn't this get fixed for globalEnvironment too then?

Done in 35c069e

infinisil added a commit that referenced this pull request Jul 18, 2018
@infinisil
nixos: systemd.globalEnvironment: fix type
35c069e 
Follow up fix of fb2a8d6

#38356
#40416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LnL7 LnL7 LnL7 approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@infinisil @dtzWill @LnL7 @GrahamcOfBorg