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

systemd: 243.7 -> 245 #85334

Merged
merged 13 commits into from
Apr 19, 2020
Merged

systemd: 243.7 -> 245 #85334

merged 13 commits into from
Apr 19, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Apr 15, 2020

Motivation for this change

This bumps systemd to its latest stable release.

As explained in #80038, we moved away from a custom systemd fork in the NixOS organization to some NixOS-specific patches inside nixpkgs.

Maintaining custom branches in a fork turned out to be very error-prone. Obsolete patches accidentially got re-introduced, maintaining things in the custom fork required write access to that repo, syncing back and forth with a corresponding nixpkgs PR, and the patches themselves became very unmanage-able as well. For example, some of the patches were partial reverts of earlier patches, not improving in getting the total patch count down ;-)

This PR intentionally isn't squashed together, as it makes it understandable how patches were folded together / became obsolete. I removed the comments originally introduced in 0e66f6b - these comments should still be acted upon in a followup PR.

I ran quite some VM tests, and switched systemd.package on my laptop to 90f9b47 (a version of this against master). So far, this seems to work, and should probably receive some broader testing :-)

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.

@Ma27 Ma27 self-requested a review April 15, 2020 22:01
@ofborg ofborg bot requested a review from Mic92 April 15, 2020 22:03
@flokli flokli force-pushed the systemd-mainline2 branch 2 times, most recently from 6197fc5 to 00854a8 Compare April 15, 2020 22:26
@flokli
Copy link
Contributor Author

flokli commented Apr 15, 2020

Note that a lot of stuff currently doesn't build on staging - I see gstreamer as a qemu dependency failing on some bash completions… Until this is sorted out, I propose testing this against master.

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Did some testing in the past days based on these changes. So far no regression that I could find. 👍

@Mic92
Copy link
Member

Mic92 commented Apr 16, 2020

Sorry if I cannot use git anymore for patch management I don't want to maintain this anymore as it is too much work to fix those patches manually.

@ofborg ofborg bot requested a review from andir April 16, 2020 04:10
@flokli
Copy link
Contributor Author

flokli commented Apr 16, 2020

Sorry if I cannot use git anymore for patch management I don't want to maintain this anymore as it is too much work to fix those patches manually.

The patches are ordered alphabetically.

The idea is that you can still git am /nixpkgs/pkgs/os-specific/systemd/*.patch to import all these patches at once into git, then take care of rebasing, reordering, squashing etc, and replay back via git format-patch. This might produce some renaming noise if the gaps in the patch numbers are closed, but this should be fine.

I didn't do that while folding patches together, as in fact I mostly dropped patches and it made the diffs more readable, and for the bump from 244 to 245, touching the patches at all wasn't required, as the (reduced amount of) patches just applied.

#80038 explains why using patches instead of "magic git revs pushed somewhere" makes things easier".

On top of that, the amount of patches is only going to get down - there's quite some comments about ways to upstream more of these patches on the original wip branch (flokli@90f9b47)

I had some similar discussions with @andir who also didn't really like the patches at first, but I could convince him as long as the "import and export from and to git" stays a simple git am path/to/*.patch. 😃

@worldofpeace
Copy link
Contributor

Note that a lot of stuff currently doesn't build on staging - I see gstreamer as a qemu dependency failing on some bash completions… Until this is sorted out, I propose testing this against master.

Hmm, I should probably look into that. cc @jtojnar wasn't there a bash completions update?

@jtojnar
Copy link
Member

jtojnar commented Apr 16, 2020

@andir
Copy link
Member

andir commented Apr 16, 2020

Here is what I've come up with as things we still have to figure out before merging this:

  • networkd does not support networkctl reconfigure, is that sufficient for config changes to networkd? When do we still have to restart it?
  • how does the new homed work? Can/Should we disable it when we do not properly use it?

@Mic92
Copy link
Member

Mic92 commented Apr 16, 2020

Sorry if I cannot use git anymore for patch management I don't want to maintain this anymore as it is too much work to fix those patches manually.

The patches are ordered alphabetically.

The idea is that you can still git am /nixpkgs/pkgs/os-specific/systemd/*.patch to import all these patches at once into git, then take care of rebasing, reordering, squashing etc, and replay back via git format-patch. This might produce some renaming noise if the gaps in the patch numbers are closed, but this should be fine.

Fair enough. I feel that I am a bit out of the loop with systemd updates. As we seem to have active group carrying about systemd I will probably step back from this one and focus on other areas.

@flokli
Copy link
Contributor Author

flokli commented Apr 16, 2020

@andir

  • I assume this should have been a "does support" - I wouldn't want to start using networkctl reconfigure just now, but leave that to a future PR
  • good point, we might want to disable homed in meson until someone checked if it works and wrote a VM test for it, to ensure it keeps working

@Mic92 fair enough. Take care, and thanks for your work so far :-)

@arianvp
Copy link
Member

arianvp commented Apr 16, 2020

On a similar note portabled should probably be disabled for now. I can confirm it has not worked in v243 on NixOS

However i'm also open for just keeping them both there; so people can experiment with it and break things with a low barrier :)

@flokli flokli force-pushed the systemd-mainline2 branch from 807fb26 to 2098825 Compare April 16, 2020 18:26
@flokli
Copy link
Contributor Author

flokli commented Apr 16, 2020

I rebased on top of latest staging (hoping staging has been fixed in the meantime).

Bumped to 245.3 and explicitly disabled homed and portabled. Let's re-enable this once we have tests and can ensure it works, instead of shipping halfway broken pieces.

@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Apr 16, 2020
@ofborg ofborg bot requested a review from Mic92 April 16, 2020 18:36
flokli and others added 10 commits April 17, 2020 00:27
These patches removed logic in the meson install phase invoking
`journalctl --update-catalog` and `systemd-hwdb update`, which would
mutate the running system, and obviously fails in the sandbox.

Upstream also knows this is a bad thing if you're not on the machine you
want to deploy to, so there's logic in there to not execute it when
DESTDIR isn't empty. In our case, it is - as we set --prefix instead for
other reasons, but by just setting DESTIDIR to "/", we can still trigger
these things to be skipped.

The patches removed some context from
0018-Install-default-configuration-into-out-share-factory.patch, which
we need to introduce there to make that patch still apply.
…patch

This was simply undoing a hunk from
0008-Don-t-try-to-unmount-nix-or-nix-store.patch, so drop that one from
there and omit
0017-Fix-mount-option-x-initrd.mount-handling-35268-16.patch entirely.
The previous patch just removed a `ConditionFileNotEmpty=…` line from
`kmod-static-nodes.service` referring to a location not existing on
NixOS. We know better, and can actually replace this Condition to point
to `run/booted-system/kernel-modules/lib/modules/%v/`, instead of just
patching it out.
This required some changes in how we treat DEFAULT_PATH_NORMAL.
We don't currently have tests to ensure it works and keeps working.

So instead of having it accidentially working, and possibly breaking it
in the future, disable it for now.
This hasn't worked with 243, let's disable it for now, until we have
tests and can ensure it works and keeps working.
@flokli flokli force-pushed the systemd-mainline2 branch from 3a1f3f7 to b3f1410 Compare April 16, 2020 22:31
@flokli
Copy link
Contributor Author

flokli commented Apr 16, 2020

I added some more comments on the commit message and the DESTDIR=/ thingie, to make it more understandable by just looking at the commit messages in nixpkgs.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

First of all thanks a lot for the hard work! I just updated my local systemd to v245 by setting systemd.package to pkgs.systemd from this PR's base branch.

I didn't have any issues when switching to the new configuration, systemd re-execed itself without any problems. Also (re)booting into a NixOS configuration with systemd v245 worked perfectly fine.

I'll keep using this systemd version for at least a few days now to ensure that I don't stumble upon any issues.


However I guess I found some kind of regression:

in my sway config I run systemctl --user import-environment and systemctl --user start graphical-session.target after that.

However, the nextcloud-client.service from home-manager (which is triggered by the graphical-session.target that is started from the sway-config) breaks since it's missing needed env-vars like $WAYLAND_DISPLAY.

As an ugly, temporary workaround, I'm now running exec systemctl --user import-environment && sleep 2 before starting the graphical-session.target which appears to fix the issue. Unfortunately this isn't an issue that just happened to occur once, I rebooted several times and the issue was completely reproducible.

Is this explanation sufficient for you or do you need e.g. a VM declaration to reproduce this? :)

pkgs/os-specific/linux/systemd/default.nix Show resolved Hide resolved
@flokli
Copy link
Contributor Author

flokli commented Apr 17, 2020

I'm currently not using sway, still on X :-)

Reading up on https://wiki.archlinux.org/index.php/Systemd/User#Environment_variables, I'd assume this might be some sort of racyness, nextcloud-client.service starting before the environment was imported. Could it be systemctl --user import-environment forks into the background? Does systemctl --user --wait import-environment work?

I don't really understand why it works in my X setup - network-manager-applet.service starts, so I assume something sets DISPLAY… In my case, it might be the login manager setting this even before invoking the systemd user session…

@Ma27
Copy link
Member

Ma27 commented Apr 17, 2020

Hmm, didn't know about the --wait flag. Will try this out tomorrow, thanks! :)

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

So long story short, after posting my last comment I realized that I had to reboot for a kernel update anyway and I decided to try using systemctl --user --wait import-environment which is working perfectly fine now -> big 👍 from my side :)

@arianvp
Copy link
Member

arianvp commented Apr 17, 2020

All my points were mostly bikesheds. which shouldn't be adressed now. From my point this seems ready to merge

@flokli
Copy link
Contributor Author

flokli commented Apr 17, 2020

Hmm, didn't know about the --wait flag.

Maybe worth updating the archlinux wiki to include a note like that, if sway with a systemd user session is usually run like this.

Copy link
Member

@picnoir picnoir left a comment

Choose a reason for hiding this comment

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

I've been running 245.3 for 3 days on my main system without running into any problems.

LGTM.

@Ma27
Copy link
Member

Ma27 commented Apr 19, 2020

Decided in #nixos-systemd that this is good to go now 👍

@Ma27 Ma27 merged commit 19de59a into NixOS:staging Apr 19, 2020
@flokli flokli deleted the systemd-mainline2 branch April 21, 2020 19:29
@lheckemann lheckemann mentioned this pull request Apr 30, 2020
@flokli flokli mentioned this pull request Jun 6, 2020
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants