systemd: build libudev.so "separately" #97051

vcunat · 2020-09-03T22:00:34Z

Motivation for this change

breaking a problematic dependency cycle: nixos-unstable is blocked because of failing luks-format1 test #96479
perhaps even closure savings

Things done

…xOS#96197" This reverts commit f6286de.

This reverts commit e02793d.

vcunat · 2020-09-03T22:02:05Z

So far this is just a quick proof of concept, and in fact tests.installer.lvm.x86_64-linux won't work and I don't know why yet.

vcunat · 2020-09-03T22:07:15Z

Note to self: libudev.so build is more tied to systemd than I expected (did I glimpse that it links statically against libsystemd?), so an approach like this might be more maintainable than really trying it to separate from our systemd expression. Of course, without too much work we could additionally reduce the build-time closure (partially) and build time (try instructing ninja to build just specific targets).

Mic92 · 2020-09-04T05:38:13Z

I think it would be easier to build libudev/libsystemd with ninja and than re-use those libraries when building systemd (replacing it with a symlink in postInstall?). Here is an example where I do it for systemd-networkd: https://github.com/Mic92/dotfiles/blob/711f8d5e20fb543267ed0620b9ee0c139675ef27/nixos/modules/networkd.nix#L11

vcunat · 2020-09-04T06:40:21Z

I do plan that, but so far there are problems with something else apparently.

arianvp · 2020-09-04T07:25:33Z

We'd have to split systemd and libsystemd first before we can do the systemd and udev split..

vcunat · 2020-09-04T07:53:55Z

Can you explain why?

Mic92 · 2020-09-04T07:55:08Z

Libudev seems to not depend on libsystemd.

Mic92 · 2020-09-04T07:57:49Z

But having a separate libsystemd would be also helpful in longterm because it might be easier to patch for musl libc rather than the whole systemd code base.

vcunat · 2020-09-04T08:02:09Z

To be clear, it does not depend on dynamic libsystemd. I'm not sure why they link stuff this way ATM: https://github.com/systemd/systemd/blob/v246/src/udev/meson.build#L145

vcunat · 2020-09-04T08:33:37Z

Yes, this attempted to separate just the library.

arianvp · 2020-09-04T12:29:49Z

Ignore my ramblings. I thought it was because of udev linking to libsystemd dynamically.

For an initial effort; I wouldn't do too much fancy things; and just keep libudev statically linked to libsystemd. Though I admit it's a bit strange. We could ask upstream why they do this.

If we're building just the libudev target; how large is the output size? Is it significantly smaller than systemd as a whole? Because of not; then this is basically the same as the current PR no? It will come down to still having systemd in the closure twice.

vcunat · 2020-09-04T12:59:30Z

Roughly 100x smaller. EDIT: and as written now even libudev isn't twice in there, I think.

arianvp · 2020-09-04T13:02:47Z

But the cycle will be back If you set pkgs.udev to the libudev package. Which I assume is what we eventually want.

However having two udevs (one with and one without cryptsetup) still is a lot better than 2 systemds...

Mic92 · 2020-09-04T13:08:27Z

To be clear, it does not depend on dynamic libsystemd. I'm not sure why they link stuff this way ATM: https://github.com/systemd/systemd/blob/v246/src/udev/meson.build#L145

It seems that libudev was built with a static libsystemd before anyway so this is not big regression size wise. And I still think it should be possible to bring back this libudev in the systemd package by putting a symlink in the package from "$lib/lib/libudev.*" to libudev. Or is the issue that cryptsetup depends on libudev and the other way around?

flokli

I'd feel more comfortable if udev would be its own derivation, in a separate file (and not just an override), only building the udev-related targets (and we pass that into the systemd expression).

Both could use the same src and patches, though.

flokli · 2020-09-04T15:57:39Z

pkgs/os-specific/linux/systemd/default.nix

+      [ linuxHeaders libcap curl.dev kmod xz pam acl
+        libuuid glib libgcrypt libgpgerror libidn2
+        pcre2 ] ++
+        stdenv.lib.optional withKexectools kexectools ++
+        stdenv.lib.optional withLibseccomp libseccomp ++
+      [ libffi audit lz4 bzip2 libapparmor
+        iptables gnu-efi
+      ] ++ stdenv.lib.optional withSelinux libselinux;


Most of these probably are not dependencies of just libudev, are they?

Yes, I was planning to do such reductions after the change actually works.

(Now it appears to work fine.)

I tried to start with empty mkDerivation instead of overriding the systemd one, but I couldn't come with anything simple. Just making the meson configure pass isn't easy – it requires lots of stuff that's useless for libudev but I see no easy way of turning these off (*Inputs, mesonFlags, etc.) – honestly it feels that maintainability will be easiest when those two derivations only have minimal differences (full systemd and libudev).

flokli · 2020-09-04T16:01:45Z

While doing so, we should also remove the udev = systemd; "alias", which would exercise the derivation we built a bit (for example for libfido2)

andersk · 2020-09-05T07:28:44Z

I suspect that splitting a libudev derivation from the udev+systemd derivation solves problems that splitting a libudev+udev derivation from the systemd derivation does not. The former split means the closure has two compatible versions of libudev.so.1 (and we can replace one with a symlink to the other), but the latter split means the closure has two incompatible versions of libsystemd-shared-246.so.

We apparently didn't fit anymore. I don't think this test is meant to (also) check closure size.

flokli · 2020-09-05T09:00:32Z

@vcunat libudev is in aliases.nix, because it was used at some point (and changed to an alias in April 2018) Nothing inside nixpkgs itself is using it at this point (it'd fail ofborg eval checks too).

I'm talking about the

udev = systemd; # TODO: move to aliases.nix

in all-packages.nix (at around L18299).

We currently use systemd and udev attributes pointing to the identical derivation, which is bad. We don't really know what really needs full systemd, and what only needs the udev bits.

Instead of moving this to aliases.nix (as suggested by the comment), and by this, having to replace udev with systemd everywhere, I'd rather like to see the udev attribute being a proper derivation providing a libudev.so in its udev.lib output (and *udev.pc and libudev.h in the udev.dev output) .

Of course we'd need to verify packages still build after that change (as things like lvm2 also want to see the *systemd.pc file, not the *udev.pc file - which might be an upstream bug).

In the longer run, having packages who only really need udev to declare udev as a build input seems much more desirable, both for closure sizes as well as cyclic dependency problems.

I don't think resurrecting libudev from aliases.nix, and using it inside nixpkgs as well as the systemd and udev attribute is desirable.

@andersk I don't really understand your comment. Why couldn't we (re-)symlink libudev.so.1 inside the systemd derivation to the udev.lib location? Why would we need this in first place? Other applications wanting to link against libudev.so could just ask pkgconfig, and get a location from the udev.lib output back?
If we need to still provide symlinks, why would they be incompatible?

On the other side, why do we need to provide libsystemd-shared-246.so in any udev output? Reading #97051 (comment), ninja might need to intermediately compile libsystemd_static to produce libudev.so.1, but the libsystemd.a file shouldn't need to be in the udev output, should it?

vcunat · 2020-09-05T09:10:14Z

This PR provides libudev.pc. There's also udev.pc and I don't see why it should be provided here, though maybe it wouldn't be hard (I haven't looked). That's why pointing udev attribute to this derivation seems confusing to me.

The *.pc files are generally quite indicative of what makes sense to be split (during runtime), and having aliases based on those seem perfectly fine to me, so that packages can express what they need even for derivations that are not split (yet)... though it's perhaps better to avoid cases where we find it very unlikely to split them in the following couple of years.

flokli · 2020-09-05T09:18:19Z

We /can't/ use aliases.nix, as it'd fail grahamcofborg-eval-package-list-no-aliases - they're only meant for backwards compatibility for thing referring to it outside of nixpkgs (IMHO a questionable thing anyways, as semantics might change too).

Creating a udev = systemd; alias outside aliases.nix doesn't really provide a lot of value IMHO - we effectively don't know it's wrong, and whether packages really need systemd or udev.

Given the dependency cycle has been resolved for now with #97008, and we don't need to rush this, can we give this some more research?

I could envision udev.dev shipping both udev.pc and libudev.pc, with udev binaries in udev.bin/udev.out, and libudev in udev.lib.

vcunat · 2020-09-05T09:22:05Z

I could see that... but as the derivations are written so far, to get udev.pc you basically need to build the systemd attribute, and you can't use that as a build-time dependency of lvm2 (for example).

flokli · 2020-09-05T09:24:01Z

I could see that... but as the derivations are written so far, to get udev.pc you basically need to build the systemd attribute, and you can't use that as a build-time dependency of lvm2 (for example).

Yeah, as written here, the udev and systemd derivations would need to be much more separated (and only share src and patches)

vcunat · 2020-09-05T09:26:40Z

we don't need to rush this

For 20.09 I think it would be preferable to have some improvement in about one week. (than the current solution with two systemds in most closures)

flokli · 2020-09-05T09:48:23Z

It's a 28MB closure size increase, yes. But it's probably fine to ship 20.09 even with that.

Splitting udev and systemd is something that probably takes more time than we have to properly fix it before the 20.09 release, and I don't think the current proposed approach improves matters from a maintainability factor. IMHO, we should just go with the closure size increase for now, work on a proper fix in unstable, and if the fix is not too invasive, it can probably be backported (but I think it does require quite some changes in many packages).

andersk · 2020-09-05T10:00:09Z

@andersk I don't really understand your comment. Why couldn't we (re-)symlink libudev.so.1 inside the systemd derivation to the udev.lib location? Why would we need this in first place? Other applications wanting to link against libudev.so could just ask pkgconfig, and get a location from the udev.lib output back?
If we need to still provide symlinks, why would they be incompatible?

Perhaps you are reading the opposite of what I wrote? I said exactly that we can make such a symlink because the two libudev.so.1s are compatible. This is why it does makes sense to split off a libudev derivation.

This libudev.so.1 symlink would also work with a udev derivation, but other things don’t:

On the other side, why do we need to provide libsystemd-shared-246.so in any udev output? Reading #97051 (comment), ninja might need to intermediately compile libsystemd_static to produce libudev.so.1, but the libsystemd.a file shouldn't need to be in the udev output, should it?

The systemd-udevd and udevadm binaries link against libsystemd-shared-246.so—you can see this with ldd. AFAIK, the libsystemd-shared-246.so from a udev derivation would not be compatible with the libsystemd-shared-246.so from a systemd derivation. (Unless we gave it the same build inputs as systemd, but then we can’t break the cycle, which I thought was the whole point.) This is why it does not make sense to split off a udev derivation.

(Sorry if the emphasis and repetition comes across as patronizing…there are certainly things I could be wrong about here, but I want to make sure we’re communicating correctly so we can figure out where that might be.)

flokli · 2020-09-05T21:10:48Z

@andersk right - splitting systemd-udevd from systemd won't probably work, and not sure how udevadm interacts with systemd (and whether it's worth digging into it).

So after a second thought, calling this other derivation libudev would make sense - but it should really be its own derivation (only sharing src and patches with the systemd expression) - and we should check all current udev=systemd buildInputs in other packages, to see if those can be replaced with libudev.

vcunat · 2020-09-05T21:22:24Z

As I wrote above, it would be nice for it to be "independent" derivation, but I can't see how to do that. The configure phase isn't easy to satisfy, nor easy to cut down to just libudev. Or at least I didn't find such a way; maybe someone else will have an idea how to approach it.

vcunat · 2020-09-05T21:32:04Z

🤔 this PR actually seems very close to the currently merged workaround, only with the binaries "deduplicated" (i.e. anything but libudev deleted from the first result, and libudev replaced in the second result).

Not only does nixos not have a split systemd.lib package, it also gets two copies of systemd in the image: NixOS/nixpkgs#97051 NixOS/nixpkgs#98094 Let's not include it for now, the only detraction is that the systemd/journald logger wont be available in uwsgi.

Not only does nixos not have a split systemd.lib package, it also gets two copies of systemd in the image: NixOS/nixpkgs#97051 NixOS/nixpkgs#98094 Let's not include it for now, the only detraction is that the systemd/journald logger wont be available in uwsgi. On the good side, the image went from 50MB to 30MB now.

Not only does nixos not have a split systemd.lib package, it also gets two copies of systemd in the image: NixOS/nixpkgs#97051 NixOS/nixpkgs#98094 Let's not include it for now, the only detraction is that the systemd/journald logger wont be available in uwsgi. On the good side, the image went from 50MB to 30MB now. The image is built against the `my-20.09` branch of nixpkgs, that is based on the release-20.09 branch with uwsgi fixes added.

gdamjan · 2020-09-18T12:07:53Z

The Arch package has a relatively simple approach to this, in its split package for systemd/systemd-libs.

It boils down to these two lines, that copy the libraries that are then shipped in systemd-libs package:
https://github.com/archlinux/svntogit-packages/blob/packages/systemd/trunk/PKGBUILD#L177-L179

vcunat · 2020-09-18T12:27:16Z

Well, yes... other distros don't have this strict automatic detection of references (cycle between systemd and some of its libs) and they can keep all output in the shared /usr/ tree, so it's easier for them to do such things.

flokli · 2020-09-25T14:03:49Z

@vcunat would a systemdMinimal, as proposed in #98299 solve this sufficiently, as in, is the closure size acceptable?

Given you apparently need dbus to build systemd, which needs systemd itself, a libudev.so only wouldn't get us that far in the longer run anyways…

arianvp · 2020-09-25T16:36:32Z

Given you apparently need dbus to build systemd

@flokli
Looking at the systemd source code, the libdbus dependency is optional and only used in the test suite. Given that we don't build the test-suite it seems we can just remove the dependency completely.

stale · 2021-06-04T08:56:12Z

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

flokli · 2021-08-04T09:16:36Z

I think the systemdMinimal approach, which we already ship for some time is sufficient. Reopen if you disagree ;-)

vcunat added 3 commits September 3, 2020 21:36

Revert "extra-utils: build a full lvm2 without udev support, fixes Ni…

06ffcdc

…xOS#96197" This reverts commit f6286de.

Revert "nixos installer tests: add a missing package"

7b84572

This reverts commit e02793d.

WIP: systemd: build libudev.so "separately"

9a87663

vcunat mentioned this pull request Sep 3, 2020

cryptsetup, lvm2, systemd: Break cyclic dependency at a different point #97008

Merged

10 tasks

ofborg 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 Sep 3, 2020

ofborg bot requested review from ajs124, andir, edolstra, 7c6f434c and flokli September 3, 2020 23:13

ofborg bot added 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Sep 3, 2020

flokli reviewed Sep 4, 2020

View reviewed changes

nixos/tests/installer swraid: increase partition size

9dff9ef

We apparently didn't fit anymore. I don't think this test is meant to (also) check closure size.

vcunat mentioned this pull request Sep 16, 2020

systemd depends on different derivation of systemd #98094

Closed

flokli mentioned this pull request Sep 25, 2020

systemd: enable homed #98299

Merged

10 tasks

ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020

infinisil added the 6.topic: closure size The final size of a derivation, including its dependencies label Oct 29, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021

flokli closed this Aug 4, 2021

systemd: build libudev.so "separately" #97051

systemd: build libudev.so "separately" #97051

Conversation

vcunat commented Sep 3, 2020

Motivation for this change

Things done

vcunat commented Sep 3, 2020

vcunat commented Sep 3, 2020 • edited Loading

Mic92 commented Sep 4, 2020 • edited Loading

vcunat commented Sep 4, 2020

arianvp commented Sep 4, 2020

vcunat commented Sep 4, 2020

Mic92 commented Sep 4, 2020

Mic92 commented Sep 4, 2020

vcunat commented Sep 4, 2020

vcunat commented Sep 4, 2020

arianvp commented Sep 4, 2020 • edited Loading

vcunat commented Sep 4, 2020 • edited Loading

arianvp commented Sep 4, 2020 • edited Loading

Mic92 commented Sep 4, 2020 • edited Loading

flokli left a comment

Choose a reason for hiding this comment

flokli Sep 4, 2020

Choose a reason for hiding this comment

vcunat Sep 4, 2020

Choose a reason for hiding this comment

vcunat Sep 5, 2020 • edited Loading

Choose a reason for hiding this comment

flokli commented Sep 4, 2020

andersk commented Sep 5, 2020

flokli commented Sep 5, 2020

vcunat commented Sep 5, 2020 • edited Loading

flokli commented Sep 5, 2020

vcunat commented Sep 5, 2020

flokli commented Sep 5, 2020

vcunat commented Sep 5, 2020

flokli commented Sep 5, 2020

andersk commented Sep 5, 2020 • edited Loading

flokli commented Sep 5, 2020

vcunat commented Sep 5, 2020 • edited Loading

vcunat commented Sep 5, 2020

gdamjan commented Sep 18, 2020

vcunat commented Sep 18, 2020 • edited Loading

flokli commented Sep 25, 2020

arianvp commented Sep 25, 2020 • edited Loading

stale bot commented Jun 4, 2021

flokli commented Aug 4, 2021

vcunat commented Sep 3, 2020 •

edited

Loading

Mic92 commented Sep 4, 2020 •

edited

Loading

arianvp commented Sep 4, 2020 •

edited

Loading

vcunat commented Sep 4, 2020 •

edited

Loading

arianvp commented Sep 4, 2020 •

edited

Loading

Mic92 commented Sep 4, 2020 •

edited

Loading

vcunat Sep 5, 2020 •

edited

Loading

vcunat commented Sep 5, 2020 •

edited

Loading

andersk commented Sep 5, 2020 •

edited

Loading

vcunat commented Sep 5, 2020 •

edited

Loading

vcunat commented Sep 18, 2020 •

edited

Loading

arianvp commented Sep 25, 2020 •

edited

Loading