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

[Norbert Lange <nolange79@gmail.com>]

Am Mi., 4. Aug. 2021 um 15:01 Uhr schrieb 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.

It was a fix for a part of the series, so nothing to fix
unless only a part gets applied.
>
>
> > 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.

yes, pragmatically it should be around for build and install,
even if its just used in one sport for now.

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

Ack

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

the 'default' target is lib-release zstd-release. No big functional effect.

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

Ack
>
>
> >       $(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.

its supposed to be a reminder to check if things break later, you easily
miss if some things are build during installation.
But ok.
>
>
> >       $(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.

Ack

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

Norbert
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot