Re: [Buildroot] [PATCH v4 1/3] package/zstd: rework build and install

[Arnout Vandecappelle <arnout@mind.be>]

 Hi Norbert,

 Sorry for not responding earlier - I apparently mis-filed this series and it
went off my backlog. It's only by looking at Fabrice's later patch that I
discovered this one again.


On 17/06/2021 11:14, Norbert Lange wrote:
> 1.5.0 uses Threads by default for cli tool and DSO,
> so override that in case the toolchain has none.

 It helps if you include a Fixes: tag in the commit message - that way it's
visible in patchwork that this patch fixes something and it's going to be
applied faster. Also the subject line suggests that the patch is just
refactoring, not fixing anything.


> make sure that everything is built in the build step,
> and fail the install if files are missing
> instead of lazily building them.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> 
> ---
> v3->v4:
> *   revert to previous scheme of BR2_TOOLCHAIN_HAS_THREADS
>     fixing build options for both static and shared libs.
> v2->v3:
> *   use normal = for assignment
> v1->v2:
> *   rebased against upstream/master
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/zstd/zstd.mk | 45 ++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk
> index 2a876376a2..5674e85bfe 100644
> --- a/package/zstd/zstd.mk
> +++ b/package/zstd/zstd.mk
> @@ -12,11 +12,7 @@ ZSTD_LICENSE_FILES = LICENSE COPYING
>  ZSTD_CPE_ID_VENDOR = facebook
>  ZSTD_CPE_ID_PRODUCT = zstandard
>  
> -ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> -ZSTD_OPTS += HAVE_THREAD=1
> -else
> -ZSTD_OPTS += HAVE_THREAD=0
> -endif
> +ZSTD_OPTS += PREFIX=/usr

 I think that this is not just refactoring, but is actually needed for the .pc
file which is now built during the build step instead of the install step. Is
that correct? In that case, please mention it in the commit message.

>  
>  ifeq ($(BR2_PACKAGE_ZLIB),y)
>  ZSTD_DEPENDENCIES += zlib
> @@ -39,43 +35,52 @@ 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
> -else
> -ZSTD_BUILD_LIBS = libzstd.a libzstd
> -ZSTD_INSTALL_LIBS = install-static install-shared
> +ZSTD_BUILD_PROG_TARGET = zstd-release
> +
> +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_BUILD_LIBS += libzstd.a
> +ZSTD_INSTALL_LIBS += install-static
> +endif
> +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +ZSTD_BUILD_LIBS += libzstd
> +ZSTD_INSTALL_LIBS += install-shared

 AFAICS this change doesn't actually change anything, it's just rewriting. You
may claim the result is better, but it's really a matter of taste. We generally
want to avoid making changes that are just "it looks nicer" if there's no real
benefit (which can be, for example, a follow-up patch makes use of the new
structure and would be more complicated in the old structure).

 For sure, such a change should be in a separate patch that doesn't do any
functional change at the same time.

>  endif
>  
> +# Since v1.5.0 the dynamic library is built for
> +# multithreading, while the static library is not.
> +#
>  # 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_TOOLCHAIN_HAS_THREADS),y)
> +ZSTD_OPTS += HAVE_THREAD=1
>  ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS))
> +else
> +ZSTD_OPTS += HAVE_THREAD=0
> +ZSTD_BUILD_LIBS := $(addsuffix -nomt,$(ZSTD_BUILD_LIBS))
>  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)

 There's no explanation about the -release suffix in the commit message.

 Since we already do override assignment to ZSTD_BUILD_LIBS, I think it would be
nicer to put

ZSTD_BUILD_LIBS := $(addsuffix -release,$(ZSTD_BUILD_LIBS))

just after the threads condition above.


>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		-C $(@D) zstd
> +		-C $(@D)/programs $(ZSTD_BUILD_PROG_TARGET)

 This change of building top-level to building in the programs subdirectory is
also not clearly explained in the commit message (I mean *why* this is changed,
not simply that it is changed).

>  endef
>  
>  define ZSTD_INSTALL_STAGING_CMDS
> +	[ -e $(@D)/lib/libzstd.pc ]

 This command serves no purpose.

 I understand that it's a way for you to "assert" that the pc file is indeed
built, but because you added it in the build commands we can be pretty sure that
it's going to be there.

 We normally don't have any assert like that in our install commands. It's also
pretty pointless - you probably discovered that it was being built at
installation time by looking at the build log, and that's the only way that
you'd discover that *that* is the file that needs to be checked. If this ever
breaks again, it's more than likely due to a different file.


>  	$(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)/lib DESTDIR=$(STAGING_DIR) $(ZSTD_INSTALL_LIBS) \
> +		install-pc install-includes

 Other than the PREFIX which is now part of ZSTD_OPTS, I believe nothing
actually changed here, right? Please try to keep the diff small in such a case.

>  endef
>  
>  define ZSTD_INSTALL_TARGET_CMDS
> +	[ -e $(@D)/programs/zstd ]

 Again, not needed.

>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/programs install
> +		-C $(@D)/programs DESTDIR=$(TARGET_DIR) install

 Again, spurious reordering of the options.


 Regards,
 Arnout

>  	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \
> -		DESTDIR=$(TARGET_DIR) PREFIX=/usr -C $(@D)/lib $(ZSTD_INSTALL_LIBS)
> +		-C $(@D)/lib DESTDIR=$(TARGET_DIR) $(ZSTD_INSTALL_LIBS)
>  endef
>  
>  HOST_ZSTD_OPTS += PREFIX=$(HOST_DIR)
> 
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot