meson: add support for building static libsystemd and libudev #8689
Conversation
meson_options.txt
Outdated
| @@ -26,6 +26,12 @@ option('rootprefix', type : 'string', | |||
| description : '''override the root prefix''') | |||
| option('link-udev-shared', type : 'boolean', | |||
| description : 'link systemd-udev and its helpers to libsystemd-shared.so') | |||
| option('static-libsystemd', type : 'combo', | |||
| choices : ['true', 'false', 'pic', 'no-pic'], value : 'false', | |||
There was a problem hiding this comment.
We generally just put the default first in the choices list, and do not specify it explicitly (mostly to make this all a bit shorter).
There was a problem hiding this comment.
nitpick: Having "true" and "false" together with other options sounds a lot like using a boolean as a tri-state
Can we have a name this option differently (not really sure what to suggest) so that the choices could be "shared", "static", "pic" and "no-pic" instead? I think that would be slightly cleaner...
There was a problem hiding this comment.
It's not a choice between "shared" and "static", this option only governs the static library. We could split this in two options: -Dstatic-libsystemd=true|false, -Dstatic-libsystemd-pic=true|false, to avoid, but I don't think it would be any better. I think it's totally OK to have multiple "true" values.
src/libsystemd/meson.build
Outdated
| pic : pic, | ||
| dependencies : [threads, | ||
| librt], | ||
| c_args : c_args) |
There was a problem hiding this comment.
Hmm, does this mean that the meson pic option does not work on its own and requires flags to be twiddled manually? You don't do this in the other library, can you comment on this?
There was a problem hiding this comment.
Correct, what I've observed (using meson 0.44.1) is that if c_args is being set explicitly, I have to make sure it includes -fno-PIC for the non-PIC build to be done properly. For libudev we aren't setting c_args, so this isn't needed.
src/udev/meson.build
Outdated
| 'udev.h', | ||
| include_directories : includes, | ||
| link_with : udev_link_with, | ||
| link_whole : libudev_basic) | ||
|
|
||
| static = get_option('static-libudev') |
There was a problem hiding this comment.
Please make the name of the variable match the name of the option, i.e. static_libudev. Also the same for the other variable.
src/udev/meson.build
Outdated
| install : true, | ||
| install_dir : rootlibdir, | ||
| pic : pic) | ||
| endif |
There was a problem hiding this comment.
I'd like this simplified by removing the conditional (here and for the other one too):
static_libudev = get_option('static-libudev')
install_libudev_static = static_library(
'udev',
'udev.h',
include_directories : includes,
link_with : udev_link_with,
link_whole : libudev_basic,
build_by_default : static_libudev != 'false',
install : static_libudev != 'false',
install_dir : rootlibdir,
pic : static_libudev == 'true' or static == 'pic')This has the advantage that the static library can be build by explicit call
$ ninja -C build src/udev/libudev.a
ninja: Entering directory `build'
[1/1] Linking static target src/udev/libudev.a.We do that with man pages, which can be built even if -Dman=false, and its useful for testing and debugging and such.
keszybz
added
the
reviewed/needs-rework 🔨
|
I've pushed an update addressing the review comments, but found a potentially significant issue. It looks like meson will only include objects that have been explicitly listed in sources in the |
|
On a related note, it looks like meson upstream is doing some work to better support mixed libraries (i.e. libraries built as both static and shared at the same time) in mesonbuild/meson#2711 and mesonbuild/meson#2816. I gave these a quick test, and while they do make things clearer, the sources issue still remains. |
|
The journal client code was missing because it is added separately. I force-pushed a version which should avoid this issue. PTAL. |
keszybz
removed
the
reviewed/needs-rework 🔨
|
Thanks @keszybz! This seems to work from a quick check, though I had to add the |
|
This is working fine, and all our stuff builds and runs correctly against it. We might be able to improve it in the future, but for now I think it's good to go. |
|
Looks like the same issue we had with journal_client happens with basic as well. I force pushed a new version and validated that libsystemd is built correctly and works as expected here, but I'm having trouble linking against the resulting libudev (it looks like symbols from libbasic aren't picked up, despite being included in the |
|
Can you attach the test case you're using? |
|
Please see the fixup commit: I think some bits were missing. |
|
Even with the static versions of the libraries available, for me the so I think you're right, we should probably just disable the tests in the |
|
As a dumb testcase (unfortunately teasing a real one out of our internal build system is hard), try something like: This is for |
|
I get the same failure with rm -f src/udev/libudev.a && gcc-ar csrD src/udev/libudev.a 'src/udev/udev@sta/.._libudev_libudev.c.o' 'src/udev/udev@sta/.._libudev_libudev-list.c.o' 'src/udev/udev@sta/.._libudev_libudev-util.c.o' 'src/udev/udev@sta/.._libudev_libudev-device.c.o' 'src/udev/udev@sta/.._libudev_libudev-device-private.c.o' 'src/udev/udev@sta/.._libudev_libudev-enumerate.c.o' 'src/udev/udev@sta/.._libudev_libudev-monitor.c.o' 'src/udev/udev@sta/.._libudev_libudev-queue.c.o' 'src/udev/udev@sta/.._libudev_libudev-hwdb.c.o'so there's no attempt to add the objects from Based on https://stackoverflow.com/questions/2157629/linking-static-libraries-to-other-static-libraries and https://sourceware.org/binutils/docs-2.30/binutils/ar-scripts.html, I tried the following: $ cd build
$ ln -s udev@sta src/udev/udev_sta # ar doesn't like @ in filenames
$ gcc-ar -M <<__EOF
CREATE src/udev/libudev.a
ADDLIB src/basic/libbasic.a
ADDLIB src/basic/libbasic-gcrypt.a
ADDLIB src/udev/libudev-basic.a
ADDLIB src/libsystemd/libsystemd.a
ADDMOD src/udev/udev_sta/.._libudev_libudev.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-list.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-util.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-device.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-device-private.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-enumerate.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-monitor.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-queue.c.o
ADDMOD src/udev/udev_sta/.._libudev_libudev-hwdb.c.o
SAVE
END
__EOF
$ cp src/udev/libudev.a /tmp/systemd-static
$ gcc udev_example.c -L/tmp/systemd-static -ludev -lmountThis works and the link succeeds. This is nice and efficient, but doing this manually is too ugly. Maybe if meson was smart enough to do this automatically. Another option would be to just compile all the sources twice. |
|
I came up with the following POC. Dunno, this works, and the same could be made to work for libsystemd.a, but I don't know if it's worth the trouble. diff --git a/src/shared/meson.build b/src/shared/meson.build
index d0cb38650b..e3b076122b 100644
--- a/src/shared/meson.build
+++ b/src/shared/meson.build
@@ -2,7 +2,7 @@
#
# Copyright 2017 Zbigniew Jędrzejewski-Szmek
-shared_sources = '''
+shared_sources = files('''
acl-util.h
acpi-fpdt.c
acpi-fpdt.h
@@ -104,25 +104,25 @@ shared_sources = '''
watchdog.c
watchdog.h
wireguard-netlink.h
-'''.split()
+'''.split())
test_tables_h = files('test-tables.h')
shared_sources += [test_tables_h]
if conf.get('HAVE_ACL') == 1
- shared_sources += ['acl-util.c']
+ shared_sources += files('acl-util.c')
endif
if conf.get('ENABLE_UTMP') == 1
- shared_sources += ['utmp-wtmp.c']
+ shared_sources += files('utmp-wtmp.c')
endif
if conf.get('HAVE_SECCOMP') == 1
- shared_sources += ['seccomp-util.c']
+ shared_sources += files('seccomp-util.c')
endif
if conf.get('HAVE_LIBIPTC') == 1
- shared_sources += ['firewall-util.c']
+ shared_sources += files('firewall-util.c')
endif
libshared_name = 'systemd-shared-@0@'.format(meson.project_version())
diff --git a/src/udev/meson.build b/src/udev/meson.build
index 613a2e24cd..8fa0627d00 100644
--- a/src/udev/meson.build
+++ b/src/udev/meson.build
@@ -121,16 +121,16 @@ static_libudev = get_option('static-libudev')
static_libudev_pic = static_libudev == 'true' or static_libudev == 'pic'
install_libudev_static = static_library(
'udev',
+ basic_sources,
+ shared_sources,
+ libsystemd_sources,
libudev_sources,
include_directories : includes,
- link_with : [libshared_static,
- libsystemd_static],
- link_whole : libudev_basic,
build_by_default : static_libudev != 'false',
install : static_libudev != 'false',
install_dir : rootlibdir,
link_depends : libudev_sym,
- dependencies : [threads],
+ dependencies : libshared_deps + [libmount],
c_args : static_libudev_pic ? [] : ['-fno-PIC'],
pic : static_libudev_pic)
|
|
Thanks @keszybz! Using your POC I was able to get libsystemd and libudev to build correctly -- as far as I can tell the resulting libraries work fine now. |
This is the same as test-lib{systemd,udev}-sym, but linked to the static
variants of those libraries.
…ibsystemd_static This means that when those targets are built, all the sources are built again, instead of reusing the work done to create libbasic.a and other convenience static libraries. It would be nice to not do this, but there seems to be no support in our toolchain for joining multiple static libraries into one. When linking a static library, any -l arguments are simply ignored by ar/gcc-ar, and .a libraries given as positional arguments are copied verbatim into the archive so they objects in them cannot be accessed. https://stackoverflow.com/questions/2157629/linking-static-libraries-to-other-static-libraries suggests either unzipping all the archives and putting them back togather, or using a linker script. Unzipping and zipping back together seems ugly. The other option is not very nice. The linker script language does not allow "+" to appear in the filenames, and filenames that meson generates use that, so files would have to be renamed before a linker script was used. And we would have to generate the linker script on the fly. Either way, this doesn't seem attractive. Since those static libraries are a niche use case, it seems reasonable to just go with the easiest and safest solution and recompile all the source files. Thanks to ccache, this is probably almost as cheap as actually reusing the convenience .a libraries. test-libsystemd-sym.c and test-libudev-sym.c compile fine with the generated static libs, so it seems that they indeed provide all the symbols they should.
|
I force-pushed to your branch with my POC patch, now with support for both static libs. PTAL. |
|
lgtm, we've been using a 238 backport of this for a few weeks now and it's working well, so it should be good to go. Thanks again for all your help with this! |
|
OK, let's merge then. This shouldn't have any impact for anyone not using the static libs. |
This PR adds optional (and disabled by default) support for building and installing static library versions of
libsystemdandlibudev, in PIC and non-PIC versions. While static build for systemd as whole obviously doesn't make much sense, I believe offering static versions of these two libraries is reasonable, as they're what expose the external interface provided by systemd and what other programs will link to. We're using static builds of these at Facebook to integratelibsystemdandlibudevwith a number of internally-built tools, as it's been working fine; I'd love to get this upstream in some form so we don't have to continue carrying a patch for it.