Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: add support for partially static builds #3039
Conversation
Some checks were not successful
| +# The goal is to link dynamically to libc and anything that talks to remote | ||
| +# things that we cannot control and that have a volatile protocol, specifically | ||
| +# this is udev. | ||
| +# |
pedronis
Mar 16, 2017
Contributor
I would turn around the phrasing here:
"""
This option controls whether snap-confine should be built largely statically.
The goal is to link dependencies statically except for libc and anything that talks to lower system components we cannot control via a volatile protocol, this is udev.
This is done so that ...
"""
I wonder also if we should s/partially/largely/ in the option and var
zyga
Mar 16, 2017
Contributor
I changed the code to have spearate knobs for enabling partially static and linking statically to each of the tree libs we care about here. I think the name is appropriate now.
pedronis
Mar 16, 2017
•
Edited 1 time
-
pedronis
Mar 16, 2017
Contributor
-
Mar 16, 2017
now I'm confused, why do you need the full general option if we have single ones? anyway the doc still needs improving and if it's the case explain that this needs to be combined with the specific lib options
niemeyer
Mar 17, 2017
Contributor
Agree with @pedronis. I think we can drop the PARTIALLY_STATIC_CORE_BUILD above altogether.
| @@ -54,7 +54,7 @@ endif | ||
| # security of the system. Discuss a proper approach to this for downstreams | ||
| # if and when they approach us | ||
| ifeq ($(shell dpkg-vendor --query Vendor),Ubuntu) | ||
| - VENDOR_ARGS=--enable-nvidia-ubuntu | ||
| + VENDOR_ARGS=--enable-nvidia-ubuntu --enable-partially-static |
pedronis
Mar 16, 2017
Contributor
mvo mentioned that 14.04 doesn't have a static seccomp, I think we are ok building all dynamic on 14.04, we don't make a core snap for it
Some checks were not successful
|
Aha, so we need static libcap on 14.04 anyway... |
Some checks were not successful
| @@ -178,5 +178,49 @@ AC_PATH_PROG([HAVE_VALGRIND],[valgrind]) | ||
| AM_CONDITIONAL([HAVE_VALGRIND], [test "x${HAVE_VALGRIND}" != "x"]) | ||
| AS_IF([test "x$HAVE_VALGRIND" = "x"], [AC_MSG_WARN(["cannot find the valgrind tool, will not run unit tests through valgrind"])]) | ||
| +# This knob controls if builds snap-confine are done in a pretty unusual way. |
| +# This knob controls if builds snap-confine are done in a pretty unusual way. | ||
| +# The goal is to link dynamically to libc and anything that talks to remote | ||
| +# things that we cannot control and that have a volatile protocol, specifically | ||
| +# this is udev. |
niemeyer
Mar 17, 2017
Contributor
And this looks like the wrong place to comment that. This is mechanics.. it should simply implement them.
A better place to describe the rationale would be debian/rules or wherever we actually use the flags.
| +# The goal is to link dynamically to libc and anything that talks to remote | ||
| +# things that we cannot control and that have a volatile protocol, specifically | ||
| +# this is udev. | ||
| +# |
pedronis
Mar 16, 2017
Contributor
I would turn around the phrasing here:
"""
This option controls whether snap-confine should be built largely statically.
The goal is to link dependencies statically except for libc and anything that talks to lower system components we cannot control via a volatile protocol, this is udev.
This is done so that ...
"""
I wonder also if we should s/partially/largely/ in the option and var
zyga
Mar 16, 2017
Contributor
I changed the code to have spearate knobs for enabling partially static and linking statically to each of the tree libs we care about here. I think the name is appropriate now.
pedronis
Mar 16, 2017
•
Edited 1 time
-
pedronis
Mar 16, 2017
Contributor
-
Mar 16, 2017
now I'm confused, why do you need the full general option if we have single ones? anyway the doc still needs improving and if it's the case explain that this needs to be combined with the specific lib options
niemeyer
Mar 17, 2017
Contributor
Agree with @pedronis. I think we can drop the PARTIALLY_STATIC_CORE_BUILD above altogether.
| @@ -47,7 +47,7 @@ endif | ||
| # security of the system. Discuss a proper approach to this for downstreams | ||
| # if and when they approach us | ||
| ifeq ($(shell dpkg-vendor --query Vendor),Ubuntu) | ||
| - VENDOR_ARGS=--enable-nvidia-ubuntu | ||
| + VENDOR_ARGS=--enable-nvidia-ubuntu --enable-partially-static --enable-static-libcap --enable-static-libapparmor --enable-static-libseccomp |
zyga
Mar 18, 2017
•
Edited 1 time
-
zyga
Mar 31, 2017
Contributor
-
Mar 31, 2017
Will do (not done yet). EDIT: done now :)
zyga
added some commits
Mar 18, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
| +endif # STATIC_LIBCAP | ||
| + | ||
| +# Use a hacked rule if we're doing static build. This allows us to inject the LIBS += .. rule below. | ||
| + |
pedronis
Mar 24, 2017
Contributor
seems the only place where you have this extra empty line after that comment
| @@ -54,7 +54,11 @@ endif | ||
| # security of the system. Discuss a proper approach to this for downstreams | ||
| # if and when they approach us | ||
| ifeq ($(shell dpkg-vendor --query Vendor),Ubuntu) | ||
| - VENDOR_ARGS=--enable-nvidia-ubuntu | ||
| + # On Ubuntu 14.04 snapd cannot add the libcap dependency because of a |
pedronis
Mar 24, 2017
Contributor
I don't understand the indentation here, it doesn't seem to align with the after the comment block
| + # variety of systems. As such we prefer static linking over dynamic linking | ||
| + # for stability, predicability and easy of deployment. We need to link some | ||
| + # things dynamically though: udev has no stable IPC protocol between | ||
| + # libudev and udevd so we need to link with it dynamically. |
|
some whitespace comments |
zyga
added some commits
Mar 28, 2017
Some checks were not successful
zyga
added this to the
2.24 milestone
Mar 31, 2017
Some checks were not successful
zyga
referenced this pull request
Mar 31, 2017
Closed
cmd: use libtool for the internal library #3108
niemeyer
approved these changes
Apr 3, 2017
Looks okay assuming this works well in practice.
A bit unhappy with the amount of fiddling with rules going on there, though. It's past the point where one can read and comprehend what's going on.
Can you please see if this suggestion works before merging:
| +if STATIC_LIBCAP | ||
| +decode_mount_opts_decode_mount_opts_STATIC += -lcap | ||
| +else | ||
| +decode_mount_opts_decode_mount_opts_LDADD += -lcap |
niemeyer
Apr 3, 2017
Contributor
Sorry for not suggesting this earlier, but wouldn't it be simpler to do this:
if STATIC_LIBPCAP
decode_mount_opts_decode_mount_opts_LDADD += libcap.a
else
decode_mount_opts_decode_mount_opts_LDADD += libcap.a
endif
Wouldn't that avoid the further fiddling with "hacked rules" described below?
zyga
Apr 4, 2017
Contributor
I'm not sure I understand, did you mean to say _STATIC in one of the two rules above?
zyga
Apr 7, 2017
Contributor
@niemeyer not quite, if you want to link to static libraries via .a files you have to spell out their full location. As a quick hack I did this:
diff --git a/cmd/Makefile.am b/cmd/Makefile.am
index 695b07e..982543c 100644
--- a/cmd/Makefile.am
+++ b/cmd/Makefile.am
@@ -110,17 +110,10 @@ libsnap_confine_private_unit_tests_CFLAGS += -D_ENABLE_FAULT_INJECTION
libsnap_confine_private_unit_tests_STATIC =
if STATIC_LIBCAP
-libsnap_confine_private_unit_tests_STATIC += -lcap
+libsnap_confine_private_unit_tests_LDADD += /usr/lib/x86_64-linux-gnu/libcap.a
else
libsnap_confine_private_unit_tests_LDADD += -lcap
endif # STATIC_LIBCAP
-
-# Use a hacked rule if we're doing static build. This allows us to inject the LIBS += .. rule below.
-libsnap-confine-private/unit-tests$(EXEEXT): $(libsnap_confine_private_unit_tests_OBJECTS) $(libsnap_confine_private_unit_tests_DEPENDENCIES) $(EXTRA_libsnap_confine_private_unit_tests_DEPENDENCIES) libsnap-confine-private/$(am__dirstamp)
- @rm -f libsnap-confine-private/unit-tests$(EXEEXT)
- $(AM_V_CCLD)$(libsnap_confine_private_unit_tests_LINK) $(libsnap_confine_private_unit_tests_OBJECTS) $(libsnap_confine_private_unit_tests_LDADD) $(LIBS)
-
-libsnap-confine-private/unit-tests$(EXEEXT): LIBS += -Wl,-Bstatic $(libsnap_confine_private_unit_tests_STATIC) -Wl,-Bdynamic
endif # WITH_UNIT_TESTS
##
The results are correct but the onus of finding the library is on us. If we use -lcap wrapped with linker options to link statically (like we currently do) it just works.
zyga@fyke:~/snapd/cmd$ ldd libsnap-confine-private/unit-tests
linux-vdso.so.1 => (0x00007fff18bfd000)
libglib-2.0.so.0 => /lib/x86_64-linux-gnu/libglib-2.0.so.0 (0x00007f414a0a3000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4149cda000)
libpcre.so.3 => /lib/x86_64-linux-gnu/libpcre.so.3 (0x00007f4149a69000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f414984c000)
/lib64/ld-linux-x86-64.so.2 (0x0000562020b78000)
zyga commentedMar 16, 2017
This patch adds a new configure switch --enable-partilly-static that
creates partially static builds of snap-confine. The partially static
builds statically link libseccomp, libapparmor and libcap. They don't
statically link libudev (because libudev talks to udevd over an unstable
protocol).
This patch also changes packaging to enable this new mode on Ubuntu
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com