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

Jump to bottom

tree-wide: use systemctl of running system #88492

Merged
merged 17 commits into from May 21, 2020
Merged

tree-wide: use systemctl of running system #88492

merged 17 commits into from May 21, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented May 21, 2020

In many different places in nixpkgs, when shelling out to systemctl, we referred to it as ${pkgs.systemd}/bin/systemctl or ${config.systemd.package}/bin/systemctl.

While this provides a systemctl binary, it's not necessarily one that's fully compatible with the one that's currently running. As the systemd of the running configuration is available at /run/current-system/systemd, we can refer to systemctl via /run/current-system/systemd/bin/systemctl.

This should also prevent some service restarts in the case of systemd.package being updated.

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.

flokli added 17 commits May 21, 2020 10:28
@flokli
nixos/power-management: always run systemctl of the currently running…
783f40b 
… systemd
@flokli
nixos/resolvconf: always run systemctl of the currently running systemd
ea462c7
@flokli
nixos/docker-registry: always run systemctl of the currently running …
8aaca0a 
…systemd
@flokli
nixos/freeswitch: always run systemctl of the currently running systemd
73392b7
@flokli
nixos/dhcpcd: always run systemctl of the currently running systemd
b0222a5
@flokli
nixos/nsd: always run systemctl of the currently running systemd
52e104c
@flokli
nixos/wpa_supplicant: always run systemctl of the currently running s…
1955982 
…ystemd
@flokli
nixos/nginx: always run systemctl of the currently running systemd
a3678ed 
Also, make the postRun script refer to that systemctl, and not just rely
on $PATH for consistency.
@flokli
nixos/display-managers: always run systemctl of the currently running…
3fcfcb8 
… systemd
@flokli
nixos/sddm: always run systemctl of the currently running systemd
de358a0
@flokli
nixos/xautolock: always run systemctl of the currently running systemd
062302e
@flokli
nixos/network-interfaces-scripted: always run systemctl of the curren…
ad26d7c 
…tly running systemd
@flokli
nixos/azure-agent: always run systemctl of the currently running systemd
45c03c0
@flokli
deepin.dde-control-center: always run systemctl of the currently runn…
c7e8cea 
…ing systemd
@flokli
displaylink: run systemctl of the currently running systemd
5cca485
@flokli
autorandr: run systemctl of the currently running systemd
2d5de92
@flokli
google-compute-engine: run systemctl of the currently running systemd
d69f95e
flokli referenced this pull request May 21, 2020
@adisbladis
nixos/display-managers: Also set DBUS_SESSION_BUS_ADDRESS in the wrap…
0f1eb8c 
…per script context
adisbladis
adisbladis approved these changes May 21, 2020
View reviewed changes
Copy link
Member

@adisbladis adisbladis left a comment

Choose a reason for hiding this comment

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

I haven't tested this, but diff LGTM.

@michaelpj
Copy link
Contributor

This makes a bunch of things depend on the filesystem layout of NixOS. If we want to say "use the ambient systemctl", would it not be reasonable to just take it from PATH and have it be a true runtime dependency?

@flokli
Copy link
Contributor Author

flokli commented May 21, 2020

This makes a bunch of things depend on the filesystem layout of NixOS. If we want to say "use the ambient systemctl", would it not be reasonable to just take it from PATH and have it be a true runtime dependency?

In some of these cases, systemctl isn't available in $PATH. Most of the changes here are inside NixOS module code, writing unit files, so assuming NixOS seems reasonable.

In terms of patching applications people might install with Nix on non-NixOS - I didn't look through all of these. The only 3 changes in this PR don't seem critical:

  • displaylink and autorandr only refer to systemctl in some udev contexts, which aren't managed when using on non-NixOS
  • deepin.dde-control-center refers to systemctl in some dbus .policy file, which also likely isn't used when using outside of NixOS

@flokli flokli merged commit 927b779 into NixOS:master May 21, 2020
@flokli flokli deleted the current-system-systemctl branch May 21, 2020 18:22
@vcunat
Copy link
Member

vcunat commented May 22, 2020

Unfortunately this blocks both channels now. There are some checks:

Checking that all programs called by absolute paths in udev rules exist... FAIL
/run/current-system/systemd/bin/systemctl is called in udev rules but is not executable or does not exist

I used

nix build -f nixos/release-small.nix nixos.tests.boot.biosCdrom.x86_64-linux

to bisect to 1955982.

flokli added a commit to flokli/nixpkgs that referenced this pull request May 22, 2020
@flokli
nixos/udev: support /run/current-system/systemd in udevRules
d64d42e 
NixOS#88492 flipped some references to
systemctl from config.systemd.package to /run/current-system/systemd/,
which udevRules obviously isn't able resolve.

If we encounter such references, replace them with
config.systemd.package before doing the check.
@flokli flokli mentioned this pull request May 22, 2020
Merged
10 tasks
@flokli
Copy link
Contributor Author

flokli commented May 22, 2020

Yeah, the check in nixos/modules/services/hardware/udev.nix doesn't know about /run/current-system. I assume other checks might fail too, if more of these patched udev rules land in udev.packages.

I taught udevRules how to resolve these references in #88607, PTAL.

@flokli flokli mentioned this pull request Sep 5, 2020
Merged
10 tasks
@flokli flokli mentioned this pull request Feb 20, 2021
Merged
5 tasks
@flokli flokli mentioned this pull request May 5, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisbladis adisbladis adisbladis approved these changes

@lheckemann lheckemann Awaiting requested review from lheckemann

@jtojnar jtojnar Awaiting requested review from jtojnar

@danbst danbst Awaiting requested review from danbst

@zimbatm zimbatm Awaiting requested review from zimbatm

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

@globin globin Awaiting requested review from globin

@romildo romildo Awaiting requested review from romildo

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flokli @michaelpj @vcunat @adisbladis