[Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0

[Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0

[Norbert Lange]

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/zstd/zstd.hash | 4 ++--
 package/zstd/zstd.mk   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/zstd/zstd.hash b/package/zstd/zstd.hash
index a979501e5f..2d7bf37b2a 100644
--- a/package/zstd/zstd.hash
+++ b/package/zstd/zstd.hash
@@ -1,5 +1,5 @@
-# From https://github.com/facebook/zstd/releases/download/v1.4.9/zstd-1.4.9.tar.gz.sha256
-sha256  29ac74e19ea28659017361976240c4b5c5c24db3b89338731a6feb97c038d293  zstd-1.4.9.tar.gz
+# From https://github.com/facebook/zstd/releases/download/v1.5.0/zstd-1.5.0.tar.gz.sha256
+sha256  5194fbfa781fcf45b98c5e849651aa7b3b0a008c6b72d4a0db760f3002291e94  zstd-1.5.0.tar.gz
 
 # License files (locally computed)
 sha256  2c1a7fa704df8f3a606f6fc010b8b5aaebf403f3aeec339a12048f1ba7331a0b  LICENSE
diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 63ea8b1b35..194974147c 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -4,7 +4,7 @@
 #
 ################################################################################
 
-ZSTD_VERSION = 1.4.9
+ZSTD_VERSION = 1.5.0
 ZSTD_SITE = https://github.com/facebook/zstd/releases/download/v$(ZSTD_VERSION)
 ZSTD_INSTALL_STAGING = YES
 ZSTD_LICENSE = BSD-3-Clause or GPL-2.0
-- 
2.30.2

[Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build

[Norbert Lange]

1.5.0 uses Threads by default for cli tool and DSO,
should not be necessary to do anything special.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/zstd/zstd.mk | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 194974147c..2a876376a2 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -78,21 +78,16 @@ define ZSTD_INSTALL_TARGET_CMDS
 		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
 endef
 
-# note: only limited 'HAVE_...' options for host library build only
-HOST_ZSTD_OPTS = HAVE_THREAD=1
+HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
 
 define HOST_ZSTD_BUILD_CMDS
 	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
-		-C $(@D)/lib libzstd.a-mt libzstd-mt
-	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
-		-C $(@D) zstd
+		-C $(@D) zstd-release lib-release
 endef
 
 define HOST_ZSTD_INSTALL_CMDS
 	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
-		DESTDIR=$(HOST_DIR) PREFIX=/usr -C $(@D)/lib install
-	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
-		DESTDIR=$(HOST_DIR) PREFIX=/usr -C $(@D)/programs install
+		-C $(@D) install
 endef
 
 $(eval $(generic-package))
-- 
2.30.2

[Buildroot] [PATCH v2 3/5] package/zstd: rework build and install

[Norbert Lange]

1.5.0 uses Threads by default for cli tool and DSO,
the build now does the same unless:

If only static libraries are build, then build
that library like the DSO is normally built.

This should ensure that programs requsting the DSO
will always get the multithreaded version.

Signed-off-by: Norbert Lange <nolange79@gmail.com>

---
v1->v2:
*   rebased against upstream/master

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/zstd/zstd.mk | 52 ++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 2a876376a2..a7a5ba4e50 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -12,6 +12,8 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
 ZSTD_CPE_ID_VENDOR = facebook
 ZSTD_CPE_ID_PRODUCT = zstandard
 
+ZSTD_OPTS += PREFIX=/usr
+
 ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
 ZSTD_OPTS += HAVE_THREAD=1
 else
@@ -39,43 +41,55 @@ else
 ZSTD_OPTS += HAVE_LZ4=0
 endif
 
-ifeq ($(BR2_STATIC_LIBS),y)
-ZSTD_BUILD_LIBS = libzstd.a
-ZSTD_INSTALL_LIBS = install-static
-else ifeq ($(BR2_SHARED_LIBS),y)
-ZSTD_BUILD_LIBS = libzstd
-ZSTD_INSTALL_LIBS = install-shared
+ZSTD_BUILD_PROG_TARGET := zstd-release
+
+# Since v1.5.0 the dynamic library is built for
+# multithreading, while the static library is not.
+#
+# Keep those defaults, unless Buildroot is not
+# providing the dynamic library and the
+# static library will be automatically used instead.
+ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+ZSTD_INSTALL_LIBS += install-static
+ifeq ($(BR2_STATIC_LIBS)$(BR2_TOOLCHAIN_HAS_THREADS),yy)
+# Use the static lib as replacement for the mt dynlib
+ZSTD_BUILD_LIBS += libzstd.a-mt
 else
-ZSTD_BUILD_LIBS = libzstd.a libzstd
-ZSTD_INSTALL_LIBS = install-static install-shared
+ZSTD_BUILD_LIBS += libzstd.a-nomt
+endif
 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_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+ZSTD_INSTALL_LIBS += install-shared
 ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
-ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
+ZSTD_BUILD_LIBS += libzstd-mt
+else
+ZSTD_BUILD_LIBS += libzstd-nomt
+endif
 endif
 
 define ZSTD_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		-C $(@D)/lib $(ZSTD_BUILD_LIBS)
+		-C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		-C $(@D) zstd
+		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
 endef
 
 define ZSTD_INSTALL_STAGING_CMDS
+	[ -e $(@D)/programs/zstd ] && [ -e $(@D)/lib/libzstd.pc ]
+	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
+		-C $(@D)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
+		install-pc install-includes
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		DESTDIR=$(STAGING_DIR) PREFIX=/usr -C $(@D)/lib \
-		install-pc install-includes $(ZSTD_INSTALL_LIBS)
+		-C $(@D)/programs DESTDIR=$(STAGING_DIR) install
 endef
 
 define ZSTD_INSTALL_TARGET_CMDS
+	[ -e $(@D)/programs/zstd ]
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
+		-C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
+		-C $(@D)/programs DESTDIR=$(TARGET_DIR) install
 endef
 
 HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
-- 
2.30.2

[Buildroot] [PATCH v2 4/5] package/zstd: Change Build options

[Norbert Lange]

Disable the legacy format, these are just needed for
decompressing files created with pre-release version.

Use Buildroot's setting for optimization, zstd's build system
overrides CFLAGS, but MOREFLAGS can override again.
Quick tests show that using -O2 (like buildroot)
is actually a little faster than -O3 on x86_64 Atoms.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/zstd/zstd.mk | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index a7a5ba4e50..a9499df4d0 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -13,6 +13,10 @@ ZSTD_CPE_ID_VENDOR = facebook
 ZSTD_CPE_ID_PRODUCT = zstandard
 
 ZSTD_OPTS += PREFIX=/usr
+ZSTD_OPTS += ZSTD_LEGACY_SUPPORT=0
+
+# zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
+ZSTD_OPTS_MOREFLAGS += $(TARGET_OPTIMIZATION)
 
 ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
 ZSTD_OPTS += HAVE_THREAD=1
@@ -41,6 +45,8 @@ else
 ZSTD_OPTS += HAVE_LZ4=0
 endif
 
+ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
+
 ZSTD_BUILD_PROG_TARGET := zstd-release
 
 # Since v1.5.0 the dynamic library is built for
-- 
2.30.2

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Norbert Lange]

There are a couple of targets to build the cli tool with less
features (no legacy-support, benchmarking),
or dynamically linked to the library.

Add an option to choose the variant.

To allow the installation step to pick the variant, it
has to be copied to the normal location.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/zstd/Config.in | 21 +++++++++++++++++++++
 package/zstd/zstd.mk   |  9 ++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/package/zstd/Config.in b/package/zstd/Config.in
index 9fa70c65cc..0d2ab84771 100644
--- a/package/zstd/Config.in
+++ b/package/zstd/Config.in
@@ -10,3 +10,24 @@ config BR2_PACKAGE_ZSTD
 	  compression formats
 
 	  https://facebook.github.io/zstd
+
+if BR2_PACKAGE_ZSTD
+
+choice
+	prompt "Executable flavor"
+	help
+	  Pick between variants of the zstd tool.
+
+config BR2_PACKAGE_ZSTD_PROG_DEFAULT
+	bool "Default build (static, full-featured)"
+
+config BR2_PACKAGE_ZSTD_PROG_DYNAMIC
+	bool "Dynamic build (needs library with identical version)"
+	depends on !BR2_STATIC_LIBS
+
+config BR2_PACKAGE_ZSTD_PROG_SMALL
+	bool "Small build (static, less features)"
+
+endchoice
+
+endif
diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index a9499df4d0..ecad26e0df 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -48,6 +48,11 @@ endif
 ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
 
 ZSTD_BUILD_PROG_TARGET := zstd-release
+ifeq ($(BR2_PACKAGE_ZSTD_PROG_DYNAMIC),y)
+ZSTD_BUILD_PROG_TARGET := zstd-dll
+else ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
+ZSTD_BUILD_PROG_TARGET := zstd-small
+endif
 
 # Since v1.5.0 the dynamic library is built for
 # multithreading, while the static library is not.
@@ -78,7 +83,9 @@ define ZSTD_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
 		-C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
+		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET) && \
+	{ [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
+		ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; }
 endef
 
 define ZSTD_INSTALL_STAGING_CMDS
-- 
2.30.2

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Thomas De Schampheleire]

Hello,

El mar, 25 may 2021 a las 19:28, Norbert Lange (<nolange79@gmail.com>) escribi?:
>
> There are a couple of targets to build the cli tool with less
> features (no legacy-support, benchmarking),
> or dynamically linked to the library.
>
> Add an option to choose the variant.
>
> To allow the installation step to pick the variant, it
> has to be copied to the normal location.
>
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/zstd/Config.in | 21 +++++++++++++++++++++
>  package/zstd/zstd.mk   |  9 ++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/package/zstd/Config.in b/package/zstd/Config.in
> index 9fa70c65cc..0d2ab84771 100644
> --- a/package/zstd/Config.in
> +++ b/package/zstd/Config.in
> @@ -10,3 +10,24 @@ config BR2_PACKAGE_ZSTD
>           compression formats
>
>           https://facebook.github.io/zstd
> +
> +if BR2_PACKAGE_ZSTD
> +
> +choice
> +       prompt "Executable flavor"
> +       help
> +         Pick between variants of the zstd tool.
> +
> +config BR2_PACKAGE_ZSTD_PROG_DEFAULT
> +       bool "Default build (static, full-featured)"
> +
> +config BR2_PACKAGE_ZSTD_PROG_DYNAMIC
> +       bool "Dynamic build (needs library with identical version)"
> +       depends on !BR2_STATIC_LIBS

Is there a reason not to make this one the default, instead of the
static variant?
From my point of view, the PROG_DYNAMIC has as main advantage a
reduction in rootfs size, since zstd is quite large and with the
static version duplicating part of the shared object.

Earlier I sent a patch for this (exact implementation was still based
on v1.4.9 and sending of the update patch was pending the new 1.5.0
release), see:
http://patchwork.ozlabs.org/project/buildroot/patch/20201204095703.4714-1-patrickdepinguin at gmail.com/

In context of Buildroot, the expectation is that one builds the entire
thing in one go. This means that binary and library will be from the
same version anyway.
In that sense, the option description may be confusing.

In fact, I even wonder if it is needed to provide the PROG_DEFAULT at
all. If shared libraries are supported, I see no reason why one would
like a static version of the binary.
So I would reduce this patch to two options:

- standard build
- small build

and depending on the options BR2_STATIC_LIBS / SHARED_STATIC_LIBS the
'standard build' would result in a dynamically-linked object or a
static one.


> +
> +config BR2_PACKAGE_ZSTD_PROG_SMALL
> +       bool "Small build (static, less features)"
> +
> +endchoice
> +
> +endif
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index a9499df4d0..ecad26e0df 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -48,6 +48,11 @@ endif
>  ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
>
>  ZSTD_BUILD_PROG_TARGET := zstd-release
> +ifeq ($(BR2_PACKAGE_ZSTD_PROG_DYNAMIC),y)
> +ZSTD_BUILD_PROG_TARGET := zstd-dll
> +else ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
> +ZSTD_BUILD_PROG_TARGET := zstd-small
> +endif

We don't generally use := in Buildroot, unless really needed. These
assignments could just be '='.


>
>  # Since v1.5.0 the dynamic library is built for
>  # multithreading, while the static library is not.
> @@ -78,7 +83,9 @@ define ZSTD_BUILD_CMDS
>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>                 -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
> +               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET) && \
> +       { [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
> +               ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; }

I find this code not particularly readable.
Why does the group need to be continued with '&&' ? If you don't do
that, a failure in the make will stop right there, which is fine.
This together with the use of the || shorthand, especially with the
negated condition, is not obvious.

Also, this hardlink is only actually needed for the 'zstd-small' case,
because both 'zstd-release' and 'zstd-dll' just generate a binary
called 'zstd'. In the latter cases, the [ ] condition will be false,
and nothing to be done.

So I would consider to leave the original make untouched and use
following instead:


ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
define ZSTD_HARDLINK_BUILD_PROG_TARGET
        ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd
endef
ZSTD_POST_BUILD_HOOKS += ZSTD_HARDLINK_BUILD_PROG_TARGET
endif


Best regards,
Thomas

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Norbert Lange]

Am Mo., 31. Mai 2021 um 17:02 Uhr schrieb Thomas De Schampheleire
<patrickdepinguin@gmail.com>:
>
> Hello,
>
> El mar, 25 may 2021 a las 19:28, Norbert Lange (<nolange79@gmail.com>) escribi?:
> >
> > There are a couple of targets to build the cli tool with less
> > features (no legacy-support, benchmarking),
> > or dynamically linked to the library.
> >
> > Add an option to choose the variant.
> >
> > To allow the installation step to pick the variant, it
> > has to be copied to the normal location.
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> > ---
> >  package/zstd/Config.in | 21 +++++++++++++++++++++
> >  package/zstd/zstd.mk   |  9 ++++++++-
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/package/zstd/Config.in b/package/zstd/Config.in
> > index 9fa70c65cc..0d2ab84771 100644
> > --- a/package/zstd/Config.in
> > +++ b/package/zstd/Config.in
> > @@ -10,3 +10,24 @@ config BR2_PACKAGE_ZSTD
> >           compression formats
> >
> >           https://facebook.github.io/zstd
> > +
> > +if BR2_PACKAGE_ZSTD
> > +
> > +choice
> > +       prompt "Executable flavor"
> > +       help
> > +         Pick between variants of the zstd tool.
> > +
> > +config BR2_PACKAGE_ZSTD_PROG_DEFAULT
> > +       bool "Default build (static, full-featured)"
> > +
> > +config BR2_PACKAGE_ZSTD_PROG_DYNAMIC
> > +       bool "Dynamic build (needs library with identical version)"
> > +       depends on !BR2_STATIC_LIBS
>
> Is there a reason not to make this one the default, instead of the
> static variant?

Hello.

Gradual change, as gambit to get the patches upstreamed faster =)
I agree, dynamic should be default.

> From my point of view, the PROG_DYNAMIC has as main advantage a
> reduction in rootfs size, since zstd is quite large and with the
> static version duplicating part of the shared object.
>
> Earlier I sent a patch for this (exact implementation was still based
> on v1.4.9 and sending of the update patch was pending the new 1.5.0
> release), see:
> http://patchwork.ozlabs.org/project/buildroot/patch/20201204095703.4714-1-patrickdepinguin at gmail.com/
>
> In context of Buildroot, the expectation is that one builds the entire
> thing in one go. This means that binary and library will be from the
> same version anyway.
> In that sense, the option description may be confusing.
>
> In fact, I even wonder if it is needed to provide the PROG_DEFAULT at
> all. If shared libraries are supported, I see no reason why one would
> like a static version of the binary.

upstream treats DSO and programs separately, the static binary could
have some more flags not available for DSO which seem to be considered
less configurable.
Some time ago I had to use the static library cause some features
where only available there.

Also despite expectation, you cant for example just build zstd (static)
without multiple packages automatically linking to libzstd.
(not easy to change now...)
For that youd need to stich together 2 runs.

> So I would reduce this patch to two options:
>
> - standard build
> - small build
>
> and depending on the options BR2_STATIC_LIBS / SHARED_STATIC_LIBS the
> 'standard build' would result in a dynamically-linked object or a
> static one.

Sounds reasonable.

>
> > +
> > +config BR2_PACKAGE_ZSTD_PROG_SMALL
> > +       bool "Small build (static, less features)"
> > +
> > +endchoice
> > +
> > +endif
> > diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> > index a9499df4d0..ecad26e0df 100644
> > --- a/package/zstd/zstd.mk
> > +++ b/package/zstd/zstd.mk
> > @@ -48,6 +48,11 @@ endif
> >  ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
> >
> >  ZSTD_BUILD_PROG_TARGET := zstd-release
> > +ifeq ($(BR2_PACKAGE_ZSTD_PROG_DYNAMIC),y)
> > +ZSTD_BUILD_PROG_TARGET := zstd-dll
> > +else ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
> > +ZSTD_BUILD_PROG_TARGET := zstd-small
> > +endif
>
> We don't generally use := in Buildroot, unless really needed. These
> assignments could just be '='.

Ok

>
> >
> >  # Since v1.5.0 the dynamic library is built for
> >  # multithreading, while the static library is not.
> > @@ -78,7 +83,9 @@ define ZSTD_BUILD_CMDS
> >         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> >                 -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
> >         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
> > +               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET) && \
> > +       { [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
> > +               ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; }
>
> I find this code not particularly readable.
> Why does the group need to be continued with '&&' ? If you don't do
> that, a failure in the make will stop right there, which is fine.
> This together with the use of the || shorthand, especially with the
> negated condition, is not obvious.

Ok

> Also, this hardlink is only actually needed for the 'zstd-small' case,
> because both 'zstd-release' and 'zstd-dll' just generate a binary
> called 'zstd'. In the latter cases, the [ ] condition will be false,
> and nothing to be done.
>
> So I would consider to leave the original make untouched and use
> following instead:
>
>
> ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
> define ZSTD_HARDLINK_BUILD_PROG_TARGET
>         ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd
> endef
> ZSTD_POST_BUILD_HOOKS += ZSTD_HARDLINK_BUILD_PROG_TARGET
> endif

I disagree here, there are multiple variants aside of -small, like a
decompress-only build,
and I'd not want to depend on upstream behaviors whether a variant is
"in-place" of zstd
or as extra file.
Even if its not gonna be supported officially there's value in locally
using "zstd-decompress"
and things just work.

Arguably with zstd-dll largely fixing the main issue (filesize) those
variants are less tempting.

Norbert
(Waiting for some more feedback and possibly upstreaming  part of the
series before rerolling)

[Buildroot] [PATCH v2 3/5] package/zstd: rework build and install

[Arnout Vandecappelle]

 Hi Norbert,

On 25/05/2021 19:26, Norbert Lange wrote:
> 1.5.0 uses Threads by default for cli tool and DSO,
> the build now does the same unless:
> 
> If only static libraries are build, then build
> that library like the DSO is normally built.
> 
> This should ensure that programs requsting the DSO
> will always get the multithreaded version.

 I don't understand what this patch is trying to do...

 Before this patch, in the threads case, the build would be done with threads
both for the program (HAVE_THREAD=1) and the libraries (-mt appended). For the
no-threads case, the build is done without threads support for both the program
(HAVE_THREAD=0) and the shared&static libraries (no -mt appended) - except that
since 1.5.0, the shared library will be built with thread support which will
probably result in a build failure.

 So as far as I can see, the only thing that needs to be done is:

ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
else
ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
endif


 You patch, however, builds the static library with thread support if no dynamic
library is built, and without thread support if a dynamic library is built. This
makes no sense whatsoever to me...


 Can you clarify the reasoning?

 For now, I've marked the patch as Changes Requested. Since I did apply the
first patch, we will probably start seeing build failures on nothreads, so we'll
need a fix soonish...

 Regards,
 Arnout

> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> 
> ---
> v1->v2:
> *   rebased against upstream/master
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/zstd/zstd.mk | 52 ++++++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 2a876376a2..a7a5ba4e50 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -12,6 +12,8 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
>  ZSTD_CPE_ID_VENDOR = facebook
>  ZSTD_CPE_ID_PRODUCT = zstandard
>  
> +ZSTD_OPTS += PREFIX=/usr
> +
>  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
>  ZSTD_OPTS += HAVE_THREAD=1
>  else
> @@ -39,43 +41,55 @@ else
>  ZSTD_OPTS += HAVE_LZ4=0
>  endif
>  
> -ifeq ($(BR2_STATIC_LIBS),y)
> -ZSTD_BUILD_LIBS = libzstd.a
> -ZSTD_INSTALL_LIBS = install-static
> -else ifeq ($(BR2_SHARED_LIBS),y)
> -ZSTD_BUILD_LIBS = libzstd
> -ZSTD_INSTALL_LIBS = install-shared
> +ZSTD_BUILD_PROG_TARGET := zstd-release
> +
> +# Since v1.5.0 the dynamic library is built for
> +# multithreading, while the static library is not.
> +#
> +# Keep those defaults, unless Buildroot is not
> +# providing the dynamic library and the
> +# static library will be automatically used instead.
> +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_INSTALL_LIBS += install-static
> +ifeq ($(BR2_STATIC_LIBS)$(BR2_TOOLCHAIN_HAS_THREADS),yy)
> +# Use the static lib as replacement for the mt dynlib
> +ZSTD_BUILD_LIBS += libzstd.a-mt
>  else
> -ZSTD_BUILD_LIBS = libzstd.a libzstd
> -ZSTD_INSTALL_LIBS = install-static install-shared
> +ZSTD_BUILD_LIBS += libzstd.a-nomt
> +endif
>  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_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_INSTALL_LIBS += install-shared
>  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> -ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> +ZSTD_BUILD_LIBS += libzstd-mt
> +else
> +ZSTD_BUILD_LIBS += libzstd-nomt
> +endif
>  endif
>  
>  define ZSTD_BUILD_CMDS
>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		-C $(@D)/lib $(ZSTD_BUILD_LIBS)
> +		-C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		-C $(@D) zstd
> +		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
>  endef
>  
>  define ZSTD_INSTALL_STAGING_CMDS
> +	[ -e $(@D)/programs/zstd ] && [ -e $(@D)/lib/libzstd.pc ]
> +	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> +		-C $(@D)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
> +		install-pc install-includes
>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(STAGING_DIR) PREFIX=/usr -C $(@D)/lib \
> -		install-pc install-includes $(ZSTD_INSTALL_LIBS)
> +		-C $(@D)/programs DESTDIR=$(STAGING_DIR) install
>  endef
>  
>  define ZSTD_INSTALL_TARGET_CMDS
> +	[ -e $(@D)/programs/zstd ]
>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
> +		-C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
> +		-C $(@D)/programs DESTDIR=$(TARGET_DIR) install
>  endef
>  
>  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
> 

[Buildroot] [PATCH v2 4/5] package/zstd: Change Build options

[Arnout Vandecappelle]

On 25/05/2021 19:26, Norbert Lange wrote:
> Disable the legacy format, these are just needed for
> decompressing files created with pre-release version.
> 
> Use Buildroot's setting for optimization, zstd's build system
> overrides CFLAGS, but MOREFLAGS can override again.
> Quick tests show that using -O2 (like buildroot)
> is actually a little faster than -O3 on x86_64 Atoms.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/zstd/zstd.mk | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index a7a5ba4e50..a9499df4d0 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -13,6 +13,10 @@ ZSTD_CPE_ID_VENDOR = facebook
>  ZSTD_CPE_ID_PRODUCT = zstandard
>  
>  ZSTD_OPTS += PREFIX=/usr
> +ZSTD_OPTS += ZSTD_LEGACY_SUPPORT=0
> +
> +# zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
> +ZSTD_OPTS_MOREFLAGS += $(TARGET_OPTIMIZATION)

 Wouldn't it make more sense to apply *all* buildroot's flags here, i.e.

ZSTD_OPTS += MOREFLAGS="$(TARGET_CFLAGS)"


(also note:
 - intermediate variable ZSTD_OPTS_MOREFLAGS is not needed for anything;
 - we usually quote only the "$(TARGET_CFLAGS)" part, not including the
MOREFLAGS= part)


 Regards,
 Arnout

>  
>  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
>  ZSTD_OPTS += HAVE_THREAD=1
> @@ -41,6 +45,8 @@ else
>  ZSTD_OPTS += HAVE_LZ4=0
>  endif
>  
> +ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
> +
>  ZSTD_BUILD_PROG_TARGET := zstd-release
>  
>  # Since v1.5.0 the dynamic library is built for
> 

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Arnout Vandecappelle]

On 01/06/2021 11:20, Norbert Lange wrote:
> Am Mo., 31. Mai 2021 um 17:02 Uhr schrieb Thomas De Schampheleire
> <patrickdepinguin@gmail.com>:
>>
>> Hello,
>>
>> El mar, 25 may 2021 a las 19:28, Norbert Lange (<nolange79@gmail.com>) escribi?:
>>>
>>> There are a couple of targets to build the cli tool with less
>>> features (no legacy-support, benchmarking),
>>> or dynamically linked to the library.
>>>
>>> Add an option to choose the variant.
>>>
>>> To allow the installation step to pick the variant, it
>>> has to be copied to the normal location.
>>>
>>> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>>> ---
>>>  package/zstd/Config.in | 21 +++++++++++++++++++++
>>>  package/zstd/zstd.mk   |  9 ++++++++-
>>>  2 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/package/zstd/Config.in b/package/zstd/Config.in
>>> index 9fa70c65cc..0d2ab84771 100644
>>> --- a/package/zstd/Config.in
>>> +++ b/package/zstd/Config.in
>>> @@ -10,3 +10,24 @@ config BR2_PACKAGE_ZSTD
>>>           compression formats
>>>
>>>           https://facebook.github.io/zstd
>>> +
>>> +if BR2_PACKAGE_ZSTD
>>> +
>>> +choice
>>> +       prompt "Executable flavor"
>>> +       help
>>> +         Pick between variants of the zstd tool.
>>> +
>>> +config BR2_PACKAGE_ZSTD_PROG_DEFAULT
>>> +       bool "Default build (static, full-featured)"
>>> +
>>> +config BR2_PACKAGE_ZSTD_PROG_DYNAMIC
>>> +       bool "Dynamic build (needs library with identical version)"
>>> +       depends on !BR2_STATIC_LIBS
>>
>> Is there a reason not to make this one the default, instead of the
>> static variant?
> 
> Hello.
> 
> Gradual change, as gambit to get the patches upstreamed faster =)
> I agree, dynamic should be default.

 Adding config options is not very likely to get patches upstreamed faster :-)

> 
>> From my point of view, the PROG_DYNAMIC has as main advantage a
>> reduction in rootfs size, since zstd is quite large and with the
>> static version duplicating part of the shared object.
>>
>> Earlier I sent a patch for this (exact implementation was still based
>> on v1.4.9 and sending of the update patch was pending the new 1.5.0
>> release), see:
>> http://patchwork.ozlabs.org/project/buildroot/patch/20201204095703.4714-1-patrickdepinguin at gmail.com/
>>
>> In context of Buildroot, the expectation is that one builds the entire
>> thing in one go. This means that binary and library will be from the
>> same version anyway.
>> In that sense, the option description may be confusing.
>>
>> In fact, I even wonder if it is needed to provide the PROG_DEFAULT at
>> all. If shared libraries are supported, I see no reason why one would
>> like a static version of the binary.
> 
> upstream treats DSO and programs separately, the static binary could
> have some more flags not available for DSO which seem to be considered
> less configurable.

 I looked at the source and simply see no way how this is even possible - the
command line argument parsing is entirely in the source files in the program
directory, and there are no different -D options for zstd-dll...

> Some time ago I had to use the static library cause some features
> where only available there.

 Maybe that was in the past and is no longer the case?

 Note BTW that zstd programs don't use the static library - they (re)build the
source files directly. That's how it's possible that the static library has no
thread support while the programs do have thread support.


> Also despite expectation, you cant for example just build zstd (static)
> without multiple packages automatically linking to libzstd.

 What you're saying is: if you enable zstd in Buildroot, it will build and
install the library, and this will cause other packages to link with it, right?

> (not easy to change now...)

 It's not *that* difficult to change...

1. Add an option BR2_PACKAGE_ZSTD_LIB (default to y because that's the legacy
situation).
2. Update all "optional dependencies" op libzstd to check BR2_PACKAGE_ZSTD_LIB
instead.


> For that youd need to stich together 2 runs.

 Er, what?


>> So I would reduce this patch to two options:
>>
>> - standard build
>> - small build

 Which is actually just a single option: full build (default y). Note that we
don't want "small build" as a boolean option, because that makes it impossible
for another package to select the full build (you can't "select
!BR2_PACKAGE_ZSTD_PROG_SMALL). We've been hit by this a couple of times already
in the past.


>> and depending on the options BR2_STATIC_LIBS / SHARED_STATIC_LIBS the
>> 'standard build' would result in a dynamically-linked object or a
>> static one.
> 
> Sounds reasonable.
> 
>>
>>> +
>>> +config BR2_PACKAGE_ZSTD_PROG_SMALL
>>> +       bool "Small build (static, less features)"
>>> +
>>> +endchoice
>>> +
>>> +endif
>>> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
>>> index a9499df4d0..ecad26e0df 100644
>>> --- a/package/zstd/zstd.mk
>>> +++ b/package/zstd/zstd.mk
>>> @@ -48,6 +48,11 @@ endif
>>>  ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
>>>
>>>  ZSTD_BUILD_PROG_TARGET := zstd-release
>>> +ifeq ($(BR2_PACKAGE_ZSTD_PROG_DYNAMIC),y)
>>> +ZSTD_BUILD_PROG_TARGET := zstd-dll
>>> +else ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
>>> +ZSTD_BUILD_PROG_TARGET := zstd-small
>>> +endif
>>
>> We don't generally use := in Buildroot, unless really needed. These
>> assignments could just be '='.
> 
> Ok
> 
>>
>>>
>>>  # Since v1.5.0 the dynamic library is built for
>>>  # multithreading, while the static library is not.
>>> @@ -78,7 +83,9 @@ define ZSTD_BUILD_CMDS
>>>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>>                 -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
>>>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
>>> +               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET) && \
>>> +       { [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \

 Am I crazy or is this actually saying

       { [ -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] && \

?

 However, wouldn't it be more logical to do

       { [ ! -e $(@D)/programs/zstd ] && \

>>> +               ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; }
>>
>> I find this code not particularly readable.
>> Why does the group need to be continued with '&&' ? If you don't do
>> that, a failure in the make will stop right there, which is fine.
>> This together with the use of the || shorthand, especially with the
>> negated condition, is not obvious.
> 
> Ok
>
>> Also, this hardlink is only actually needed for the 'zstd-small' case,
>> because both 'zstd-release' and 'zstd-dll' just generate a binary
>> called 'zstd'. In the latter cases, the [ ] condition will be false,
>> and nothing to be done.>>
>> So I would consider to leave the original make untouched and use
>> following instead:
>>
>>
>> ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
>> define ZSTD_HARDLINK_BUILD_PROG_TARGET
>>         ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd
>> endef
>> ZSTD_POST_BUILD_HOOKS += ZSTD_HARDLINK_BUILD_PROG_TARGET
>> endif
> 
> I disagree here, there are multiple variants aside of -small, like a
> decompress-only build,
> and I'd not want to depend on upstream behaviors whether a variant is
> "in-place" of zstd
> or as extra file.
> Even if its not gonna be supported officially there's value in locally
> using "zstd-decompress"
> and things just work.
> 
> Arguably with zstd-dll largely fixing the main issue (filesize) those
> variants are less tempting.

 So in conclusion, I think there should be just one config option
BR2_PACKAGE_ZSTD_PROG_FULL (default y) which builds zstd-dll if !static and
zstd-release if static.


> 
> Norbert
> (Waiting for some more feedback and possibly upstreaming  part of the
> series before rerolling)

 Good plan :-)


 Regards,
 Arnout

[Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0

[Arnout Vandecappelle]

On 25/05/2021 19:26, Norbert Lange wrote:
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

 Applied to master, thanks.

 Regards,
 Arnout

> ---
>  package/zstd/zstd.hash | 4 ++--
>  package/zstd/zstd.mk   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/package/zstd/zstd.hash b/package/zstd/zstd.hash
> index a979501e5f..2d7bf37b2a 100644
> --- a/package/zstd/zstd.hash
> +++ b/package/zstd/zstd.hash
> @@ -1,5 +1,5 @@
> -# From https://github.com/facebook/zstd/releases/download/v1.4.9/zstd-1.4.9.tar.gz.sha256
> -sha256  29ac74e19ea28659017361976240c4b5c5c24db3b89338731a6feb97c038d293  zstd-1.4.9.tar.gz
> +# From https://github.com/facebook/zstd/releases/download/v1.5.0/zstd-1.5.0.tar.gz.sha256
> +sha256  5194fbfa781fcf45b98c5e849651aa7b3b0a008c6b72d4a0db760f3002291e94  zstd-1.5.0.tar.gz
>  
>  # License files (locally computed)
>  sha256  2c1a7fa704df8f3a606f6fc010b8b5aaebf403f3aeec339a12048f1ba7331a0b  LICENSE
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 63ea8b1b35..194974147c 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -4,7 +4,7 @@
>  #
>  ################################################################################
>  
> -ZSTD_VERSION = 1.4.9
> +ZSTD_VERSION = 1.5.0
>  ZSTD_SITE = https://github.com/facebook/zstd/releases/download/v$(ZSTD_VERSION)
>  ZSTD_INSTALL_STAGING = YES
>  ZSTD_LICENSE = BSD-3-Clause or GPL-2.0
> 

[Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build

[Arnout Vandecappelle]

On 25/05/2021 19:26, Norbert Lange wrote:
> 1.5.0 uses Threads by default for cli tool and DSO,
> should not be necessary to do anything special.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

 Applied to master, thanks.

 Regards,
 Arnout

> ---
>  package/zstd/zstd.mk | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 194974147c..2a876376a2 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -78,21 +78,16 @@ define ZSTD_INSTALL_TARGET_CMDS
>  		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
>  endef
>  
> -# note: only limited 'HAVE_...' options for host library build only
> -HOST_ZSTD_OPTS = HAVE_THREAD=1
> +HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
>  
>  define HOST_ZSTD_BUILD_CMDS
>  	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
> -		-C $(@D)/lib libzstd.a-mt libzstd-mt
> -	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
> -		-C $(@D) zstd
> +		-C $(@D) zstd-release lib-release
>  endef
>  
>  define HOST_ZSTD_INSTALL_CMDS
>  	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
> -		DESTDIR=$(HOST_DIR) PREFIX=/usr -C $(@D)/lib install
> -	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(HOST_ZSTD_OPTS) \
> -		DESTDIR=$(HOST_DIR) PREFIX=/usr -C $(@D)/programs install
> +		-C $(@D) install
>  endef
>  
>  $(eval $(generic-package))
> 

[Buildroot] [PATCH v2 3/5] package/zstd: rework build and install

[Norbert Lange]

Am Mo., 14. Juni 2021 um 21:50 Uhr schrieb Arnout Vandecappelle
<arnout@mind.be>:
>
>  Hi Norbert,
>
> On 25/05/2021 19:26, Norbert Lange wrote:
> > 1.5.0 uses Threads by default for cli tool and DSO,
> > the build now does the same unless:
> >
> > If only static libraries are build, then build
> > that library like the DSO is normally built.
> >
> > This should ensure that programs requsting the DSO
> > will always get the multithreaded version.
>
>  I don't understand what this patch is trying to do...
>
>  Before this patch, in the threads case, the build would be done with threads
> both for the program (HAVE_THREAD=1) and the libraries (-mt appended). For the
> no-threads case, the build is done without threads support for both the program
> (HAVE_THREAD=0) and the shared&static libraries (no -mt appended) - except that
> since 1.5.0, the shared library will be built with thread support which will
> probably result in a build failure.

I would want the static library to be built without threads (like upstream).
the expectation should be:

1) gcc ... -llibzstd
    pickup the DSO, with multithread support
2) gcc ... ../usr/lib/libzstd.a
    pickup the static library without multithread support

in case there are no shared libraries, 1) will use the static library
instead, so it should be compiled like the DSO normally would.
in case there are no static libraries, 2) plainly wont work

For the program, the Makefile should be able to determine whether
threads are available or not.

>
>  So as far as I can see, the only thing that needs to be done is:
>
> ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> else
> ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
> endif
>
>
>  You patch, however, builds the static library with thread support if no dynamic
> library is built, and without thread support if a dynamic library is built. This
> makes no sense whatsoever to me...
>
>
>  Can you clarify the reasoning?

I hope I did. Might be that I don't understand the build failures you
are talking about,
does the zstd package fail, or some user of libzstd?

>
>  For now, I've marked the patch as Changes Requested. Since I did apply the
> first patch, we will probably start seeing build failures on nothreads, so we'll
> need a fix soonish...

Sure, I should get a mail as commit author!?

Norbert

>
>  Regards,
>  Arnout
>
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >
> > ---
> > v1->v2:
> > *   rebased against upstream/master
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> > ---
> >  package/zstd/zstd.mk | 52 ++++++++++++++++++++++++++++----------------
> >  1 file changed, 33 insertions(+), 19 deletions(-)
> >
> > diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> > index 2a876376a2..a7a5ba4e50 100644
> > --- a/package/zstd/zstd.mk
> > +++ b/package/zstd/zstd.mk
> > @@ -12,6 +12,8 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
> >  ZSTD_CPE_ID_VENDOR = facebook
> >  ZSTD_CPE_ID_PRODUCT = zstandard
> >
> > +ZSTD_OPTS += PREFIX=/usr
> > +
> >  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> >  ZSTD_OPTS += HAVE_THREAD=1
> >  else
> > @@ -39,43 +41,55 @@ else
> >  ZSTD_OPTS += HAVE_LZ4=0
> >  endif
> >
> > -ifeq ($(BR2_STATIC_LIBS),y)
> > -ZSTD_BUILD_LIBS = libzstd.a
> > -ZSTD_INSTALL_LIBS = install-static
> > -else ifeq ($(BR2_SHARED_LIBS),y)
> > -ZSTD_BUILD_LIBS = libzstd
> > -ZSTD_INSTALL_LIBS = install-shared
> > +ZSTD_BUILD_PROG_TARGET := zstd-release
> > +
> > +# Since v1.5.0 the dynamic library is built for
> > +# multithreading, while the static library is not.
> > +#
> > +# Keep those defaults, unless Buildroot is not
> > +# providing the dynamic library and the
> > +# static library will be automatically used instead.
> > +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +ZSTD_INSTALL_LIBS += install-static
> > +ifeq ($(BR2_STATIC_LIBS)$(BR2_TOOLCHAIN_HAS_THREADS),yy)
> > +# Use the static lib as replacement for the mt dynlib
> > +ZSTD_BUILD_LIBS += libzstd.a-mt
> >  else
> > -ZSTD_BUILD_LIBS = libzstd.a libzstd
> > -ZSTD_INSTALL_LIBS = install-static install-shared
> > +ZSTD_BUILD_LIBS += libzstd.a-nomt
> > +endif
> >  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_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +ZSTD_INSTALL_LIBS += install-shared
> >  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> > -ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> > +ZSTD_BUILD_LIBS += libzstd-mt
> > +else
> > +ZSTD_BUILD_LIBS += libzstd-nomt
> > +endif
> >  endif
> >
> >  define ZSTD_BUILD_CMDS
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             -C $(@D)/lib $(ZSTD_BUILD_LIBS)
> > +             -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             -C $(@D) zstd
> > +             -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
> >  endef
> >
> >  define ZSTD_INSTALL_STAGING_CMDS
> > +     [ -e $(@D)/programs/zstd ] && [ -e $(@D)/lib/libzstd.pc ]
> > +     $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > +             -C $(@D)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
> > +             install-pc install-includes
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             DESTDIR=$(STAGING_DIR) PREFIX=/usr -C $(@D)/lib \
> > -             install-pc install-includes $(ZSTD_INSTALL_LIBS)
> > +             -C $(@D)/programs DESTDIR=$(STAGING_DIR) install
> >  endef
> >
> >  define ZSTD_INSTALL_TARGET_CMDS
> > +     [ -e $(@D)/programs/zstd ]
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
> > +             -C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
> > +             -C $(@D)/programs DESTDIR=$(TARGET_DIR) install
> >  endef
> >
> >  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
> >

[Buildroot] [PATCH v2 4/5] package/zstd: Change Build options

[Norbert Lange]

Am Mo., 14. Juni 2021 um 21:56 Uhr schrieb Arnout Vandecappelle
<arnout@mind.be>:
>
>
>
> On 25/05/2021 19:26, Norbert Lange wrote:
> > Disable the legacy format, these are just needed for
> > decompressing files created with pre-release version.
> >
> > Use Buildroot's setting for optimization, zstd's build system
> > overrides CFLAGS, but MOREFLAGS can override again.
> > Quick tests show that using -O2 (like buildroot)
> > is actually a little faster than -O3 on x86_64 Atoms.
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> > ---
> >  package/zstd/zstd.mk | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> > index a7a5ba4e50..a9499df4d0 100644
> > --- a/package/zstd/zstd.mk
> > +++ b/package/zstd/zstd.mk
> > @@ -13,6 +13,10 @@ ZSTD_CPE_ID_VENDOR = facebook
> >  ZSTD_CPE_ID_PRODUCT = zstandard
> >
> >  ZSTD_OPTS += PREFIX=/usr
> > +ZSTD_OPTS += ZSTD_LEGACY_SUPPORT=0
> > +
> > +# zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
> > +ZSTD_OPTS_MOREFLAGS += $(TARGET_OPTIMIZATION)
>
>  Wouldn't it make more sense to apply *all* buildroot's flags here, i.e.
>
> ZSTD_OPTS += MOREFLAGS="$(TARGET_CFLAGS)"

The CFLAGS are already picked up from the environment,
the package normally should be able to add options on top.

>
> (also note:
>  - intermediate variable ZSTD_OPTS_MOREFLAGS is not needed for anything;

Not yet. There are a couple of interesting options available with macros,
ex. ZSTD_LIB_MINIFY. see lib/README.md
But I think ill cut that out for now.

>  - we usually quote only the "$(TARGET_CFLAGS)" part, not including the
> MOREFLAGS= part)

OK.

>
>
>  Regards,
>  Arnout
>
> >
> >  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> >  ZSTD_OPTS += HAVE_THREAD=1
> > @@ -41,6 +45,8 @@ else
> >  ZSTD_OPTS += HAVE_LZ4=0
> >  endif
> >
> > +ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
> > +
> >  ZSTD_BUILD_PROG_TARGET := zstd-release
> >
> >  # Since v1.5.0 the dynamic library is built for
> >

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Norbert Lange]

Am Mo., 14. Juni 2021 um 22:20 Uhr schrieb Arnout Vandecappelle
<arnout@mind.be>:
>
>
>
> On 01/06/2021 11:20, Norbert Lange wrote:
> > Am Mo., 31. Mai 2021 um 17:02 Uhr schrieb Thomas De Schampheleire
> > <patrickdepinguin@gmail.com>:
> >>
> >> Hello,
> >>
> >> El mar, 25 may 2021 a las 19:28, Norbert Lange (<nolange79@gmail.com>) escribi?:
> >>>
> >>> There are a couple of targets to build the cli tool with less
> >>> features (no legacy-support, benchmarking),
> >>> or dynamically linked to the library.
> >>>
> >>> Add an option to choose the variant.
> >>>
> >>> To allow the installation step to pick the variant, it
> >>> has to be copied to the normal location.
> >>>
> >>> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >>> ---
> >>>  package/zstd/Config.in | 21 +++++++++++++++++++++
> >>>  package/zstd/zstd.mk   |  9 ++++++++-
> >>>  2 files changed, 29 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/package/zstd/Config.in b/package/zstd/Config.in
> >>> index 9fa70c65cc..0d2ab84771 100644
> >>> --- a/package/zstd/Config.in
> >>> +++ b/package/zstd/Config.in
> >>> @@ -10,3 +10,24 @@ config BR2_PACKAGE_ZSTD
> >>>           compression formats
> >>>
> >>>           https://facebook.github.io/zstd
> >>> +
> >>> +if BR2_PACKAGE_ZSTD
> >>> +
> >>> +choice
> >>> +       prompt "Executable flavor"
> >>> +       help
> >>> +         Pick between variants of the zstd tool.
> >>> +
> >>> +config BR2_PACKAGE_ZSTD_PROG_DEFAULT
> >>> +       bool "Default build (static, full-featured)"
> >>> +
> >>> +config BR2_PACKAGE_ZSTD_PROG_DYNAMIC
> >>> +       bool "Dynamic build (needs library with identical version)"
> >>> +       depends on !BR2_STATIC_LIBS
> >>
> >> Is there a reason not to make this one the default, instead of the
> >> static variant?
> >
> > Hello.
> >
> > Gradual change, as gambit to get the patches upstreamed faster =)
> > I agree, dynamic should be default.
>
>  Adding config options is not very likely to get patches upstreamed faster :-)

Damnit, thought having a choice is easier than figuring out which one
should be used =)

>
> >
> >> From my point of view, the PROG_DYNAMIC has as main advantage a
> >> reduction in rootfs size, since zstd is quite large and with the
> >> static version duplicating part of the shared object.
> >>
> >> Earlier I sent a patch for this (exact implementation was still based
> >> on v1.4.9 and sending of the update patch was pending the new 1.5.0
> >> release), see:
> >> http://patchwork.ozlabs.org/project/buildroot/patch/20201204095703.4714-1-patrickdepinguin at gmail.com/
> >>
> >> In context of Buildroot, the expectation is that one builds the entire
> >> thing in one go. This means that binary and library will be from the
> >> same version anyway.
> >> In that sense, the option description may be confusing.
> >>
> >> In fact, I even wonder if it is needed to provide the PROG_DEFAULT at
> >> all. If shared libraries are supported, I see no reason why one would
> >> like a static version of the binary.
> >
> > upstream treats DSO and programs separately, the static binary could
> > have some more flags not available for DSO which seem to be considered
> > less configurable.
>
>  I looked at the source and simply see no way how this is even possible - the
> command line argument parsing is entirely in the source files in the program
> directory, and there are no different -D options for zstd-dll...
>
> > Some time ago I had to use the static library cause some features
> > where only available there.
>
>  Maybe that was in the past and is no longer the case?

Its still the case, see lib/README.md and the macro ZSTD_STATIC_LINKING_ONLY,
probably less than some time ago.

The statically program build could be using those extra features too.

>
>  Note BTW that zstd programs don't use the static library - they (re)build the
> source files directly. That's how it's possible that the static library has no
> thread support while the programs do have thread support.

there is the static library, dynamic library and the tool,
so there are 3 sets of options possible.

>
>
> > Also despite expectation, you cant for example just build zstd (static)
> > without multiple packages automatically linking to libzstd.
>
>  What you're saying is: if you enable zstd in Buildroot, it will build and
> install the library, and this will cause other packages to link with it, right?

Yes

>
> > (not easy to change now...)
>
>  It's not *that* difficult to change...
>
> 1. Add an option BR2_PACKAGE_ZSTD_LIB (default to y because that's the legacy
> situation).
> 2. Update all "optional dependencies" op libzstd to check BR2_PACKAGE_ZSTD_LIB
> instead.

Yeah, touching multiple packages, given how long some of my patches
linger around that sounds like fun to rebase every few months.
Probably easy if you have write permissions ;)

>
>
> > For that youd need to stich together 2 runs.
>
>  Er, what?

I'd mean pick a buildroot config, enable zstd, pick the static zstd
tool from there
disable zstd, clean rebuild, merge that with the zstd tool from before.
Then you can have the static/small zstd tool and nothing links to libzstd.

>
>
> >> So I would reduce this patch to two options:
> >>
> >> - standard build
> >> - small build
>
>  Which is actually just a single option: full build (default y). Note that we
> don't want "small build" as a boolean option, because that makes it impossible
> for another package to select the full build (you can't "select
> !BR2_PACKAGE_ZSTD_PROG_SMALL). We've been hit by this a couple of times already
> in the past.
>
>
> >> and depending on the options BR2_STATIC_LIBS / SHARED_STATIC_LIBS the
> >> 'standard build' would result in a dynamically-linked object or a
> >> static one.
> >
> > Sounds reasonable.
> >
> >>
> >>> +
> >>> +config BR2_PACKAGE_ZSTD_PROG_SMALL
> >>> +       bool "Small build (static, less features)"
> >>> +
> >>> +endchoice
> >>> +
> >>> +endif
> >>> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> >>> index a9499df4d0..ecad26e0df 100644
> >>> --- a/package/zstd/zstd.mk
> >>> +++ b/package/zstd/zstd.mk
> >>> @@ -48,6 +48,11 @@ endif
> >>>  ZSTD_OPTS += "MOREFLAGS=$(ZSTD_OPTS_MOREFLAGS)"
> >>>
> >>>  ZSTD_BUILD_PROG_TARGET := zstd-release
> >>> +ifeq ($(BR2_PACKAGE_ZSTD_PROG_DYNAMIC),y)
> >>> +ZSTD_BUILD_PROG_TARGET := zstd-dll
> >>> +else ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
> >>> +ZSTD_BUILD_PROG_TARGET := zstd-small
> >>> +endif
> >>
> >> We don't generally use := in Buildroot, unless really needed. These
> >> assignments could just be '='.
> >
> > Ok
> >
> >>
> >>>
> >>>  # Since v1.5.0 the dynamic library is built for
> >>>  # multithreading, while the static library is not.
> >>> @@ -78,7 +83,9 @@ define ZSTD_BUILD_CMDS
> >>>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> >>>                 -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
> >>>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> >>> -               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
> >>> +               -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET) && \
> >>> +       { [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
>
>  Am I crazy or is this actually saying
>
>        { [ -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] && \
>
> ?
>
>  However, wouldn't it be more logical to do
>
>        { [ ! -e $(@D)/programs/zstd ] && \

That would fail and stop the build if zstd exists (which is the case
for zstd-release and zstd-dll but not for zstd-small,
zstd-decompress).
ie. the build either creates $(@D)/programs/zstd or
$(@D)/programs/$(ZSTD_BUILD_PROG_TARGET)

The code that correctly succeeds
when $(@D)/programs/zstd exists or a link to $(ZSTD_BUILD_PROG_TARGET)
can be made is:

[ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
  ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd

or

[ -e $(@D)/programs/zstd ] || \
  ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd

>
> >>> +               ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; }
> >>
> >> I find this code not particularly readable.
> >> Why does the group need to be continued with '&&' ? If you don't do
> >> that, a failure in the make will stop right there, which is fine.
> >> This together with the use of the || shorthand, especially with the
> >> negated condition, is not obvious.
> >
> > Ok
> >
> >> Also, this hardlink is only actually needed for the 'zstd-small' case,
> >> because both 'zstd-release' and 'zstd-dll' just generate a binary
> >> called 'zstd'. In the latter cases, the [ ] condition will be false,
> >> and nothing to be done.>>
> >> So I would consider to leave the original make untouched and use
> >> following instead:
> >>
> >>
> >> ifeq ($(BR2_PACKAGE_ZSTD_PROG_SMALL),y)
> >> define ZSTD_HARDLINK_BUILD_PROG_TARGET
> >>         ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd
> >> endef
> >> ZSTD_POST_BUILD_HOOKS += ZSTD_HARDLINK_BUILD_PROG_TARGET
> >> endif
> >
> > I disagree here, there are multiple variants aside of -small, like a
> > decompress-only build,
> > and I'd not want to depend on upstream behaviors whether a variant is
> > "in-place" of zstd
> > or as extra file.
> > Even if its not gonna be supported officially there's value in locally
> > using "zstd-decompress"
> > and things just work.
> >
> > Arguably with zstd-dll largely fixing the main issue (filesize) those
> > variants are less tempting.
>
>  So in conclusion, I think there should be just one config option
> BR2_PACKAGE_ZSTD_PROG_FULL (default y) which builds zstd-dll if !static and
> zstd-release if static.

hmm, so then we don't need a config?
I mean zstd-small only has an advantage if no libzstd is used
(zstd-dll is smaller). At least as it it now,
perhaps config options with some macros from lib/README.md would make
more sense...

>
>
> >
> > Norbert
> > (Waiting for some more feedback and possibly upstreaming  part of the
> > series before rerolling)
>
>  Good plan :-)

Procrastination is a super-power.

Norbert

>
>
>  Regards,
>  Arnout
>

[Buildroot] [PATCH v2 3/5] package/zstd: rework build and install

[Arnout Vandecappelle]

On 14/06/2021 23:04, Norbert Lange wrote:
> Am Mo., 14. Juni 2021 um 21:50 Uhr schrieb Arnout Vandecappelle
> <arnout@mind.be>:
>>
>>  Hi Norbert,
>>
>> On 25/05/2021 19:26, Norbert Lange wrote:
>>> 1.5.0 uses Threads by default for cli tool and DSO,
>>> the build now does the same unless:
>>>
>>> If only static libraries are build, then build
>>> that library like the DSO is normally built.
>>>
>>> This should ensure that programs requsting the DSO
>>> will always get the multithreaded version.
>>
>>  I don't understand what this patch is trying to do...
>>
>>  Before this patch, in the threads case, the build would be done with threads
>> both for the program (HAVE_THREAD=1) and the libraries (-mt appended). For the
>> no-threads case, the build is done without threads support for both the program
>> (HAVE_THREAD=0) and the shared&static libraries (no -mt appended) - except that
>> since 1.5.0, the shared library will be built with thread support which will
>> probably result in a build failure.
> 
> I would want the static library to be built without threads (like upstream).

 I'm afraid that "like upstream" is not a great argument if upstream does crazy
stuff (which is definitely the case in zstd).

> the expectation should be:
> 
> 1) gcc ... -llibzstd
>     pickup the DSO, with multithread support

 This should actually be:

    pickup the library (shared or static, depending on BR2_STATIC_LIBS), which
always has multithread support.

 This one is OK and makes sense.


> 2) gcc ... ../usr/lib/libzstd.a
>     pickup the static library without multithread support

 This should actually be:

    pickup the static library which has multithread support if
BR2_STATIC_LIBS=y, else it doesn't have multithread support.

 That weird condition makes no sense to me. It means that your application (if
it is multithreaded) will work fine if BR2_SHARED_STATIC=y and break if you set
BR2_STATIC_LIBS=y.

 But OK, your point is that if you link explicitly with the static library, you
should assume that it doesn't have thread support (although sometimes it *does*
have thread support, but who cares?).


> in case there are no shared libraries, 1) will use the static library
> instead, so it should be compiled like the DSO normally would.
> in case there are no static libraries, 2) plainly wont work
> 
> For the program, the Makefile should be able to determine whether
> threads are available or not.
> 
>>
>>  So as far as I can see, the only thing that needs to be done is:
>>
>> ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
>> ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
>> else
>> ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
>> endif
>>
>>
>>  You patch, however, builds the static library with thread support if no dynamic
>> library is built, and without thread support if a dynamic library is built. This
>> makes no sense whatsoever to me...
>>
>>
>>  Can you clarify the reasoning?
> 
> I hope I did. Might be that I don't understand the build failures you
> are talking about,
> does the zstd package fail, or some user of libzstd?

 I expect the zstd package to start failing, because it will no try to build the
multithread version on toolchains where threads are not available.

 But I'm apparently wrong - there are failures of zstd, but for a different
reason [1].

> 
>>
>>  For now, I've marked the patch as Changes Requested. Since I did apply the
>> first patch, we will probably start seeing build failures on nothreads, so we'll
>> need a fix soonish...
> 
> Sure, I should get a mail as commit author!?

 You would if you add the package to yourself in DEVELOPERS.


 Regards,
 Arnout

[1] http://autobuild.buildroot.net/?reason=zstd%

> 
> Norbert
> 
>>
>>  Regards,
>>  Arnout
>>
>>>
>>> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>>>
>>> ---
>>> v1->v2:
>>> *   rebased against upstream/master
>>>
>>> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>>> ---
>>>  package/zstd/zstd.mk | 52 ++++++++++++++++++++++++++++----------------
>>>  1 file changed, 33 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
>>> index 2a876376a2..a7a5ba4e50 100644
>>> --- a/package/zstd/zstd.mk
>>> +++ b/package/zstd/zstd.mk
>>> @@ -12,6 +12,8 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
>>>  ZSTD_CPE_ID_VENDOR = facebook
>>>  ZSTD_CPE_ID_PRODUCT = zstandard
>>>
>>> +ZSTD_OPTS += PREFIX=/usr
>>> +
>>>  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
>>>  ZSTD_OPTS += HAVE_THREAD=1
>>>  else
>>> @@ -39,43 +41,55 @@ else
>>>  ZSTD_OPTS += HAVE_LZ4=0
>>>  endif
>>>
>>> -ifeq ($(BR2_STATIC_LIBS),y)
>>> -ZSTD_BUILD_LIBS = libzstd.a
>>> -ZSTD_INSTALL_LIBS = install-static
>>> -else ifeq ($(BR2_SHARED_LIBS),y)
>>> -ZSTD_BUILD_LIBS = libzstd
>>> -ZSTD_INSTALL_LIBS = install-shared
>>> +ZSTD_BUILD_PROG_TARGET := zstd-release
>>> +
>>> +# Since v1.5.0 the dynamic library is built for
>>> +# multithreading, while the static library is not.
>>> +#
>>> +# Keep those defaults, unless Buildroot is not
>>> +# providing the dynamic library and the
>>> +# static library will be automatically used instead.
>>> +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
>>> +ZSTD_INSTALL_LIBS += install-static
>>> +ifeq ($(BR2_STATIC_LIBS)$(BR2_TOOLCHAIN_HAS_THREADS),yy)
>>> +# Use the static lib as replacement for the mt dynlib
>>> +ZSTD_BUILD_LIBS += libzstd.a-mt
>>>  else
>>> -ZSTD_BUILD_LIBS = libzstd.a libzstd
>>> -ZSTD_INSTALL_LIBS = install-static install-shared
>>> +ZSTD_BUILD_LIBS += libzstd.a-nomt
>>> +endif
>>>  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_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
>>> +ZSTD_INSTALL_LIBS += install-shared
>>>  ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
>>> -ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
>>> +ZSTD_BUILD_LIBS += libzstd-mt
>>> +else
>>> +ZSTD_BUILD_LIBS += libzstd-nomt
>>> +endif
>>>  endif
>>>
>>>  define ZSTD_BUILD_CMDS
>>>       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -             -C $(@D)/lib $(ZSTD_BUILD_LIBS)
>>> +             -C $(@D)/lib $(addsuffix -release,$(ZSTD_BUILD_LIBS) libzstd.pc)
>>>       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -             -C $(@D) zstd
>>> +             -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
>>>  endef
>>>
>>>  define ZSTD_INSTALL_STAGING_CMDS
>>> +     [ -e $(@D)/programs/zstd ] && [ -e $(@D)/lib/libzstd.pc ]
>>> +     $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> +             -C $(@D)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
>>> +             install-pc install-includes
>>>       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -             DESTDIR=$(STAGING_DIR) PREFIX=/usr -C $(@D)/lib \
>>> -             install-pc install-includes $(ZSTD_INSTALL_LIBS)
>>> +             -C $(@D)/programs DESTDIR=$(STAGING_DIR) install
>>>  endef
>>>
>>>  define ZSTD_INSTALL_TARGET_CMDS
>>> +     [ -e $(@D)/programs/zstd ]
>>>       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
>>> +             -C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
>>>       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
>>> -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
>>> +             -C $(@D)/programs DESTDIR=$(TARGET_DIR) install
>>>  endef
>>>
>>>  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
>>>

[Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary

[Arnout Vandecappelle]

On 14/06/2021 23:47, Norbert Lange wrote:
> Am Mo., 14. Juni 2021 um 22:20 Uhr schrieb Arnout Vandecappelle
> <arnout@mind.be>:
>>
>>
>>
>> On 01/06/2021 11:20, Norbert Lange wrote:
>>> Am Mo., 31. Mai 2021 um 17:02 Uhr schrieb Thomas De Schampheleire
>>> <patrickdepinguin@gmail.com>:
>>>>
[snip]
>>>> In fact, I even wonder if it is needed to provide the PROG_DEFAULT at
>>>> all. If shared libraries are supported, I see no reason why one would
>>>> like a static version of the binary.
>>>
>>> upstream treats DSO and programs separately, the static binary could
>>> have some more flags not available for DSO which seem to be considered
>>> less configurable.
>>
>>  I looked at the source and simply see no way how this is even possible - the
>> command line argument parsing is entirely in the source files in the program
>> directory, and there are no different -D options for zstd-dll...
>>
>>> Some time ago I had to use the static library cause some features
>>> where only available there.
>>
>>  Maybe that was in the past and is no longer the case?
> 
> Its still the case, see lib/README.md and the macro ZSTD_STATIC_LINKING_ONLY,
> probably less than some time ago.

 That's for the static library. The programs always define
ZSTD_STATIC_LINKING_ONLY, even when building zstd-dll. Huh, I wonder how that
even works...


> The statically program build could be using those extra features too.

 They *are* using those extra features, so I'm not sure how the dll thing works
even... Maybe they just use inline functions or something like that.



[snip]
>>  However, wouldn't it be more logical to do
>>
>>        { [ ! -e $(@D)/programs/zstd ] && \
> 
> That would fail and stop the build if zstd exists (which is the case
> for zstd-release and zstd-dll but not for zstd-small,
> zstd-decompress).

 Ah, yes, I knew there was a reason why I prefer if ...; then over ... ||

> ie. the build either creates $(@D)/programs/zstd or
> $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET)
> 
> The code that correctly succeeds
> when $(@D)/programs/zstd exists or a link to $(ZSTD_BUILD_PROG_TARGET)
> can be made is:
> 
> [ ! -e $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) ] || \
>   ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd
> 
> or
> 
> [ -e $(@D)/programs/zstd ] || \
>   ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd

 Or

	if [ ! -e $(@D)/programs/zstd ]; then \
		ln -f $(@D)/programs/$(ZSTD_BUILD_PROG_TARGET) $(@D)/programs/zstd; \
	fi


[snip]
>>  So in conclusion, I think there should be just one config option
>> BR2_PACKAGE_ZSTD_PROG_FULL (default y) which builds zstd-dll if !static and
>> zstd-release if static.
> 
> hmm, so then we don't need a config?
> I mean zstd-small only has an advantage if no libzstd is used
> (zstd-dll is smaller). At least as it it now,

 Good point. Your v3 is a lot simpler :-)


 Regards,
 Arnout


> perhaps config options with some macros from lib/README.md would make
> more sense...
> 
>>
>>
>>>
>>> Norbert
>>> (Waiting for some more feedback and possibly upstreaming  part of the
>>> series before rerolling)
>>
>>  Good plan :-)
> 
> Procrastination is a super-power.
> 
> Norbert
> 
>>
>>
>>  Regards,
>>  Arnout
>>