Skip to content

Avoid Two Sets of systemd Binaries in System Closure#261798

Merged
flokli merged 4 commits intoNixOS:stagingfrom
blitz:systemd-minimization
Oct 24, 2023
Merged

Avoid Two Sets of systemd Binaries in System Closure#261798
flokli merged 4 commits intoNixOS:stagingfrom
blitz:systemd-minimization

Conversation

@blitz
Copy link
Copy Markdown
Contributor

@blitz blitz commented Oct 18, 2023

Description of changes

We currently end up with two systemds in every system closure. One is the one you want and the second one is systemdMinimal. This includes the full set of systemd binaries, but it's only there to provide libudev and libsystemd. This wastes around 10 MiB of space.

I'm not sure that what I've done here is a good idea. Consider it a PoC.

Also see #261795.

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/)
  • 23.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. label Oct 18, 2023
@blitz blitz requested review from a team and RaitoBezarius October 18, 2023 10:34
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should probably go into pkgs/development/tools/build-managers/meson/setup-hook.sh.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something like mesonInstallFlags?

@arianvp
Copy link
Copy Markdown
Member

arianvp commented Oct 18, 2023

I like the idea of this change. I think we can even delete systemdMinimal now. It was only there for breaking the dependency loop between systemd -> lvm2 -> udev. If the systemdLibs package has no circular dependency on lvm2 then this is a much more elegant solution

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Oct 18, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Oct 18, 2023
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something like mesonInstallFlags?

@blitz blitz force-pushed the systemd-minimization branch from a65b958 to 99f4d2a Compare October 19, 2023 10:42
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Oct 19, 2023
@blitz blitz changed the base branch from master to staging October 19, 2023 10:43
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 19, 2023
@blitz blitz force-pushed the systemd-minimization branch from 99f4d2a to 6f05322 Compare October 19, 2023 12:05
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. and removed 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 19, 2023
@blitz blitz force-pushed the systemd-minimization branch from 6f05322 to 3083cb9 Compare October 20, 2023 08:54
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The headers where ### and #### for no apparent reason. I fixed them to be consistent.

@blitz blitz marked this pull request as ready for review October 20, 2023 10:29
@blitz blitz requested a review from AndersonTorres October 20, 2023 11:01
@blitz
Copy link
Copy Markdown
Contributor Author

blitz commented Oct 20, 2023

@arianvp

I think we can even delete systemdMinimal now.

The problem is that some places actually use this as a "minimal systemd" and not as "libudev". :(

@AndersonTorres
Copy link
Copy Markdown
Member

The problem is that some places actually use this as a "minimal systemd" and not as "libudev". :(

Like an alias?

@AndersonTorres
Copy link
Copy Markdown
Member

You wrote on the commit message:

Projects building with meson are currently installTargets. Map these to install tags, which are roughly equivalent. This allows projects to selectively install components.

A not so long time ago I came across a nasty bug: I treated wafConfigureFlags as roughly equivalent to configureFlags. It caused a problem because some configureFlags are not recognized by waf.

#254111

Since then I do not recommend the reuse of flags.

So, I suggest an exclusive name for this: mesonInstallTags.

Also, in tandem I will open a refactor PR of Meson, specifically capturing this issue.

@flokli
Copy link
Copy Markdown
Member

flokli commented Oct 20, 2023

The problem is that some places actually use this as a "minimal systemd" and not as "libudev". :(

Then this should be fixed :-)

@blitz
Copy link
Copy Markdown
Contributor Author

blitz commented Oct 20, 2023

@AndersonTorres

So, I suggest an exclusive name for this: mesonInstallTags.

Also, in tandem I will open a refactor PR of Meson, specifically capturing this issue.

If you do this, that'd be great. I can also change it here for now.

@blitz
Copy link
Copy Markdown
Contributor Author

blitz commented Oct 20, 2023

@flokli

The problem is that some places actually use this as a "minimal systemd" and not as "libudev". :(

Then this should be fixed :-)

This PR sort of does this. systemdMinimal is now the minimal systemd people hope for and for dependency breaking there is systemdLibs.

@blitz blitz force-pushed the systemd-minimization branch from 3083cb9 to 0cebd40 Compare October 22, 2023 23:43
blitz added 4 commits October 23, 2023 01:46
Projects building with meson are currently installTargets. Map these
to install tags, which are roughly equivalent. This allows projects to
selectively install components.
systemdMinimal is used to break the dependency cycle of libraries
required by systemd on itself. The problem is that this means that
every system closure will have two sets of systemd binaries in the
closure. This wastes around 10 MiB in every system closure.

Add the option to systemd to only ship libudev and libsystemd instead
of a whole systemd installation.
pcsclite only depends on libsystemd and libudev.
@blitz blitz force-pushed the systemd-minimization branch from 0cebd40 to dd97082 Compare October 22, 2023 23:47
@blitz
Copy link
Copy Markdown
Contributor Author

blitz commented Oct 22, 2023

@AndersonTorres I've resolved the issues. Do you think it makes sense to fix existing uses of systemdMinimal in this PR (use systemdLibs where appropriate) or would this be better in a subsequent PR?

@AndersonTorres
Copy link
Copy Markdown
Member

Another PR is better.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 23, 2023
@flokli flokli merged commit 87896bf into NixOS:staging Oct 24, 2023
@blitz blitz deleted the systemd-minimization branch October 25, 2023 16:51
@blitz blitz mentioned this pull request Oct 29, 2023
13 tasks
udev =
if (with stdenv.hostPlatform; isLinux && isStatic) then libudev-zero
else systemd; # TODO: change to systemdMinimal
else systemdLibs;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This broke some things that expected udev to have executables. See e.g. #268813.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@blitz thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arianvp Let's revert this change then. Long-term I think the better solution is to create a real udev package with libs and binaries and all. The systemd build system makes this hard though. I have to study how Fedora does this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was done as an alternative: #268718

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. That also makes sense.

If it still comes to reverting this, please leave the mesonInstallTags change in place. I think this is genuinely useful elsewhere.

@arianvp
Copy link
Copy Markdown
Member

arianvp commented Nov 21, 2023

Darnit. I think we should revert this PR. A lot of places in nixpkgs assume udevadm to exist in the udev package

If you search for udevadm there are a lot of hits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants