Skip to content

Comments

nixos: systemd: split module up into multiple files#164016

Merged
flokli merged 17 commits intoNixOS:masterfrom
bobvanderlinden:pr-refactor-systemd-module
Mar 21, 2022
Merged

nixos: systemd: split module up into multiple files#164016
flokli merged 17 commits intoNixOS:masterfrom
bobvanderlinden:pr-refactor-systemd-module

Conversation

@bobvanderlinden
Copy link
Member

@bobvanderlinden bobvanderlinden commented Mar 13, 2022

Description of changes

Currently the systemd module is quite large. It contains multiple components that could be split off into separate files.

The idea behind this refactor is to not only make the modules more readable, but it is also a step towards more reusable systemd configuration building.

This PR does introduce a new option called systemd.additionalUpstreamUserUnits. It was introduced to have systemd-tmpfiles specify its own user units, instead of systemd-user doing that for systemd-tmpfiles. I think the way these upstream units still needs a bit of work, but I intend to do that in a separate PR.

A next step is to generalize unit directory building between systemd system and systemd user. The main intention is to also reuse this for systemd in initrd (#120015). It avoids creating yet another system of unit building.

I did not ran all tests, but I did ran the systemd tests as well as applied this configuration to my own system.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@bobvanderlinden bobvanderlinden requested a review from dasJ as a code owner March 13, 2022 16:07
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 13, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 13, 2022
@bobvanderlinden bobvanderlinden mentioned this pull request Mar 13, 2022
Closed
20 tasks
@nixos-discourse
Copy link

nixos-discourse commented Mar 15, 2022

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/777

@bobvanderlinden bobvanderlinden mentioned this pull request Mar 15, 2022
Merged
13 tasks
Artturin
Artturin reviewed Mar 15, 2022
View reviewed changes
SuperSandro2000
SuperSandro2000 reviewed Mar 15, 2022
View reviewed changes
@bobvanderlinden bobvanderlinden force-pushed the pr-refactor-systemd-module branch from ede620c to 2991ce5 Compare March 17, 2022 20:20
@github-actions github-actions bot added the 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. label Mar 17, 2022
@bobvanderlinden bobvanderlinden force-pushed the pr-refactor-systemd-module branch 2 times, most recently from d07fd68 to 7339fbc Compare March 17, 2022 21:47
@bobvanderlinden
nixos: systemd: split off logind into separate module
c647002
@bobvanderlinden
nixos: systemd-logind: use cfg shorthand
e2cb890
@bobvanderlinden
nixos: systemd: split off journald into separate module
fee1e24
@bobvanderlinden
nixos: systemd-journald: use cfg shorthand
7adc8ec
@bobvanderlinden
nixos: systemd: split off coredump into separate module
ccfcb78
@bobvanderlinden
nixos: systemd-coredump: use cfg shorthand
cae8ef1
@bobvanderlinden
nixos: systemd: split off systemd-user into separate module
022b420
@bobvanderlinden
nixos: systemd-user: use cfg shorthand
b6d5052
@bobvanderlinden
nixos: systemd-user: allow additional upstream user units
0e665d1 
Currently it is only possible to add upstream _system_ units. The option
systemd.additionalUpstreamSystemUnits can be used for this.

However, this was not yet possible for systemd.user. In a similar
fashion this was added to systemd-user.nix.

This is intended to have other modules add upstream units.
@bobvanderlinden
nixos: systemd: split off systemd-tmpfiles into separate module
38d043d
@bobvanderlinden
nixos: systemd-tmpfiles: use cfg shorthand
753b911
@bobvanderlinden bobvanderlinden force-pushed the pr-refactor-systemd-module branch from 7339fbc to 753b911 Compare March 17, 2022 22:17
@bobvanderlinden
Copy link
Member Author

bobvanderlinden commented Mar 17, 2022 •
edited
Loading

Unfortunately I couldn't make the build-result unchanged, because the order of upstreamSystemUnits and upstreamUserUnits changed (due to merging the lists). The order of those lists shouldn't affect functionality.

I ran for each of the commits to make sure they succeed:

nix-build nixos/release.nix -A tests.systemd.x86_64-linux

@cole-h
Copy link
Member

cole-h commented Mar 18, 2022

cc @NixOS/systemd

@mweinelt mweinelt requested review from Mic92, flokli and kloenk March 18, 2022 03:31
flokli
flokli reviewed Mar 18, 2022
View reviewed changes
@bobvanderlinden bobvanderlinden requested a review from flokli March 19, 2022 11:07
@bobvanderlinden bobvanderlinden mentioned this pull request Mar 19, 2022
Merged
13 tasks
@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Mar 20, 2022

I would really like to see this merge. It's been helpful in my systemd-initramfs effort

@ElvishJerricco ElvishJerricco mentioned this pull request Mar 20, 2022
Merged
13 tasks
@arianvp
Copy link
Member

arianvp commented Mar 20, 2022

@GrahamcOfBorg test systemd

@arianvp
Copy link
Member

arianvp commented Mar 20, 2022

Could we squash the systemd-user bits into a single commit or split it off into a new PR? Given it adds new functionaltiy it's a bit hard to follow that it's interwoven with plain refactorings.

Apart from that LGTM

dasJ
dasJ reviewed Mar 20, 2022
View reviewed changes
Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

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

Changes look great, apart from my (somewhat opinionated) minor style nits ;)

I'd be happy if this went through without my concerns being adressed, so I'm only commenting here and not requesting changes.

@bobvanderlinden @dasJ
nixos: systemd: remove unhelpful comments in additionalUpstreamSystem…
92af44e 
…Units

Co-authored-by: Janne Heß <janne@hess.ooo>
@bobvanderlinden
Copy link
Member Author

bobvanderlinden commented Mar 20, 2022 •
edited
Loading

I would really like to see this merge. It's been helpful in my systemd-initramfs effort

@ElvishJerricco Review approval would help 😄 👍

dasJ
dasJ approved these changes Mar 20, 2022
View reviewed changes
Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

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

LGTM. If nobody brings up arguments why I shouldn't, I'd merge this tomorrow or so

mweinelt
mweinelt reviewed Mar 20, 2022
View reviewed changes
mweinelt
mweinelt approved these changes Mar 20, 2022
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Looks sane enough. Tested on my desktop and I didn't notice any regression.

@bobvanderlinden @mweinelt
nixos: systemd-user: fix additionalUpstreamUserUnits description
397b825 
Co-authored-by: Martin Weinelt <mweinelt@users.noreply.github.com>
@flokli flokli merged commit 9427a17 into NixOS:master Mar 21, 2022
@bobvanderlinden
Copy link
Member Author

bobvanderlinden commented Mar 21, 2022

🥳 thank you all!

@erikarvstedt erikarvstedt mentioned this pull request Apr 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mweinelt mweinelt mweinelt approved these changes

@flokli flokli flokli approved these changes

@arianvp arianvp arianvp approved these changes

@dasJ dasJ dasJ approved these changes

@cole-h cole-h cole-h approved these changes

@Artturin Artturin Awaiting requested review from Artturin

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@Mic92 Mic92 Awaiting requested review from Mic92

@kloenk kloenk Awaiting requested review from kloenk

@ElvishJerricco ElvishJerricco Awaiting requested review from ElvishJerricco

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@bobvanderlinden @nixos-discourse @cole-h @ElvishJerricco @arianvp @mweinelt @flokli @dasJ @SuperSandro2000 @Artturin