From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnout Vandecappelle Date: Mon, 14 Jun 2021 22:20:31 +0200 Subject: [Buildroot] [PATCH v2 5/5] package/zstd: Add option for cli binary In-Reply-To: References: <20210525172652.821351-1-nolange79@gmail.com> <20210525172652.821351-5-nolange79@gmail.com> Message-ID: <1c5020ee-d779-2c8d-e6ea-2bc6e7bd2d9f@mind.be> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net On 01/06/2021 11:20, Norbert Lange wrote: > Am Mo., 31. Mai 2021 um 17:02 Uhr schrieb Thomas De Schampheleire > : >> >> Hello, >> >> El mar, 25 may 2021 a las 19:28, Norbert Lange () 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 >>> --- >>> 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