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

Dynamically load compression libraries #31550

Merged
merged 3 commits into from Mar 5, 2024

Conversation

teknoraver
Copy link
Contributor

Dynamically load compression libraries (LZ4, ZSTD, LZMA) so we can reduce the size of the initram images by omitting libraries which aren't really used.

@github-actions github-actions bot added build-system util-lib tests meson please-review PR is ready for (re-)review by a maintainer labels Feb 29, 2024
@YHNdnzj
Copy link
Member

YHNdnzj commented Feb 29, 2024

I'd really like to know if we can replace all these with libarchive first (see also #31131 (comment))

@poettering
Copy link
Member

I'd really like to know if we can replace all these with libarchive first (see also #31131 (comment))

hmm, so we link against these libraries in journald for example to compress individual fields, does it really make sense to involve libarchive in that?

does libarchive support uncompressing blobs that aren't tarballs/zipfiles?

i kinda like the fact that if we make the individual compression libs dlopen-deps we truly minimize the dep tree, because people can pick one compressor only. After all libarchive itself does not employ dlopen() afaics.

I mean:

 lddtree /usr/lib64/libarchive.so
libarchive.so => /usr/lib64/libarchive.so (interpreter => none)
    libcrypto.so.3 => /lib64/libcrypto.so.3
    libacl.so.1 => /lib64/libacl.so.1
        libattr.so.1 => /lib64/libattr.so.1
    liblzma.so.5 => /lib64/liblzma.so.5
    libzstd.so.1 => /lib64/libzstd.so.1
    liblz4.so.1 => /lib64/liblz4.so.1
    libbz2.so.1 => /lib64/libbz2.so.1
    libz.so.1 => /lib64/libz.so.1
    libxml2.so.2 => /lib64/libxml2.so.2
        libm.so.6 => /lib64/libm.so.6
            ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2
    libc.so.6 => /lib64/libc.so.6

Hence I am very much inclined to merging this PR.

In particular libbz2 I think is something we should try to get out of the deptree. By using libarchive for everyting we'd still link to it indirectly.

src/basic/meson.build Outdated Show resolved Hide resolved
src/test/meson.build Outdated Show resolved Hide resolved
src/test/meson.build Outdated Show resolved Hide resolved
src/basic/compress.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

lgtm, but some issues, see above.

And I guess the CIs need some updating before we can merge this, because the deps are now implicit rather than explicit.

@mrc0mmand any chance you can look into that?

@poettering
Copy link
Member

Please also add an extra commit that deletes/updates the relevant lines in the TODO file in the toplevel dir. the one containing "bzip2, xz, lz4"

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Mar 1, 2024
@YHNdnzj
Copy link
Member

YHNdnzj commented Mar 1, 2024

does libarchive support uncompressing blobs that aren't tarballs/zipfiles?

Yes, see archive_write_set_format_raw.

i kinda like the fact that if we make the individual compression libs dlopen-deps we truly minimize the dep tree, because people can pick one compressor only. After all libarchive itself does not employ dlopen() afaics.

Indeed. However, as discussed in the other PR, libkmod depends on some of those libs anyway too. And bzip2 is required by things like gnupg/ffmpeg/...

While it would be great if bzip2 can be eliminated from the whole system, that won't happen any time soon. So instead of adding more dlopen()-ed deps, which are hard to manage, it seems more practical to me to reduce the number of direct deps first.

@YHNdnzj
Copy link
Member

YHNdnzj commented Mar 1, 2024

IOW, the reduced dependency tree isn't really observable by users, in which case adding more code and introducing more packaging/CI changes really sounds like a pure burden.

@poettering
Copy link
Member

Indeed. However, as discussed in the other PR, libkmod depends on some of those libs anyway too. And bzip2 is required by things like gnupg/ffmpeg/...

well, the focus here is really on building tiny systems for initrds for example. I don't see why you'd need gunpg/ffmpeg/… in an initrd...

i mean, sure, on a full blown desktop they will end up int he full install, but even then, i see no reason they need to be in the initrd.

While it would be great if bzip2 can be eliminated from the whole system, that won't happen any time soon. So instead of adding more dlopen()-ed deps, which are hard to manage, it seems more practical to me to reduce the number of direct deps first.

but libarchive itself doesn#t need to be in the initrd, no?

src/test/test-compress.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Mar 1, 2024
@teknoraver
Copy link
Contributor Author

Should I call dlopen_* functions also in decompress_startswith_* functions?

This could be the cause of systemd:libsystemd/test-journal-enum failing since they're are referenced in src/libsystemd/sd-journal/journal-file.c

@teknoraver teknoraver force-pushed the dlopen_compress branch 2 times, most recently from 7c5a90d to 0631df0 Compare March 1, 2024 22:29
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

test-dlopen-so.c should be modified as well to test the new dlopen functions

src/basic/compress.c Show resolved Hide resolved
src/test/test-compress.c Show resolved Hide resolved
src/test/test-compress.c Outdated Show resolved Hide resolved
src/basic/compress.c Show resolved Hide resolved
src/basic/compress.c Show resolved Hide resolved
src/basic/compress.c Show resolved Hide resolved
@eworm-de
Copy link
Contributor

eworm-de commented Mar 5, 2024

Wondering if we need changes for the initrd generation now... Are these libraries required at early boot?

@poettering
Copy link
Member

Wondering if we need changes for the initrd generation now... Are these libraries required at early boot?

I think you might get away without them for now.

@aafeijoo-suse
Copy link
Contributor

Is this failing only for me? shouldn't libdl be added to the libbasic deps?

[495/2818] Linking target libsystemd.so.0.38.0
FAILED: libsystemd.so.0.38.0 
cc  -o libsystemd.so.0.38.0  -Wl,--as-needed -Wl,--no-undefined -shared -fPIC -Wl,--start-group -Wl,-soname,libsystemd.so.0 -Wl,--whole-archive src/libsystemd/libsystemd_static.a -Wl,--no-whole-archive -fstack-protector src/basic/libbasic.a src/basic/libbasic-gcrypt.a -shared -Wl,--version-script=/mnt/work/src/systemd/upstream-fork/main/src/libsystemd/libsystemd.sym -lrt -pthread -Wl,--fatal-warnings -Wl,-z,now -Wl,-z,relro -Wl,--warn-common /usr/lib64/libcap.so -lm -Wl,--end-group -Wl,--fatal-warnings -Wl,-z,now -Wl,-z,relro -Wl,--warn-common -Wl,--fatal-warnings -Wl,-z,now -Wl,-z,relro -Wl,--warn-common -Wl,--fatal-warnings -Wl,-z,now -Wl,-z,relro -Wl,--warn-common
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: src/basic/libbasic.a.p/dlfcn-util.c.o: in function `safe_dlclose':
/mnt/work/src/systemd/upstream-fork/main/build/../src/basic/dlfcn-util.h:12:(.text+0x22): undefined reference to `dlclose'
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: src/basic/libbasic.a.p/dlfcn-util.c.o: in function `dlsym_many_or_warnv':
/mnt/work/src/systemd/upstream-fork/main/build/../src/basic/dlfcn-util.c:18:(.text+0xf1): undefined reference to `dlsym'
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: /mnt/work/src/systemd/upstream-fork/main/build/../src/basic/dlfcn-util.c:20:(.text+0x15e): undefined reference to `dlerror'
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: src/basic/libbasic.a.p/dlfcn-util.c.o: in function `dlopen_many_sym_or_warn_sentinel':
/mnt/work/src/systemd/upstream-fork/main/build/../src/basic/dlfcn-util.c:47:(.text+0x3a4): undefined reference to `dlopen'
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: /mnt/work/src/systemd/upstream-fork/main/build/../src/basic/dlfcn-util.c:49:(.text+0x42c): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

yuwata added a commit to yuwata/systemd that referenced this pull request Mar 6, 2024
@yuwata
Copy link
Member

yuwata commented Mar 6, 2024

@aafeijoo-suse Maybe #31652 fixes your issue?

@aafeijoo-suse
Copy link
Contributor

@aafeijoo-suse Maybe #31652 fixes your issue?

Yes, definitely. I had already tried it :)

@yuwata
Copy link
Member

yuwata commented Mar 6, 2024

@aafeijoo-suse Maybe #31652 fixes your issue?

Yes, definitely. I had already tried it :)

Sorry for interrupting your work :-p

@aafeijoo-suse
Copy link
Contributor

Sorry for interrupting your work :-p

No problem at all! Thanks for fixing it so quickly.

yuwata added a commit to yuwata/systemd that referenced this pull request Mar 6, 2024
@teknoraver teknoraver deleted the dlopen_compress branch March 6, 2024 12:49
bluca pushed a commit that referenced this pull request Mar 6, 2024
n3rdopolis pushed a commit to n3rdopolis/systemd that referenced this pull request Mar 22, 2024
ayhamthemayhem pushed a commit to neighbourhoodie/nh-systemd that referenced this pull request Mar 25, 2024
@bluca bluca linked an issue Mar 31, 2024 that may be closed by this pull request
nemobis added a commit to nemobis/systemd that referenced this pull request Apr 1, 2024
nemobis added a commit to nemobis/systemd that referenced this pull request Apr 1, 2024
@nemobis nemobis mentioned this pull request Apr 1, 2024
chunyi-wu pushed a commit to chunyi-wu/systemd that referenced this pull request Apr 3, 2024
aafeijoo-suse added a commit to aafeijoo-suse/dracut that referenced this pull request Apr 4, 2024
…y included

Some required libraries that used to be statically included are in the process
to be opened via `dlopen()`.

References:
- systemd/systemd#31131
- systemd/systemd#31550
- systemd/systemd#32019

Closes dracutdevs#2642
aafeijoo-suse added a commit to aafeijoo-suse/dracut that referenced this pull request Apr 4, 2024
…y included

Some required libraries that used to be statically included are in the process
to be opened via `dlopen()`.

References:
- systemd/systemd#31131
- systemd/systemd#31550
- systemd/systemd#32019
aafeijoo-suse added a commit to aafeijoo-suse/dracut that referenced this pull request Apr 4, 2024
…y included

Some required libraries that used to be statically included are in the process
to be opened via `dlopen()`.

References:
- systemd/systemd#31131
- systemd/systemd#31550
- systemd/systemd#32019
dracutng pushed a commit to dracutng/dracut-ng that referenced this pull request Apr 6, 2024
…y included

Some required libraries that used to be statically included are in the process
to be opened via `dlopen()`.

References:
- systemd/systemd#31131
- systemd/systemd#31550
- systemd/systemd#32019

Closes #2642
Conan-Kudo pushed a commit to dracut-ng/dracut-ng that referenced this pull request Apr 8, 2024
…y included

Some required libraries that used to be statically included are in the process
to be opened via `dlopen()`.

References:
- systemd/systemd#31131
- systemd/systemd#31550
- systemd/systemd#32019

Closes #2642
@mrc0mmand mrc0mmand added login and removed logind labels Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Reduce dependencies of libsystemd
8 participants