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

["Yann E. MORIN" <yann.morin.1998@free.fr>]

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

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

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.

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

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.

Those comments apply to all the pakcages in this series.

Regards,
Yann E. MORIN.

> +endef
> +
> +$(eval $(generic-package))
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot