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

[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 :-)

> 
>> 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