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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/systemd: Don't use apply for $PATH #91092

Merged
merged 1 commit into from Sep 4, 2020

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Jun 19, 2020

When not using apply, other modules can use $PATH as a list instead of
getting a colon-separated list to each /bin directory.

Motivation for this change

Follow-up of #75510.
@ajs124 and I have an AppArmor module which would greatly benefit from being able to parse the path as a list.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dasJ
Copy link
Member Author

dasJ commented Jun 19, 2020

cc @Ma27 @flokli @cole-h

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

Can you provide an example, or (even better a test)?

@dasJ
Copy link
Member Author

dasJ commented Jun 19, 2020

{ config, lib, ... }: with lib; {
  config = {
    warnings = mapAttrsToList (n: v: "${n}.service needs ${if isList v.path then concatStringsSep ", " v.path else v.path}") config.systemd.services;
  };
}

This will either print the colon-separated $PATH or a list of packages (depending on whether you applied this PR).

@flokli flokli requested a review from infinisil June 19, 2020 21:15
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Big 👍 on this. I think this needs a backward incompatibility release note though.

@dasJ
Copy link
Member Author

dasJ commented Sep 1, 2020

@infinisil like this?

@ajs124
Copy link
Member

ajs124 commented Sep 3, 2020

There are conflicts in the release notes @dasJ

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looks good after conflicts resolved

When not using apply, other modules can use $PATH as a list instead of
getting a colon-separated list to each /bin directory.
@dasJ
Copy link
Member Author

dasJ commented Sep 3, 2020

Can anyone with write permissions do the mergy thing?

@dasJ dasJ requested a review from Mic92 September 3, 2020 19:54
@infinisil infinisil merged commit 41c29f2 into NixOS:master Sep 4, 2020
@dasJ dasJ deleted the systemd-path-noapply branch September 4, 2020 11:02
ajs124 pushed a commit that referenced this pull request Sep 9, 2020
Following changes in #91092 the `path` attribute is now a list
instead of being a string. This resulted resulted in the following evaluation error:

"cannot coerce a list to a string, at [...]/nixos/modules/services/networking/openvpn.nix:16:18"

so we now need to convert it to the right type ourselves.

Closes #97360.
infinisil pushed a commit that referenced this pull request Sep 10, 2020
Following changes in #91092 the `path` attribute is now a list
instead of being a string. This resulted resulted in the following evaluation error:

"cannot coerce a list to a string, at [...]/nixos/modules/services/networking/openvpn.nix:16:18"

so we now need to convert it to the right type ourselves.

Closes #97360.

(cherry picked from commit cb14135)
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

4 participants