systemd: improve our downstream patch situation

[flokli]

Currently, our release process of a new systemd version is quite complicated.

This is mostly due to the fact that we apply a lot of patches on top of systemd upstream (currently 27!).
We currently track those in a custom https://github.com/nixos/systemd fork of systemd, which is very hard to maintain.

• A separate repo needs separate commits there, very few people can push branches there, and PRs aren't really right
• It's hard to keep track with systemd-stable, and we don't want to rewrite history on pushed branches either.
• Legitimately removed patches (nixos: remove dependencies on local-fs.target #67858, ) accidentially got reintroduced (systemd: make sysinit.target depend on local-fs.target again #69284)
• It's way too many patches in general!

Ideally, we'd end up with like 4-5 downstream patches, have upstreamed the others in a more generic fashion, or just handled differently.

I pushed a WIP of that effort to https://github.com/flokli/nixpkgs/commits/systemd-mainline, would be happy to see some helping hands.

cc @andir @edolstra @Mic92 @arianvp

[danbst]

Another one systemd/systemd#14884

[flokli]

Please don't open upstream issues for every NixOS-specific patch we have here - it's really on us to integrate those ;-)

I had some discussion with systemd devs some time ago, most of the things should already be doable with systemd provides today, or could be more generic patches.

There's some comments on some of the patches, happy to provide more context here.

[danbst]

@flokli I agree patches situation has to be improved

Could you split patches into "potentially upstreamable" and "no go"? For example, this one NixOS/systemd@ce79214 won't be accepted by systemd (I think).

[flokli]

@danbst This patch has already been dropped in the branch linked above, with an explanation.

I'll try to compile a list of the remaining ones and describe what'd need to be done there, but might not get to it this weekend.

[flokli]

I added some notes on the systemd-mainline branch. Will also ask upstream about some of their opinions.

[flokli]

The commits in the mentioned branch received quite some testing. I opened a PR against staging (with the comments removed) at #85334, PTAL.

[flokli]

Once that PR has gone through, I'll clean up and post the comments from the systemd-mainline branch here, so we can discuss about upstreaming what's possible.

[arianvp]

FYI; I am currently running a systemd that only requires 1 patch in path-util.h:

https://github.com/arianvp/server-optimised-nixos/blob/master/overlays/systemd.nix

Biggest difference is that I install all systemd build outputs in their expected locations; and actually include all systemd units by default:
https://github.com/arianvp/server-optimised-nixos/blob/master/modules/systemd.nix#L251-L254

I then disable the units I don't need in NixOS config:
https://github.com/arianvp/server-optimised-nixos/blob/master/modules/stage-1.nix#L84-L114

This is a slight inversion of what we have in NixOS; where we have an explicit allow-list; instead of just setting enable = false on units that are unused. But it leads to a cleaner approach that requires no patches to upstream's build system

I'll see how usable this is in NixOS tree.

[andir]

Arian: can you explain the downsides of your current version and why we shouldn't just move to the much simpler expression? I guess generators etc. are not working / aren't being discovered?

…

On Thu, Aug 6, 2020, 11:25 Arian van Putten ***@***.***> wrote: FYI; I am currently running a systemd that only requires 1 patch in path-util.h: https://github.com/arianvp/server-optimised-nixos/blob/master/overlays/systemd.nix https://github.com/arianvp/server-optimised-nixos/blob/master/modules/stage-1.nix#L84-L114 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80038 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE365CHAOGS2RTBZUF4Z5DR7JZJVANCNFSM4KUYCWEQ> .

[arianvp]

No downsides as of yet. It's just not complete yet. Indeed generators are the next item on my list.

[flokli]

I finally found took the time to extract the comments on our patches, and update them:

0001-Start-device-units-for-uninitialised-encrypted-devic.patch

This only removes a single udev rule from 99-systemd.rules.
I don't fully understand what this was supposed to fix, and also heard rumours that's also an upstream discussion.
Given NixOS' systemd recently gained cryptsetup support, it should certainly be revisited (if it's still applicable)

0002-Don-t-try-to-unmount-nix-or-nix-store.patch

This adds /nix and /nix/store to the list of "extrinsic" and nonumountable
paths. It seems similar behaviour could be archieved if /nix and /nix/store
would be present in /etc/fstab with a x-initrd option - needs to be verified, though.

0003-Fix-NixOS-containers.patch

The commit message says that NixOS containers bind-mount the init script into
the container by systemd-nspawn, and some check routine inside nspawn checks
for it (or is it /etc/os-release?) being present before doing the bind mount.

It's not entirely certain if this was a bug once and got fixed in the meantime.
If it isn't fixed yet, it should probably be fixed upstream, and if checking
for bind mounts would become too complicated, upstream would be fine with some
sort of a command line argument to disable that check.

Simply patching these checks out entirely is not what we should do.

0004-Look-for-fsck-in-the-right-place.patch

There have been quite some refactorings going on recently in systemd w.r.t
discovering mkfs.*/mkswap from $PATH.

Given these calls are dispatched through systemd-makefs (and we could add an
override unit file setting PATH), we should be able to fix finding these
tools, without adding it to the runtime closure of the package itself.

Same could apply for fsck, which seems to be dispatched via
systemd-fsck@.service - So we could just send a PR upstream to make this use
find_executable as well - and set PATH in an override unit.

0005-Add-some-NixOS-specific-unit-directories.patch

Some of the removals here are mostly insignificant "performance optimizations"
we don't necessarily need to do - especially given we're applying them
consistently across the whole codebase (and docs).

The addition of /nix/var/nix/profiles/default/lib/systemd/{user,system} would
need to be kept to allow for using systemctl to manage packages installed via
nix-env -i (even though I personally find it a bit odd, especially on NixOS,
where most units shipped with packages need some tweaking anyways, and
configuring them declaratively through the module system seems much easier.

The Dysnomia-specific path /etc/systemd-mutable/system should probably be
moved into a separate derivation, which can be set by systemd.package.

Maybe both Dysnomia and nix-env -i scenarios be solved more elegantly by
some activation script that essentially just places symlinks in
/run/systemd/system as well - to be investigated.

0006-Get-rid-of-a-useless-message-in-user-sessions.patch

This seems to workaround some (superfluous? annoying?) log lines about
mountpoints below /nix being shown.

We'd need to check if that's a side-effect of
0002-Don-t-try-to-unmount-nix-or-nix-store.patch, or something useful
upstream too.

0007-hostnamed-localed-timedated-disable-methods-that-cha.patch

This lets above tools immediately bail out if you want to change something that
might be specified through NixOS configuration, and read-only files.

Most of the code afterwards should still provide somewhat meaningful error
messages about files being read-only (and if it doesn't, it should be fixed upstream)

Once the upstream code does handle it nicely, we could drop this patch.

0008-Fix-hwdb-paths.patch

Removes a bunch of (FHS) lookup paths from systemd, not sure about what it
fixed.
There might have been a circular dependency between out and lib outputs.

0009-Change-usr-share-zoneinfo-to-etc-zoneinfo.patch

Would need to be kept, of some sort, given we don't assemble things in /usr/share.

Interestingly, this also patches documentation, contrary to
0005-Add-some-NixOS-specific-unit-directories.patch.

We might be able to upstream a patch making /usr/share/zoneinfo configurable
from meson, and then just set option to /etc/zoneinfo in mesonFlags.

0010-localectl-use-etc-X11-xkb-for-list-x11.patch

Same here, changes {/usr/share -> /etc}/X11/xkb/rules/base.lst.
Probably adding a meson flag upstream is also possible.

0011-build-don-t-create-statedir-and-don-t-touch-prefixdi.patch

This could probably become unnecessary by setting DESTDIR to an empty string.
Also, ask upstream: why is this part of the build system, and not created on boot?

0012-Install-default-configuration-into-out-share-factory.patch

This should probably just be DESTDIR. TODO: follow-up with @Mic92 and @andir

0013-inherit-systemd-environment-when-calling-generators.patch

Maybe I misunderstand something else from what this is doing, but this could
probably be solved by setting PATH in override units for systemd-makefs and
friends as explained in 0004-Look-for-fsck-in-the-right-place.patch.

0014-add-rootprefix-to-lookup-dir-paths.patch

We'd probably need to keep this patch. It'd be good to see what else relies on
it, though.

0015-systemd-shutdown-execute-scripts-in-etc-systemd-syst.patch

This might be useful upstream as well, and once it's added to the docs could be
easily upstreamed.

0016-systemd-sleep-execute-scripts-in-etc-systemd-system-.patch

Same as 0015-systemd-shutdown-execute-scripts-in-etc-systemd-syst.patch.
Easy upstreamable.

0017-kmod-static-nodes.service-Update-ConditionFileNotEmp.patch

Pretty NixOS-specific.

0018-path-util.h-add-placeholder-for-DEFAULT_PATH_NORMAL.patch

I'd love for this to be configurable from meson, but it seems systemd currently
is pretty much tied to FHS paths, so as long as
there's no upstream push on making this more configurable, we probably need to
ship that patch downstream.

[arianvp]

About patch 0009:

This discussion in the systemd-devel mailing list seems related:

https://lists.freedesktop.org/archives/systemd-devel/2020-September/045265.html

[doronbehar]

Some comments regarding patch 0005:

The addition of /nix/var/nix/profiles/default/lib/systemd/{user,system} would need to be kept to allow for using systemctl to manage packages installed via nix-env -i (even though I personally find it a bit odd, especially on NixOS, where most units shipped with packages need some tweaking anyways, and configuring them declaratively through the module system seems much easier.

Maybe I misunderstand the intention of the patch, but I think doesn't work. For instance, try:

nix-env -iA mpd-mpris
systemctl --user daemon-reload
systemctl --user cat mpd-mpris.service

Gives the error:

No files found for mpd-mpris.service.

Where:

ls -l ~/.nix-profile/lib/systemd/user/

Shows the service file is there. But, if you:

ln -s ~/.nix-profile/lib/systemd/user ~/.local/share/systemd
systemctl --user daemon-reload

I.e follow this, suddenly this:

systemctl --user cat mpd-mpris.service

Prints the service file content, as expected.

I'm currently documenting the behavior of current Systemd in NixOS in #98661 , and I think there's more information to put there, and I don't see any evidence that this patch does something, please correct me if I'm wrong. If I'm not, I think it'd be best to remove the patch and give better documentation instead.

The Dysnomia-specific path /etc/systemd-mutable/system should probably be moved into a separate derivation, which can be set by systemd.package.

I have never heard of the path - /etc/systemd-mutable/system, is it only me?

Maybe both Dysnomia and nix-env -i scenarios be solved more elegantly by some activation script that essentially just places symlinks in /run/systemd/system as well - to be investigated.

nix-env doesn't have activation scripts right?

---

Besides that, I agree with all of the statements regarding upstreaming patches. 💚 @flokli.

[jasom]

@doronbehar

systemctl --user show -p UnitPath --value lists /home/aidenn/.nix-profile/share/systemd/user under search paths; but that directory doesn't exist in my profile? I suspect something is confused between the user profile systemd configuration and packages that install user services. In 20.03 gnome3.evolution worked, but in 20.09 it required me to make a symlink as you suggested for it to work properly; not sure if something changed in systemd, evolution, or nixpkgs in that time period.

[flokli]

The lack of tests and documentation on this makes me wonder whether it's really widely used, and whether we shouldn't just drop it…

nix-env doesn't have activation scripts right?

No, but NixOS could provide some glue code activation script symlinking things from the system profile to /run/…, so we could remove that path from inside systemd.

[flokli]

@ttuegel, what's the status of #99621 and 0019-revert-get-rid-of-seat_can_multi_session.patch?

Has there been any discussion upstream that we could follow?

[flokli]

I did dig a bit further, apparently "switch user" not shown in the start bar is fixed in plasma-workspace upstream, and #99582 backports this fix.

"Switch User" not being visible in the lockscreen seems to have a similar cause, but we apparently never reported this upstream to plasma (or no-one did link to it at least, I don't know).

I'd much prefer applying a plasma patch that upstream already has in master over adding more patches to systemd.

And I feel like we also haven't reported back to systemd they accidentially made a public dbus method private (if that's what happened), and whether it can be reverted in a systemd-stable release.

What about (on unstable) dropping 0019-revert-get-rid-of-seat_can_multi_session.patch, re-rolling #99582, and sorting out any other fixes that might be necessary?

[stale]

I marked this as stale due to inactivity. → More info

[andir]

Not stale you stupid bot.

…

On 18 June 2021 23:44:42 CEST, "stale[bot]" ***@***.***> wrote: I marked this as stale due to inactivity. &rarr; [More info](https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #80038 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[stale]

I marked this as stale due to inactivity. → More info

[cole-h]

Still not stale :)

[RaitoBezarius]

Could we do a round-up again on our current patches?

[nikstur]

I plan to continue this effort. After we merge #265951, I'll extend/update on @flokli list on how/if we can remove these patches.

[nyabinary]

I plan to continue this effort. After we merge #265951, I'll extend/update on @flokli list on how/if we can remove these patches.

Any updates to this?