| Message ID | 20210525172652.821351-5-nolange79@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] package/zstd: bump to version 1.5.0 | expand |
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@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
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@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)
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@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
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@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 >
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 >>
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
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(-)