[Buildroot] [PATCH v2 3/5] package/zstd: rework build and install

Arnout Vandecappelle arnout at mind.be
Tue Jun 15 20:31:33 UTC 2021



On 14/06/2021 23:04, Norbert Lange wrote:
> Am Mo., 14. Juni 2021 um 21:50 Uhr schrieb Arnout Vandecappelle
> <arnout at 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 at gmail.com>
>>>
>>> ---
>>> v1->v2:
>>> *   rebased against upstream/master
>>>
>>> Signed-off-by: Norbert Lange <nolange79 at 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)
>>>



More information about the buildroot mailing list