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 dasJ commented Jun 19, 2020

Copy link
Contributor

@flokli flokli left a comment

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

@dasJ
Copy link
Member Author

@dasJ 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 Jun 19, 2020
Copy link
Member

@Infinisil Infinisil left a comment

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

@dasJ dasJ force-pushed the helsinki-systems:systemd-path-noapply branch from 91d4bc8 to 8965e73 Sep 1, 2020
@dasJ
Copy link
Member Author

@dasJ dasJ commented Sep 1, 2020

@Infinisil like this?

@ajs124
Copy link
Member

@ajs124 ajs124 commented Sep 3, 2020

There are conflicts in the release notes @dasJ

Copy link
Member

@Infinisil Infinisil left a comment

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 dasJ force-pushed the helsinki-systems:systemd-path-noapply branch from 8965e73 to 8cf4ec8 Sep 3, 2020
@dasJ
Copy link
Member Author

@dasJ dasJ commented Sep 3, 2020

Can anyone with write permissions do the mergy thing?

@dasJ dasJ requested a review from Mic92 Sep 3, 2020
@Infinisil Infinisil merged commit 41c29f2 into NixOS:master Sep 4, 2020
17 checks passed
17 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="8cf4ec8"; rev="8cf4ec8b97f4a76aa3243e344e64c8a29b8788cc"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@dasJ dasJ deleted the helsinki-systems:systemd-path-noapply branch Sep 4, 2020
Tomahna added a commit to Tomahna/nixpkgs that referenced this pull request Sep 9, 2020
Following changes in NixOS#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 NixOS#97360.
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 added 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.