* [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 @ 2021-05-25 17:26 Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build Norbert Lange ` (4 more replies) 0 siblings, 5 replies; 17+ messages in thread From: Norbert Lange @ 2021-05-25 17:26 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange @ 2021-05-25 17:26 ` Norbert Lange 2021-06-14 20:20 ` Arnout Vandecappelle 2021-05-25 17:26 ` [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install Norbert Lange ` (3 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-05-25 17:26 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build 2021-05-25 17:26 ` [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build Norbert Lange @ 2021-06-14 20:20 ` Arnout Vandecappelle 0 siblings, 0 replies; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-14 20:20 UTC (permalink / raw) To: buildroot 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)) > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build Norbert Lange @ 2021-05-25 17:26 ` Norbert Lange 2021-06-14 19:50 ` Arnout Vandecappelle 2021-05-25 17:26 ` [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options Norbert Lange ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-05-25 17:26 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install 2021-05-25 17:26 ` [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install Norbert Lange @ 2021-06-14 19:50 ` Arnout Vandecappelle 2021-06-14 21:04 ` Norbert Lange 0 siblings, 1 reply; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-14 19:50 UTC (permalink / raw) To: buildroot 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) > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install 2021-06-14 19:50 ` Arnout Vandecappelle @ 2021-06-14 21:04 ` Norbert Lange 2021-06-15 20:31 ` Arnout Vandecappelle 0 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-06-14 21:04 UTC (permalink / raw) To: buildroot 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) > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install 2021-06-14 21:04 ` Norbert Lange @ 2021-06-15 20:31 ` Arnout Vandecappelle 0 siblings, 0 replies; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-15 20:31 UTC (permalink / raw) To: buildroot 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) >>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install Norbert Lange @ 2021-05-25 17:26 ` Norbert Lange 2021-06-14 19:56 ` Arnout Vandecappelle 2021-05-25 17:26 ` [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary Norbert Lange 2021-06-14 20:20 ` [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Arnout Vandecappelle 4 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-05-25 17:26 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options 2021-05-25 17:26 ` [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options Norbert Lange @ 2021-06-14 19:56 ` Arnout Vandecappelle 2021-06-14 21:19 ` Norbert Lange 0 siblings, 1 reply; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-14 19:56 UTC (permalink / raw) To: buildroot 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options 2021-06-14 19:56 ` Arnout Vandecappelle @ 2021-06-14 21:19 ` Norbert Lange 0 siblings, 0 replies; 17+ messages in thread From: Norbert Lange @ 2021-06-14 21:19 UTC (permalink / raw) To: buildroot 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 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange ` (2 preceding siblings ...) 2021-05-25 17:26 ` [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options Norbert Lange @ 2021-05-25 17:26 ` Norbert Lange 2021-05-31 15:02 ` Thomas De Schampheleire 2021-06-14 20:20 ` [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Arnout Vandecappelle 4 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-05-25 17:26 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-05-25 17:26 ` [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary Norbert Lange @ 2021-05-31 15:02 ` Thomas De Schampheleire 2021-06-01 9:20 ` Norbert Lange 0 siblings, 1 reply; 17+ messages in thread From: Thomas De Schampheleire @ 2021-05-31 15:02 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-05-31 15:02 ` Thomas De Schampheleire @ 2021-06-01 9:20 ` Norbert Lange 2021-06-14 20:20 ` Arnout Vandecappelle 0 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-06-01 9:20 UTC (permalink / raw) To: buildroot 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-06-01 9:20 ` Norbert Lange @ 2021-06-14 20:20 ` Arnout Vandecappelle 2021-06-14 21:47 ` Norbert Lange 0 siblings, 1 reply; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-14 20:20 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-06-14 20:20 ` Arnout Vandecappelle @ 2021-06-14 21:47 ` Norbert Lange 2021-06-15 21:15 ` Arnout Vandecappelle 0 siblings, 1 reply; 17+ messages in thread From: Norbert Lange @ 2021-06-14 21:47 UTC (permalink / raw) To: buildroot 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary 2021-06-14 21:47 ` Norbert Lange @ 2021-06-15 21:15 ` Arnout Vandecappelle 0 siblings, 0 replies; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-15 21:15 UTC (permalink / raw) To: buildroot 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 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange ` (3 preceding siblings ...) 2021-05-25 17:26 ` [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary Norbert Lange @ 2021-06-14 20:20 ` Arnout Vandecappelle 4 siblings, 0 replies; 17+ messages in thread From: Arnout Vandecappelle @ 2021-06-14 20:20 UTC (permalink / raw) To: buildroot 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-06-15 21:15 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-25 17:26 [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 2/5] package/zstd: Simplify host-build Norbert Lange 2021-06-14 20:20 ` Arnout Vandecappelle 2021-05-25 17:26 ` [Buildroot] [PATCH v2 3/5] package/zstd: rework build and install Norbert Lange 2021-06-14 19:50 ` Arnout Vandecappelle 2021-06-14 21:04 ` Norbert Lange 2021-06-15 20:31 ` Arnout Vandecappelle 2021-05-25 17:26 ` [Buildroot] [PATCH v2 4/5] package/zstd: Change Build options Norbert Lange 2021-06-14 19:56 ` Arnout Vandecappelle 2021-06-14 21:19 ` Norbert Lange 2021-05-25 17:26 ` [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary Norbert Lange 2021-05-31 15:02 ` Thomas De Schampheleire 2021-06-01 9:20 ` Norbert Lange 2021-06-14 20:20 ` Arnout Vandecappelle 2021-06-14 21:47 ` Norbert Lange 2021-06-15 21:15 ` Arnout Vandecappelle 2021-06-14 20:20 ` [Buildroot] [PATCH v2 1/5] package/zstd: bump to version 1.5.0 Arnout Vandecappelle
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.