Re: [Buildroot] [PATCH v2 2/5] package/ulog: new package

[Herve Codina <herve.codina@bootlin.com>]

Hi Yann,

On Sat, 1 Jan 2022 17:05:33 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Hervé, All,
> 
> On 2021-11-12 14:12 +0100, Herve Codina spake thusly:
> > The ulog library is a minimalistic logging library derived from
> > Android logger.
> > 
> > https://github.com/Parrot-Developers/ulog
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> [--SNIP--]
> > diff --git a/package/ulog/ulog.mk b/package/ulog/ulog.mk
> > new file mode 100644
> > index 0000000000..fab16e80d8
> > --- /dev/null
> > +++ b/package/ulog/ulog.mk
> > @@ -0,0 +1,44 @@
> > +################################################################################
> > +#
> > +# ulog
> > +#
> > +################################################################################
> > +
> > +ULOG_VERSION = 0389d243352255f6182326dccdae3d56dadc078f
> > +ULOG_SITE = $(call github,Parrot-Developers,ulog,$(ULOG_VERSION))
> > +ULOG_LICENSE = Apache-2.0
> > +ULOG_LICENSE_FILES = COPYING
> > +ULOG_DEPENDENCIES = host-alchemy
> > +ULOG_INSTALL_STAGING = YES
> > +
> > +define ULOG_BUILD_CMDS
> > +	$(ALCHEMY_TARGET_ENV) \
> > +		$(ALCHEMY_MAKE) libulog
> > +endef
> > +
> > +define ULOG_INSTALL_STATIC_LIBS
> > +	$(INSTALL) -m 644 $(@D)/alchemy-out/staging/usr/lib/libulog.a $(1)/usr/lib/
> > +endef  
> 
> So, it looks like you are always going to install the static library,
> but the package does not have:  depends on !BR2_STATIC_LIBS

The v3 series will take care of BR2_STATIC_LIBS.

> 
> > +
> > +define ULOG_INSTALL_HEADERS
> > +	cp -Raf $(@D)/libulog/include/* $(1)/usr/include/
> > +endef
> > +
> > +ifeq ($(BR2_STATIC_LIBS),)
> > +define ULOG_INSTALL_SHARED_LIBS
> > +	$(INSTALL) -m 755 $(@D)/alchemy-out/staging/usr/lib/libulog.so* $(1)/usr/lib/  
> 
> Although it is very improbable that the destination directory does not
> already exist, that is still a possibility, especially in target/. And
> for consistency with all the other packages, you must create the
> destination directory first before you start copying multiple files in
> there.

Will be fixed in v3

> 
> Also for consistency, use either 'cp' or '$(INSTALL)', not both , in the
> same .mk (you used 'cp' for headers, and '$(INSTALL)' for libs, although
> I don't see a reason not to use them consistently).
> 
> Oh, and shared libraries do not need to be +x, so -m 644 is enough.

Ok, I will use '$(INSTALL) -m 644 ' in v3 series

> 
> > +endef
> > +endif
> > +
> > +define ULOG_INSTALL_TARGET_CMDS
> > +	$(call ULOG_INSTALL_SHARED_LIBS, $(TARGET_DIR))
> > +endef
> > +
> > +define ULOG_INSTALL_STAGING_CMDS
> > +	$(call ULOG_INSTALL_STATIC_LIBS, $(STAGING_DIR))
> > +	$(call ULOG_INSTALL_SHARED_LIBS, $(STAGING_DIR))
> > +	$(call ULOG_INSTALL_HEADERS, $(STAGING_DIR))
> > +	$(call ALCHEMY_INSTALL_LIB_SDK_FILE, ulog, libulog, libulog.so)  
> 
> So, what happens with BR2_STATIC_LIBS=y? The .so would not exist in that
> case, would it? But then, this is what would be registered in the
> atom.mk...

The file passed to Alchemy must be the .so.
Internally, the static name will be deduced by replacing the shared
suffix (.so) by the static suffix (.a)

> 
> And with BR2_STATIC_LIBS not set, then the .a should not be built (or at
> least not installed!), yet the installation of the static lib is
> unconditional (and see the first comment too).
> 
> It is difficult to understand if all of the static-shared combinations
> are supported, so could you please check/clarify this?
> 
> If the three combinations are indeed supported, then both the shared and
> the staic install macros should be conditional. Otherwise, proper guards
> should be added in the Config.in.

In the next version (v3) of this series, I will install .a and .so according
to flags set (BR2_{STATIC,SHARED}_LIBS).

I will have one exception: libshdata, libshdata-section-lookup.a.
This static lib is needed to use libshdata and cannot be a shared library.
It is forced as static in libshdata atom.mk file (include $(BUILD_STATIC_LIBRARY))

So, I choose to install it even if we supposed to be shared library only (ie
BR2_SHARED_LIBS=y)


> 
> Those comments apply to all the pakcages in this series.
> 

Thanks for the review,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot