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 depends on different derivation of systemd #98094

Closed
gdamjan opened this issue Sep 16, 2020 · 20 comments
Closed

systemd depends on different derivation of systemd #98094

gdamjan opened this issue Sep 16, 2020 · 20 comments
Labels
0.kind: bug 0.kind: regression Something that worked before working no longer 6.topic: closure-size
Milestone

Comments

@gdamjan
Copy link
Contributor

gdamjan commented Sep 16, 2020

Describe the bug

Installing systemd pulls 2 different derivations of systemd

To Reproduce
Steps to reproduce the behavior:
1.

nix-env -iA systemd -f https://github.com/NixOS/nixpkgs/archive/release-20.09.tar.gz # or `master`
# ls -ld /nix/store/*systemd*
dr-xr-xr-x    3 root     root          4096 Jan  1  1970 /nix/store/jv6bw65cp3z782gy6iwwd6jsmnw8grwk-systemd-246-man
dr-xr-xr-x    7 root     root          4096 Jan  1  1970 /nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246
dr-xr-xr-x    7 root     root          4096 Jan  1  1970 /nix/store/lkzflryr02ahapqln1cwrkkhncwfb91y-systemd-246
# du -sh /nix/store/lkzflryr02ahapqln1cwrkkhncwfb91y-systemd-246 /nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246
27.5M   /nix/store/lkzflryr02ahapqln1cwrkkhncwfb91y-systemd-246
27.7M   /nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246
# nix path-info -r /nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246 | grep systemd
/nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246
/nix/store/lkzflryr02ahapqln1cwrkkhncwfb91y-systemd-246

Investigating further, it seems that libsystemd-shared depends on cryptsetup, which depends on devmapper which depends on udev from another systemd store. it can be confirmed by going up the library chain with ldd and readelf -d
5.

/nix/store/kmlzasz1lbkigpirk4vwyg14g206wq18-systemd-246/lib/systemd/libsystemd-shared-246.so
/nix/store/gn8cibqbzvdz7yqgg6har1p4598fc0bk-cryptsetup-2.3.3/lib/libcryptsetup.so.12
/nix/store/rglz8psiy8r8qhlpy747pf5xxdbyq1yd-lvm2-2.03.10-lib/lib/libdevmapper.so.1.02
/nix/store/lkzflryr02ahapqln1cwrkkhncwfb91y-systemd-246/lib/libudev.so.1

Additional context

Truth to be told, I'm running nixos in a podman container (podman run -it --rm nixos/nix:latest).

Notify maintainers

@flokli @edolstra

Metadata
Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

 - system: `"x86_64-linux"`
 - host os: `Linux 5.8.8-arch1-1, Alpine Linux, noversion`
 - multi-user?: `yes`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.3.6`
 - channels(root): `"nixpkgs-20.09pre228453.dcb64ea42e6"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixpkgs`
@vcunat
Copy link
Member

vcunat commented Sep 16, 2020

That's a known temporary tradeoff. See #97051 (and cross-links)

EDIT: 20.09 is affected, too.

gdamjan added a commit to gdamjan/tt-rss-service that referenced this issue Sep 16, 2020
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.
gdamjan added a commit to gdamjan/tt-rss-service that referenced this issue Sep 16, 2020
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.
gdamjan added a commit to gdamjan/tt-rss-service that referenced this issue Sep 16, 2020
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.
@FRidh FRidh added this to the 20.09 milestone Sep 18, 2020
@flokli
Copy link
Contributor

flokli commented Sep 20, 2020

This probably wouldn't be that bad if we could resurrect the separate lib output which had to be removed in 246 due to #94354 (comment).

We could probably end up patching some of the paths leading to the circular dependency to instead refer to something more mutable, maybe in /run/current-system, but I'm not sure if this might break during generation switching or systemd-in-initramfs scenarios and currently lack the time to think through this. Help welcome :-)

@flokli
Copy link
Contributor

flokli commented Oct 1, 2020

If we can't resurrect the .lib output, there would also be systemdMinimal from https://github.com/NixOS/nixpkgs/pull/98998/files#diff-036410e9211b4336186fc613f7200b12R18583.

@arianvp
Copy link
Member

arianvp commented Oct 1, 2020

My suggestion would be to patch sd_path_lookup_* in libsystemd to not refer to these paths that are causing trouble. So that we can reintroduce systemd.lib. The patch sounds rather non-invasive to me, given that most paths that sd_path_* exposes are not valid on NixOS anyway

Thoughts @edolstra @flokli ? I know we want to keep our patch count low, as it makes maintenance a lot more streamlined (thanks for the work on that @flokli. As a result we have a lot quicker updates). But this one sounds minimally invasive (just wrap the calls around an #ifdef with bogus implementations) and will solve a lot of headaches. This would make the patch almost always cleanly apply and I don't mind carrying it in our tree.

@flokli
Copy link
Contributor

flokli commented Oct 1, 2020

We would need to trace down what's using these paths. If systemd uses that logic internally aswell, this will be a problem. The problem is not the patch length, but the necessary investigation in what's using these codepaths.

@flokli
Copy link
Contributor

flokli commented Oct 1, 2020

Copying over flags from #99240.

@flokli flokli added 0.kind: regression Something that worked before working no longer 1.severity: blocker labels Oct 1, 2020
@arianvp
Copy link
Member

arianvp commented Oct 1, 2020

I personally don't have time to go through systemd source code and analyse the usages sd_path_lookup_* within the next week; and I'm a bit uncomfortable being a bottleneck blocking the entire release whilst nothing is functionally broken. Help is welcome if we find it important to fix this before release!

@arianvp
Copy link
Member

arianvp commented Oct 1, 2020

Note:

In the old situation libudev and libsystemd were packaged in the same package (udev is an alias of systemd so udev.lib is an alias of systemd.lib).

Introducing a systemd.libudev output would probably not be too hard as it's completely standalone (https://github.com/systemd/systemd/tree/master/src/libudev). Then at least the things that soley depend on udev.lib before don't pull in systemd in its entirety. This will reduce the runtime closure for many packages that only depend on libudev

This change will not require any patching of systemd and should be relatively easy to pull off.

@KamilaBorowska
Copy link
Member

KamilaBorowska commented Oct 1, 2020

I don't think this should block NixOS 20.09, it's 30MB of unnecessary bloat, and I think that's it. It would be nice to have this fixed, but it doesn't need to be fixed before release.

@edolstra
Copy link
Member

edolstra commented Oct 1, 2020

It's a lot more than that, e.g. 119 MiB for dbus or 85 for Emacs. We need to take closure size regressions more seriously. My system closure grows by 1-2 gigabytes per release which is not a sustainable trajectory for NixOS. And of course the situation is even worse for stuff like containers.

@arianvp
Copy link
Member

arianvp commented Oct 1, 2020

Ok. Could you bring these things up not on the day of of the go/no go but when we review and merge these PRs? You're marked as one of the maintainers of this package too.

The closure bloat is at best temporary. And we're very much aware of it We know how to tackle it. I just don't think postponing updates outweighs the benefits. So we made a call to merge it. Already quite a while ago. After quite a bit of discussion

Happy to review any PRs that address this.

I also don't understand how this PR would increase your NixOS closure. Systemd is always in the closure of NixOS. As systemd is a mandatory component of the system.

This is why I thought this was the right tradeoff for now.

@edolstra
Copy link
Member

edolstra commented Oct 2, 2020

NixOS is just one application of Nixpkgs, and not even necessarily the most widely used. People who use Nix outside of NixOS (e.g. for development shells or for building containers) are affected by having (multiple) systemd in the closure of everything.

@vcunat
Copy link
Member

vcunat commented Oct 2, 2020

CI and build farms (like Hydra.nixos.org) are another case where you usually have different versions than your host OS or perhaps even multiple versions at once (like our different jobsets), dragging all those dependencies between remote builders, caches, etc.

Introducing a systemd.libudev output would probably not be too hard [...]

One way of doing that wasn't too hard... well, Arianvp already saw the PR linked from my first comment here. (Just splitting an output wouldn't do by itself, due to eval-time cycle, at least at that time.)

@KamilaBorowska
Copy link
Member

KamilaBorowska commented Oct 2, 2020

I agree that this issue matters, I just don't think it should block NixOS 20.09 release as it is mostly irrelevant to NixOS. It increases the size of an installation image, however minimal installation CD can still be burned on CD-ROM if anyone still cares about CD-ROMs.

@edolstra
Copy link
Member

edolstra commented Oct 2, 2020

But NixOS 20.09 is also the stable branch of Nixpkgs for the next 6 months. So any Nix user will be affected by this issue until 21.03. (E.g. anybody running nix run nixpkgs#emacs will get a closure 85 MB larger than previously.)

@arianvp
Copy link
Member

arianvp commented Oct 2, 2020

Again; PRs are welcome if you consider this a major blocker. Neither I or @flokli have time to work on this the coming days. This PR has been in the 20.09 branch for over a month. and was opened 2 months ago.

As I said I don't see why we wouldn't backport a closure-bloat reduction PR, as already discussed in this ticket.. We won't change the major version of systemd; just change how it is packaged. Do you think that's an unacceptable solution? Why?

What is the alternative solution? Revert 20.09 branch back to 243 or 245? The patches will be rather tricky to revert as unstable went through multiple systemd versions between 20.03 and 20.09 (both 245 and 246) and I think there are new nixos modules that depend on behaviour introduced in recent systemd versions. But perhaps it's worth reverting to 245 which I think didn't have this cyclic dependency yet.

It would be unfortunate as I know there are some people like @andir who are waiting for IPv6 fixes that landed in 246

@arianvp
Copy link
Member

arianvp commented Oct 2, 2020

I have a WIP fix in #99382 . I don't have time to drive it forward; and I haven't tested it yet. but I think it should work. Please take a look @edolstra

@edolstra
Copy link
Member

edolstra commented Oct 2, 2020

Do you think that's an unacceptable solution? Why?

It's not necessarily unacceptable but it's higher risk than we'd usually like on a stable branch. It could cause unexpected build- or runtime breakage, e.g. a package expects a file in ${systemd.out} but now it's suddenly in ${systemd.lib}.

@flokli
Copy link
Contributor

flokli commented Nov 24, 2020

Based on #99382 (comment), I don't think we should try to play cat and mouse with upstream, at least until their refactoring is still ongoing.

There has been a some work ongoing recently in making our systemd expression somewhat more composable, and systemd-minimal isn't too bad anymore. We might be able to do some more yakshaving in other places to distinguish between libudev and libsystemd, but I'm not sure if this is worth the effort.

I propose closing this one, the current approach is somewhat safe, and much less annoying or confusing than it was while this issue was opened - and there's a clear distinction, as systemdMinimal as a different name, too.

@arianvp
Copy link
Member

arianvp commented Nov 24, 2020

I agree this can be closed for now. We changed the derivation name so people are less confused during rebuilds. In the future we might be able to get rid of systemdMinimal but for now it's here to stay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 0.kind: regression Something that worked before working no longer 6.topic: closure-size
Projects
Status: Done
Development

No branches or pull requests

7 participants