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-lib: don't fail on systemd.packages duplicates #77294

Merged
merged 1 commit into from Jan 17, 2020

Conversation

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jan 8, 2020

In some cases like we've noticed in #76169,
having duplicate packages in systemd.packages like

systemd.packages = [ gnome-shell gnome-shell gnome-session ];

breaks.

Here we use an associative array to ensure no
duplicate paths when we symlink all the units listed
in systemd.packages.

Motivation for this change

A certain situation popped up where gnome3 adds gnome-shell and gnome-session to systemd.packages, but gdm also depends on gnome-shell there.

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.
In some cases like we've noticed in #76169,
having duplicate packages in systemd.packages like
```
systemd.packages = [ gnome-shell gnome-shell gnome-session ];
```
breaks.

Here we use an associative array to ensure no
duplicate paths when we symlink all the units listed
in systemd.packages.
@@ -147,7 +147,13 @@ in rec {
done
# Symlink all units provided listed in systemd.packages.
for i in ${toString cfg.packages}; do
packages="${toString cfg.packages}"

This comment has been minimized.

@jtojnar

jtojnar Jan 8, 2020
Contributor

Would not toString (lib.unique cfg.packages) work?

This comment has been minimized.

@hedning

hedning Jan 8, 2020
Contributor

lib.unique is O(n^2), though not sure if that will realistically cause problems. Otherwise I'd say applying unique to systemd.packages would be close to ideal (having a built in set type would be the best though :).

This comment has been minimized.

@worldofpeace

worldofpeace Jan 8, 2020
Author Member

Wouldn't a built in set type be attrsOf? I think it would be un-intuitive for systemd.packages to have to be declared in anything other than a list, and because of systemPackages.
On the topic of being O(n^2), there's a deprecated type that is basically this suggestion uniqList.

When it comes to removing duplicates in bash versus lib.unique, idk.

This comment has been minimized.

@hedning

hedning Jan 10, 2020
Contributor

Wouldn't a built in set type be attrsOf?

Right, I was thinking more like a math set, ie. something like uniqList. Agreed that it makes the most UX sense for it to be list.

When it comes to removing duplicates in bash versus lib.unique, idk.

Yep, I'm fine with both. Doing it through the option's apply with unique is clean and will guard against any other uses, but O(n^2) isn't ideal so 🤷‍♂️

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Jan 13, 2020

@GrahamcOfBorg test systemd gnome3

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Jan 13, 2020

Copy link
Contributor

@hedning hedning left a comment

Looks fine to me at least. Tested together with #76985.

@worldofpeace worldofpeace merged commit b3c8534 into NixOS:master Jan 17, 2020
14 checks passed
14 checks passed
tests.gnome3, tests.systemd on aarch64-linux Failure
Details
tests.gnome3, tests.systemd on x86_64-linux Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
@worldofpeace worldofpeace deleted the worldofpeace:systemd-packages-duplicates branch Jan 17, 2020
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

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