[Buildroot] [PATCH v3 1/3] package/zstd: rework build and install

[Buildroot] [PATCH v3 1/3] 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>

---
v2->v3:
*   use normal = for assignment
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..95f25ebfa4 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 v3 2/3] 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>
---
v2->v3:
*   Remove ZSTD_OPTS_MOREFLAGS variable
---
 package/zstd/zstd.mk | 4 ++++
 1 file changed, 4 insertions(+)

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

[Buildroot] [PATCH v3 3/3] package/zstd: Prefer dynamically linked tool

[Norbert Lange]

If the libzstd DSO is available, then link the
tool against it.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
v2->v3:
*   Drop config options, just use zstd-dll if possible
---
 package/zstd/zstd.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 54ce4133fe..726c6ae4a4 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -45,7 +45,12 @@ endif
 # zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
 ZSTD_OPTS += MOREFLAGS="$(TARGET_OPTIMIZATION)"
 
+# prefer zstd-dll unless no library is available
+ifeq ($(BR2_STATIC_LIBS),y)
 ZSTD_BUILD_PROG_TARGET = zstd-release
+else
+ZSTD_BUILD_PROG_TARGET = zstd-dll
+endif
 
 # Since v1.5.0 the dynamic library is built for
 # multithreading, while the static library is not.
-- 
2.30.2

[Buildroot] [PATCH v4 1/3] package/zstd: rework build and install

[Norbert Lange]

1.5.0 uses Threads by default for cli tool and DSO,
so override that in case the toolchain has none.

make sure that everything is built in the build step,
and fail the install if files are missing
instead of lazily building them.

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

---
v3->v4:
*   revert to previous scheme of BR2_TOOLCHAIN_HAS_THREADS
    fixing build options for both static and shared libs.
v2->v3:
*   use normal = for assignment
v1->v2:
*   rebased against upstream/master

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

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 2a876376a2..5674e85bfe 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -12,11 +12,7 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
 ZSTD_CPE_ID_VENDOR = facebook
 ZSTD_CPE_ID_PRODUCT = zstandard
 
-ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
-ZSTD_OPTS += HAVE_THREAD=1
-else
-ZSTD_OPTS += HAVE_THREAD=0
-endif
+ZSTD_OPTS += PREFIX=/usr
 
 ifeq ($(BR2_PACKAGE_ZLIB),y)
 ZSTD_DEPENDENCIES += zlib
@@ -39,43 +35,52 @@ 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
-else
-ZSTD_BUILD_LIBS = libzstd.a libzstd
-ZSTD_INSTALL_LIBS = install-static install-shared
+ZSTD_BUILD_PROG_TARGET = zstd-release
+
+ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+ZSTD_BUILD_LIBS += libzstd.a
+ZSTD_INSTALL_LIBS += install-static
+endif
+ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+ZSTD_BUILD_LIBS += libzstd
+ZSTD_INSTALL_LIBS += install-shared
 endif
 
+# Since v1.5.0 the dynamic library is built for
+# multithreading, while the static library is not.
+#
 # 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_OPTS += HAVE_THREAD=1
 ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
+else
+ZSTD_OPTS += HAVE_THREAD=0
+ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
 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)/lib/libzstd.pc ]
 	$(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)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
+		install-pc install-includes
 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)/programs DESTDIR=$(TARGET_DIR) install
 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
-		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
+		-C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
 endef
 
 HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
-- 
2.30.2

Re: [Buildroot] [PATCH v4 1/3] package/zstd: rework build and install

[Arnout Vandecappelle]

 Hi Norbert,

 Sorry for not responding earlier - I apparently mis-filed this series and it
went off my backlog. It's only by looking at Fabrice's later patch that I
discovered this one again.


On 17/06/2021 11:14, Norbert Lange wrote:
> 1.5.0 uses Threads by default for cli tool and DSO,
> so override that in case the toolchain has none.

 It helps if you include a Fixes: tag in the commit message - that way it's
visible in patchwork that this patch fixes something and it's going to be
applied faster. Also the subject line suggests that the patch is just
refactoring, not fixing anything.


> make sure that everything is built in the build step,
> and fail the install if files are missing
> instead of lazily building them.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> 
> ---
> v3->v4:
> *   revert to previous scheme of BR2_TOOLCHAIN_HAS_THREADS
>     fixing build options for both static and shared libs.
> v2->v3:
> *   use normal = for assignment
> v1->v2:
> *   rebased against upstream/master
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/zstd/zstd.mk | 45 ++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 2a876376a2..5674e85bfe 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -12,11 +12,7 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
>  ZSTD_CPE_ID_VENDOR = facebook
>  ZSTD_CPE_ID_PRODUCT = zstandard
>  
> -ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> -ZSTD_OPTS += HAVE_THREAD=1
> -else
> -ZSTD_OPTS += HAVE_THREAD=0
> -endif
> +ZSTD_OPTS += PREFIX=/usr

 I think that this is not just refactoring, but is actually needed for the .pc
file which is now built during the build step instead of the install step. Is
that correct? In that case, please mention it in the commit message.

>  
>  ifeq ($(BR2_PACKAGE_ZLIB),y)
>  ZSTD_DEPENDENCIES += zlib
> @@ -39,43 +35,52 @@ 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
> -else
> -ZSTD_BUILD_LIBS = libzstd.a libzstd
> -ZSTD_INSTALL_LIBS = install-static install-shared
> +ZSTD_BUILD_PROG_TARGET = zstd-release
> +
> +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_BUILD_LIBS += libzstd.a
> +ZSTD_INSTALL_LIBS += install-static
> +endif
> +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_BUILD_LIBS += libzstd
> +ZSTD_INSTALL_LIBS += install-shared

 AFAICS this change doesn't actually change anything, it's just rewriting. You
may claim the result is better, but it's really a matter of taste. We generally
want to avoid making changes that are just "it looks nicer" if there's no real
benefit (which can be, for example, a follow-up patch makes use of the new
structure and would be more complicated in the old structure).

 For sure, such a change should be in a separate patch that doesn't do any
functional change at the same time.

>  endif
>  
> +# Since v1.5.0 the dynamic library is built for
> +# multithreading, while the static library is not.
> +#
>  # 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_OPTS += HAVE_THREAD=1
>  ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> +else
> +ZSTD_OPTS += HAVE_THREAD=0
> +ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
>  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)

 There's no explanation about the -release suffix in the commit message.

 Since we already do override assignment to ZSTD_BUILD_LIBS, I think it would be
nicer to put

ZSTD_BUILD_LIBS := $(addsuffix -release,$(ZSTD_BUILD_LIBS))

just after the threads condition above.


>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		-C $(@D) zstd
> +		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)

 This change of building top-level to building in the programs subdirectory is
also not clearly explained in the commit message (I mean *why* this is changed,
not simply that it is changed).

>  endef
>  
>  define ZSTD_INSTALL_STAGING_CMDS
> +	[ -e $(@D)/lib/libzstd.pc ]

 This command serves no purpose.

 I understand that it's a way for you to "assert" that the pc file is indeed
built, but because you added it in the build commands we can be pretty sure that
it's going to be there.

 We normally don't have any assert like that in our install commands. It's also
pretty pointless - you probably discovered that it was being built at
installation time by looking at the build log, and that's the only way that
you'd discover that *that* is the file that needs to be checked. If this ever
breaks again, it's more than likely due to a different file.


>  	$(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)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
> +		install-pc install-includes

 Other than the PREFIX which is now part of ZSTD_OPTS, I believe nothing
actually changed here, right? Please try to keep the diff small in such a case.

>  endef
>  
>  define ZSTD_INSTALL_TARGET_CMDS
> +	[ -e $(@D)/programs/zstd ]

 Again, not needed.

>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
> +		-C $(@D)/programs DESTDIR=$(TARGET_DIR) install

 Again, spurious reordering of the options.


 Regards,
 Arnout

>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
> +		-C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
>  endef
>  
>  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
> 
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v4 1/3] package/zstd: rework build and install

[Norbert Lange]

Am Mi., 4. Aug. 2021 um 15:01 Uhr schrieb Arnout Vandecappelle <arnout@mind.be>:
>
>  Hi Norbert,
>
>  Sorry for not responding earlier - I apparently mis-filed this series and it
> went off my backlog. It's only by looking at Fabrice's later patch that I
> discovered this one again.
>
>
> On 17/06/2021 11:14, Norbert Lange wrote:
> > 1.5.0 uses Threads by default for cli tool and DSO,
> > so override that in case the toolchain has none.
>
>  It helps if you include a Fixes: tag in the commit message - that way it's
> visible in patchwork that this patch fixes something and it's going to be
> applied faster. Also the subject line suggests that the patch is just
> refactoring, not fixing anything.

It was a fix for a part of the series, so nothing to fix
unless only a part gets applied.
>
>
> > make sure that everything is built in the build step,
> > and fail the install if files are missing
> > instead of lazily building them.
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >
> > ---
> > v3->v4:
> > *   revert to previous scheme of BR2_TOOLCHAIN_HAS_THREADS
> >     fixing build options for both static and shared libs.
> > v2->v3:
> > *   use normal = for assignment
> > v1->v2:
> > *   rebased against upstream/master
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
> > ---
> >  package/zstd/zstd.mk | 45 ++++++++++++++++++++++++--------------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> > index 2a876376a2..5674e85bfe 100644
> > --- a/package/zstd/zstd.mk
> > +++ b/package/zstd/zstd.mk
> > @@ -12,11 +12,7 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
> >  ZSTD_CPE_ID_VENDOR = facebook
> >  ZSTD_CPE_ID_PRODUCT = zstandard
> >
> > -ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> > -ZSTD_OPTS += HAVE_THREAD=1
> > -else
> > -ZSTD_OPTS += HAVE_THREAD=0
> > -endif
> > +ZSTD_OPTS += PREFIX=/usr
>
>  I think that this is not just refactoring, but is actually needed for the .pc
> file which is now built during the build step instead of the install step. Is
> that correct? In that case, please mention it in the commit message.

yes, pragmatically it should be around for build and install,
even if its just used in one sport for now.

>
> >
> >  ifeq ($(BR2_PACKAGE_ZLIB),y)
> >  ZSTD_DEPENDENCIES += zlib
> > @@ -39,43 +35,52 @@ 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
> > -else
> > -ZSTD_BUILD_LIBS = libzstd.a libzstd
> > -ZSTD_INSTALL_LIBS = install-static install-shared
> > +ZSTD_BUILD_PROG_TARGET = zstd-release
> > +
> > +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +ZSTD_BUILD_LIBS += libzstd.a
> > +ZSTD_INSTALL_LIBS += install-static
> > +endif
> > +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +ZSTD_BUILD_LIBS += libzstd
> > +ZSTD_INSTALL_LIBS += install-shared
>
>  AFAICS this change doesn't actually change anything, it's just rewriting. You
> may claim the result is better, but it's really a matter of taste. We generally
> want to avoid making changes that are just "it looks nicer" if there's no real
> benefit (which can be, for example, a follow-up patch makes use of the new
> structure and would be more complicated in the old structure).
>
>  For sure, such a change should be in a separate patch that doesn't do any
> functional change at the same time.

Ack

>
> >  endif
> >
> > +# Since v1.5.0 the dynamic library is built for
> > +# multithreading, while the static library is not.
> > +#
> >  # 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_OPTS += HAVE_THREAD=1
> >  ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> > +else
> > +ZSTD_OPTS += HAVE_THREAD=0
> > +ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
> >  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)
>
>  There's no explanation about the -release suffix in the commit message.

the 'default' target is lib-release zstd-release. No big functional effect.

>
>  Since we already do override assignment to ZSTD_BUILD_LIBS, I think it would be
> nicer to put
>
> ZSTD_BUILD_LIBS := $(addsuffix -release,$(ZSTD_BUILD_LIBS))
>
> just after the threads condition above.

Ack
>
>
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             -C $(@D) zstd
> > +             -C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)
>
>  This change of building top-level to building in the programs subdirectory is
> also not clearly explained in the commit message (I mean *why* this is changed,
> not simply that it is changed).
>
> >  endef
> >
> >  define ZSTD_INSTALL_STAGING_CMDS
> > +     [ -e $(@D)/lib/libzstd.pc ]
>
>  This command serves no purpose.
>
>  I understand that it's a way for you to "assert" that the pc file is indeed
> built, but because you added it in the build commands we can be pretty sure that
> it's going to be there.
>
>  We normally don't have any assert like that in our install commands. It's also
> pretty pointless - you probably discovered that it was being built at
> installation time by looking at the build log, and that's the only way that
> you'd discover that *that* is the file that needs to be checked. If this ever
> breaks again, it's more than likely due to a different file.

its supposed to be a reminder to check if things break later, you easily
miss if some things are build during installation.
But ok.
>
>
> >       $(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)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
> > +             install-pc install-includes
>
>  Other than the PREFIX which is now part of ZSTD_OPTS, I believe nothing
> actually changed here, right? Please try to keep the diff small in such a case.

Ack

>
> >  endef
> >
> >  define ZSTD_INSTALL_TARGET_CMDS
> > +     [ -e $(@D)/programs/zstd ]
>
>  Again, not needed.
>
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
> > +             -C $(@D)/programs DESTDIR=$(TARGET_DIR) install
>
>  Again, spurious reordering of the options.
>
>
>  Regards,
>  Arnout
>
> >       $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> > -             DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
> > +             -C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
> >  endef
> >
> >  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
> >

Norbert
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

[Buildroot] [PATCH v4 2/3] 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>
---
v2->v3:
*   Remove ZSTD_OPTS_MOREFLAGS variable
---
 package/zstd/zstd.mk | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 5674e85bfe..58b0d0099e 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -13,6 +13,7 @@ ZSTD_CPE_ID_VENDOR = facebook
 ZSTD_CPE_ID_PRODUCT = zstandard
 
 ZSTD_OPTS += PREFIX=/usr
+ZSTD_OPTS += ZSTD_LEGACY_SUPPORT=0
 
 ifeq ($(BR2_PACKAGE_ZLIB),y)
 ZSTD_DEPENDENCIES += zlib
@@ -35,6 +36,9 @@ else
 ZSTD_OPTS += HAVE_LZ4=0
 endif
 
+# zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
+ZSTD_OPTS += MOREFLAGS="$(TARGET_OPTIMIZATION)"
+
 ZSTD_BUILD_PROG_TARGET = zstd-release
 
 ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
-- 
2.30.2

Re: [Buildroot] [PATCH v4 2/3] package/zstd: Change Build options

[Arnout Vandecappelle]

On 17/06/2021 11:14, 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>

 Applied to master, thanks. I just rewrote the commit message a little (in
particular, the subject line gave no indication that this patch is actually
fixing something).

 Regards,
 Arnout

> ---
> v2->v3:
> *   Remove ZSTD_OPTS_MOREFLAGS variable
> ---
>  package/zstd/zstd.mk | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 5674e85bfe..58b0d0099e 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -13,6 +13,7 @@ ZSTD_CPE_ID_VENDOR = facebook
>  ZSTD_CPE_ID_PRODUCT = zstandard
>  
>  ZSTD_OPTS += PREFIX=/usr
> +ZSTD_OPTS += ZSTD_LEGACY_SUPPORT=0
>  
>  ifeq ($(BR2_PACKAGE_ZLIB),y)
>  ZSTD_DEPENDENCIES += zlib
> @@ -35,6 +36,9 @@ else
>  ZSTD_OPTS += HAVE_LZ4=0
>  endif
>  
> +# zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
> +ZSTD_OPTS += MOREFLAGS="$(TARGET_OPTIMIZATION)"
> +
>  ZSTD_BUILD_PROG_TARGET = zstd-release
>  
>  ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> 
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

[Buildroot] [PATCH v4 3/3] package/zstd: Prefer dynamically linked tool

[Norbert Lange]

If the libzstd DSO is available, then link the
tool against it.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
v2->v3:
*   Drop config options, just use zstd-dll if possible
---
 package/zstd/zstd.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
index 58b0d0099e..2131f3fb16 100644
--- a/package/zstd/zstd.mk
+++ b/package/zstd/zstd.mk
@@ -39,7 +39,12 @@ endif
 # zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
 ZSTD_OPTS += MOREFLAGS="$(TARGET_OPTIMIZATION)"
 
+# prefer zstd-dll unless no library is available
+ifeq ($(BR2_STATIC_LIBS),y)
 ZSTD_BUILD_PROG_TARGET = zstd-release
+else
+ZSTD_BUILD_PROG_TARGET = zstd-dll
+endif
 
 ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
 ZSTD_BUILD_LIBS += libzstd.a
-- 
2.30.2

Re: [Buildroot] [PATCH v4 3/3] package/zstd: Prefer dynamically linked tool

[Arnout Vandecappelle]

On 17/06/2021 11:14, Norbert Lange wrote:
> If the libzstd DSO is available, then link the
> tool against it.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 However this depends on ZSTD_BUILD_PROG_TARGET which was added in the first
patch, so I haven't applied.


 Regards,
 Arnout


> ---
> v2->v3:
> *   Drop config options, just use zstd-dll if possible
> ---
>  package/zstd/zstd.mk | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 58b0d0099e..2131f3fb16 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -39,7 +39,12 @@ endif
>  # zstd will append -O3 after $(CFLAGS), use MOREFLAGS to override again
>  ZSTD_OPTS += MOREFLAGS="$(TARGET_OPTIMIZATION)"
>  
> +# prefer zstd-dll unless no library is available
> +ifeq ($(BR2_STATIC_LIBS),y)
>  ZSTD_BUILD_PROG_TARGET = zstd-release
> +else
> +ZSTD_BUILD_PROG_TARGET = zstd-dll
> +endif
>  
>  ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
>  ZSTD_BUILD_LIBS += libzstd.a
> 
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot