[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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 <thomas.de_schampheleire@nokia.com>
---
v2: split off of next patch

 package/zstd/zstd.mk | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 35002da332..37c1a8d687 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -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)
-- 
2.26.2

[Buildroot] [PATCHv2 2/2] package/zstd: link programs dynamically with libzstd to save space

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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 .

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
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

diff --git a/package/zstd/0002-make-support-ZSTD_PROGRAMS_LINK_SHARED-to-link-zstd-.patch b/package/zstd/0002-make-support-ZSTD_PROGRAMS_LINK_SHARED-to-link-zstd-.patch
new file mode 100644
index 0000000000..100cfaba95
--- /dev/null
+++ b/package/zstd/0002-make-support-ZSTD_PROGRAMS_LINK_SHARED-to-link-zstd-.patch
@@ -0,0 +1,80 @@
+From afc368e7247abfe89250def5b7673a9ccb79e0b1 Mon Sep 17 00:00:00 2001
+From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
+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 <thomas.de_schampheleire@nokia.com>
+
+---
+ 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
+
diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 37c1a8d687..c7b224b002 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -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'
-- 
2.26.2

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas Petazzoni]

Hello Thomas,

On Fri,  4 Dec 2020 10:57:01 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> 
> 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 <thomas.de_schampheleire@nokia.com>

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas De Schampheleire]

Hi Thomas,

El s?b, 2 ene 2021 a las 18:49, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> Hello Thomas,
>
> On Fri,  4 Dec 2020 10:57:01 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > 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 <thomas.de_schampheleire@nokia.com>
>
> 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.

>
> 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


>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas Petazzoni]

Hello,

On Tue, 5 Jan 2021 17:20:39 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> > 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.

> 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.

> > 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.

OK. This is for PATCH 2/2, right ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas De Schampheleire]

On Tue, Jan 5, 2021, 17:36 Thomas Petazzoni <thomas.petazzoni@bootlin.com>
wrote:

> Hello,
>
> On Tue, 5 Jan 2021 17:20:39 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > > 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.
>
> > 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.
>
> > > 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.
>
> 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>

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas Petazzoni]

On Fri,  4 Dec 2020 10:57:01 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> 
> 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 <thomas.de_schampheleire@nokia.com>
> ---
> 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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCHv2 2/2] package/zstd: link programs dynamically with libzstd to save space

[Thomas Petazzoni]

On Fri,  4 Dec 2020 10:57:02 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> 
> 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 .
> 
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> ---
> 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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCHv2 1/2] package/zstd: build multithreaded library if supported

[Thomas De Schampheleire]

Hi Thomas,

El dom, 17 ene 2021 a las 15:30, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> On Fri,  4 Dec 2020 10:57:01 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > 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 <thomas.de_schampheleire@nokia.com>
> > ---
> > 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.
>

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