package/zstd: build multithreaded library if supported
PATCH v2 1/2 package/zstd: build multithreaded library if supported
From: Thomas De Schampheleire <[email protected]>
libzstd.so is built without multi-threading support by default. The 'HAVE_THREAD' flag is not respected by lib/Makefile, only by programs/Makefile.
Use the %-mt recipe in lib/Makefile to enable multithreading.
---
v2: split off of next patch
package/zstd/zstd.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
package/zstd/zstd.mk +8 -0
@@ -48,6 +48,14 @@ ZSTD_BUILD_LIBS = libzstd.a libzstd ZSTD_INSTALL_LIBS = install-static install-shared endif +# The HAVE_THREAD flag is read by the 'programs' makefile but not by the 'lib' +# one. Building a multi-threaded binary with a library (which defaults to +# single-threaded) gives a runtime error when compressing files. +# The 'lib' makefile provides specific '%-mt' targets for this purpose. +ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y) +ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS)) +endif + define ZSTD_BUILD_CMDS $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \ -C $(@D)/lib $(ZSTD_BUILD_LIBS)
Signature · 1 line
2.26.2
PATCH v2 2/2 package/zstd: link programs dynamically with libzstd to save space
From: Thomas De Schampheleire <[email protected]>
Even when a shared libzstd is built, the zstd command-line programs are statically linked with libzstd, causing a large rootfs footprint.
While the cmake backend in zstd already supported a flag ZSTD_PROGRAMS_LINK_SHARED, the make backend did not.
This commit adds support for ZSTD_PROGRAMS_LINK_SHARED in the make system and applies it for the target compilation, unless only static libs are supported.
See https://github.com/facebook/zstd/issues/2261 .
---
v2: no functional changes other than splitting off part to a separate patch
...D_PROGRAMS_LINK_SHARED-to-link-zstd-.patch | 80 +++++++++++++++++++ package/zstd/zstd.mk | 2 + 2 files changed, 82 insertions(+) create mode 100644 package/zstd/0002-make-support-ZSTD_PROGRAMS_LINK_SHARED-to-link-zstd-.patch
package/zstd/0002-make-support-ZSTD_PROGRAMS_LINK_SHARED-to-link-zstd-.patch new file +80 -0
@@ -0,0 +1,80 @@ +From afc368e7247abfe89250def5b7673a9ccb79e0b1 Mon Sep 17 00:00:00 2001 +From: Thomas De Schampheleire <[email protected]> +Date: Wed, 5 Aug 2020 15:35:01 +0200 +Subject: [PATCH] make: support ZSTD_PROGRAMS_LINK_SHARED to link zstd with + libzstd + +zstd allows building via make, cmake, meson, and others, but unfortunately +the features and flags used for these build systems are inconsistent. + +The cmake version respects an option 'ZSTD_PROGRAMS_LINK_SHARED' to let the +zstd binary link dynamically with its own libzstd. Without it, the zstd +program uses static linking and thus has a big size. + +This option is not provided in the 'make' system. There does exist a target +'zstd-dll' which intends to do exactly this, but it does not work out of the +box due to linking issues. These in turn are caused because the lib makefile +passes '-fvisibility=hidden' to the linker, which hides certain symbols that +the zstd program needs. The cmake-based compilation does not pass this +visibility flag, due to which all symbols are visible. + +Unfortunately, there is no 'install' rule that will install zstd-dll and +create the derived programs like zstdless etc. + +With the above in mind, when ZSTD_PROGRAMS_LINK_SHARED is set in make +context: +- remove '-fvisibility=hidden' from the lib compilation +- alter the 'zstd' target to not include the lib objects directly but link + dynamically with libzstd. + +See also https://github.com/facebook/zstd/issues/2261 which reported these +inconsistencies between make and cmake compilation. + +Signed-off-by: Thomas De Schampheleire <[email protected]> + +--- + lib/Makefile | 5 ++++- + programs/Makefile | 8 +++++++- + 2 files changed, 11 insertions(+), 2 deletions(-) + +diff --git a/lib/Makefile b/lib/Makefile +index 7c6dff02..8f9f36a6 100644 +--- a/lib/Makefile ++++ b/lib/Makefile +@@ -204,7 +204,10 @@ $(LIBZSTD): $(ZSTD_FILES) + else + + LIBZSTD = libzstd.$(SHARED_EXT_VER) +-$(LIBZSTD): LDFLAGS += -shared -fPIC -fvisibility=hidden ++$(LIBZSTD): LDFLAGS += -shared -fPIC ++ifndef ZSTD_PROGRAMS_LINK_SHARED ++$(LIBZSTD): LDFLAGS += -fvisibility=hidden ++endif + $(LIBZSTD): $(ZSTD_FILES) + @echo compiling dynamic library $(LIBVER) + $(Q)$(CC) $(FLAGS) $^ $(LDFLAGS) $(SONAME_FLAGS) -o $@ +diff --git a/programs/Makefile b/programs/Makefile +index 418ad4e6..8897dcf9 100644 +--- a/programs/Makefile ++++ b/programs/Makefile +@@ -169,10 +169,16 @@ $(ZSTDDECOMP_O): CFLAGS += $(ALIGN_LOOP) + zstd : CPPFLAGS += $(THREAD_CPP) $(ZLIBCPP) $(LZMACPP) $(LZ4CPP) + zstd : LDFLAGS += $(THREAD_LD) $(ZLIBLD) $(LZMALD) $(LZ4LD) $(DEBUGFLAGS_LD) + zstd : CPPFLAGS += -DZSTD_LEGACY_SUPPORT=$(ZSTD_LEGACY_SUPPORT) ++ifdef ZSTD_PROGRAMS_LINK_SHARED ++# link dynamically against own library ++zstd : LDFLAGS += -L$(ZSTDDIR) -lzstd ++else ++zstd : $(ZSTDLIB_FILES) ++endif + ifneq (,$(filter Windows%,$(OS))) + zstd : $(RES_FILE) + endif +-zstd : $(ZSTDLIB_FILES) $(ZSTD_CLI_OBJ) ++zstd : $(ZSTD_CLI_OBJ) + @echo "$(THREAD_MSG)" + @echo "$(ZLIB_MSG)" + @echo "$(LZMA_MSG)" +-- +2.26.2 +
package/zstd/zstd.mk +2 -0
@@ -43,9 +43,11 @@ ZSTD_INSTALL_LIBS = install-static else ifeq ($(BR2_SHARED_LIBS),y) ZSTD_BUILD_LIBS = libzstd ZSTD_INSTALL_LIBS = install-shared +ZSTD_OPTS += ZSTD_PROGRAMS_LINK_SHARED=1 else ZSTD_BUILD_LIBS = libzstd.a libzstd ZSTD_INSTALL_LIBS = install-static install-shared +ZSTD_OPTS += ZSTD_PROGRAMS_LINK_SHARED=1 endif # The HAVE_THREAD flag is read by the 'programs' makefile but not by the 'lib'
Signature · 1 line
2.26.2
PATCH v2 2/2 package/zstd: link programs dynamically with libzstd to save space
Quoted 12 lines · On Fri, 4 Dec 2020 10:57:02 +0100 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
Even when a shared libzstd is built, the zstd command-line programs are statically linked with libzstd, causing a large rootfs footprint.
While the cmake backend in zstd already supported a flag
ZSTD_PROGRAMS_LINK_SHARED, the make backend did not.This commit adds support for
ZSTD_PROGRAMS_LINK_SHAREDin the make system and applies it for the target compilation, unless only static libs are supported.
Quoted 5 lines
See https://github.com/facebook/zstd/issues/2261 .
Signed-off-by Thomas De Schampheleire <[email protected]>---
v2: no functional changes other than splitting off part to a separate patch
I've marked as Changes Requested, as you proposed in the review of PATCH 1/2 in this series.
Thanks,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v2 1/2 package/zstd: build multithreaded library if supported
In reply to Thomas De SchampheleireHello Thomas,
Quoted 9 lines · On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
libzstd.so is built without multi-threading support by default. The '
HAVE_THREAD' flag is not respected by lib/Makefile, only by programs/Makefile.Use the %-mt recipe in lib/Makefile to enable multithreading.
Signed-off-by Thomas De Schampheleire <[email protected]>
Your patch is fixing this for the target variant of zstd, but to me it seems like the host variant also builds the library and programs separately, so it is probably affected by the same issue, isn't it ?
Also, in your PATCH 2/2 you mention that zstd also supports cmake and meson. Would switching to cmake-package or meson-package simplify a bit this issue and generally what zstd.mk looks like ?
Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v2 1/2 package/zstd: build multithreaded library if supported
Hi Thomas,
El s?b, 2 ene 2021 a las 18:49, Thomas Petazzoni
(<[email protected]>) escribi?:
Quoted 12 lines
Hello Thomas,
Quoted 6 lines · On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
libzstd.so is built without multi-threading support by default. The '
HAVE_THREAD' flag is not respected by lib/Makefile, only by programs/Makefile.
Quoted 7 lines
Quoted 3 lines
Use the %-mt recipe in lib/Makefile to enable multithreading.
Signed-off-by Thomas De Schampheleire <[email protected]>Your patch is fixing this for the target variant of zstd, but to me it seems like the host variant also builds the library and programs separately, so it is probably affected by the same issue, isn't it ?
The main driver for this series was the ever-expanding size of zstd, worsened by the fact that the binary was using static linking. For host, there is generally no size restriction, at least not of this order.
I'm not sure about host-mtd, and host-squashfs, but for the host-zstd command-line tool itself, it is important that the binary and library are either both built for single-threaded or both for multi-threaded. This means that we'd have to pass HAVE_THREAD=1 for the host compilation of zstd as well. Probably possible, not sure why it is not done today.
Quoted 4 lines
Also, in your PATCH 2/2 you mention that zstd also supports cmake and meson. Would switching to cmake-package or meson-package simplify a bit this issue and generally what zstd.mk looks like ?
The developers mention in certain issues that 'make' is the official build system, and that the other ones (like cmake and meson) are 'community supported'.
So while their make logic is not clean at all, I would not necessarily trust non-official solutions better.
Meanwhile, a FreeBSD developer proposed an alternative patch on the corresponding issue:
https://github.com/facebook/zstd/issues/2261#issuecomment-753632465
So I suggest to hold this patch for a while, I'll try to get it moving upstream.
Thanks,
Thomas
Quoted 8 lines
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
PATCH v2 1/2 package/zstd: build multithreaded library if supported
Hello,
Quoted 7 lines · On Tue, 5 Jan 2021 17:20:39 +0100 Thomas De Schampheleire…
Quoted 3 lines
Your patch is fixing this for the target variant of zstd, but to me it seems like the host variant also builds the library and programs separately, so it is probably affected by the same issue, isn't it ?
The main driver for this series was the ever-expanding size of zstd, worsened by the fact that the binary was using static linking. For host, there is generally no size restriction, at least not of this order.
Right, but my point was not about size here but
correctness/functionality.
Quoted 6 lines
I'm not sure about host-mtd, and host-squashfs, but for the host-zstd command-line tool itself, it is important that the binary and library are either both built for single-threaded or both for multi-threaded. This means that we'd have to pass HAVE_THREAD=1 for the host compilation of zstd as well. Probably possible, not sure why it is not done today.
This is the point I missed: the host variant is not built with any HAVE_THREAD= value, so I suppose it's built single-threaded, which is why it works fine as it is today.
So your patch is correct in fixing just the target package to use the %-mt make targets, because only the target package is built with thread support when available.
Optionally, we could have a separate patch that turns on building with HAVE_THREAD=1 for the host package, but it's a separate matter.
Makes sense.
Quoted 10 lines
Quoted 3 lines
Also, in your PATCH 2/2 you mention that zstd also supports cmake and meson. Would switching to cmake-package or meson-package simplify a bit this issue and generally what zstd.mk looks like ?
The developers mention in certain issues that 'make' is the official build system, and that the other ones (like cmake and meson) are 'community supported'.
So while their make logic is not clean at all, I would not necessarily trust non-official solutions better.
Quoted 5 lines
Meanwhile, a FreeBSD developer proposed an alternative patch on the corresponding issue:
https://github.com/facebook/zstd/issues/2261#issuecomment-753632465So I suggest to hold this patch for a while, I'll try to get it moving upstream.
OK. This is for PATCH 2/2, right ?
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v2 1/2 package/zstd: build multithreaded library if supported
Quoted 47 lines · On Tue, Jan 5, 2021, 17:36 Thomas Petazzoni <[email protected]>…
Hello,
Quoted 7 lines · On Tue, 5 Jan 2021 17:20:39 +0100 Thomas De Schampheleire…
Quoted 3 lines
Your patch is fixing this for the target variant of zstd, but to me it seems like the host variant also builds the library and programs separately, so it is probably affected by the same issue, isn't it ?
The main driver for this series was the ever-expanding size of zstd, worsened by the fact that the binary was using static linking. For host, there is generally no size restriction, at least not of this
order.
Right, but my point was not about size here but
correctness/functionality.Quoted 6 lines
I'm not sure about host-mtd, and host-squashfs, but for the host-zstd command-line tool itself, it is important that the binary and library are either both built for single-threaded or both for multi-threaded. This means that we'd have to pass HAVE_THREAD=1 for the host compilation of zstd as well. Probably possible, not sure why it is not done today.
This is the point I missed: the host variant is not built with any HAVE_THREAD= value, so I suppose it's built single-threaded, which is why it works fine as it is today.
So your patch is correct in fixing just the target package to use the %-mt make targets, because only the target package is built with thread support when available.
Optionally, we could have a separate patch that turns on building with HAVE_THREAD=1 for the host package, but it's a separate matter.
Makes sense.
Quoted 10 lines
Quoted 3 lines
Also, in your PATCH 2/2 you mention that zstd also supports cmake and meson. Would switching to cmake-package or meson-package simplify a bit this issue and generally what zstd.mk looks like ?
The developers mention in certain issues that 'make' is the official build system, and that the other ones (like cmake and meson) are 'community supported'.
So while their make logic is not clean at all, I would not necessarily trust non-official solutions better.
Quoted 9 lines
Quoted 5 lines
Meanwhile, a FreeBSD developer proposed an alternative patch on the corresponding issue:
https://github.com/facebook/zstd/issues/2261#issuecomment-753632465So I suggest to hold this patch for a while, I'll try to get it moving
upstream.
OK. This is for PATCH 2/2, right ?
Yes, sorry for the confusion. Patch 1 can proceed, and patch 2 to be held.
Thanks,
Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20210105/a2148658/attachment.html>
Thomas Petazzoni
AppliedPATCH v2 1/2 package/zstd: build multithreaded library if supported
In reply to Thomas De SchampheleireQuoted 11 lines · On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
libzstd.so is built without multi-threading support by default. The '
HAVE_THREAD' flag is not respected by lib/Makefile, only by programs/Makefile.Use the %-mt recipe in lib/Makefile to enable multithreading.
Signed-off-by Thomas De Schampheleire <[email protected]>---
v2: split off of next patch
Applied to master, thanks. It would be good to have a patch that enables the multi-threading for the host variant of zstd.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v2 1/2 package/zstd: build multithreaded library if supported
Hi Thomas,
El dom, 17 ene 2021 a las 15:30, Thomas Petazzoni
(<[email protected]>) escribi?:
Quoted 10 lines
Quoted 6 lines · On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
libzstd.so is built without multi-threading support by default. The '
HAVE_THREAD' flag is not respected by lib/Makefile, only by programs/Makefile.
Quoted 9 lines
Quoted 5 lines
Use the %-mt recipe in lib/Makefile to enable multithreading.
Signed-off-by Thomas De Schampheleire <[email protected]>---
v2: split off of next patchApplied to master, thanks. It would be good to have a patch that enables the multi-threading for the host variant of zstd.
Thanks, I just sent such patch for host-zstd.
I learned that upstream has added a clean way of building the zstd cli programs against its own libzstd, currently in its 'dev' branch (not yet released). This will provide an alternative implementation than my 2/2 patch.
With that new 'zstd-dll' target we can link both target and host zstd dynamically. I have the Buildroot changes ready, but am waiting for a new release of zstd (1.4.9 or 1.5.x ?).
Best regards,
Thomas