Re: [Buildroot] [PATCH v3] package/libzenoh-c: new package

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

Michel, All,

Thanks for this new iteration. Howeve, I have some comments and
questions, see bewlow...

On 2024-03-01 09:56 +0000, Michel Alex spake thusly:
> This package provides a C binding based on the main
> Zenoh implementation written in Rust.
> 
> https://github.com/eclipse-zenoh/zenoh-c

Having _just_ the description of the package in the commit log is not
that interesting, especially as it is just a copy of the description
in the help text.

The commit log is there to explain the change; it is meant to help
reviewers (and maintainers) assess the quality of the change.

In htis case, what would have been interesting to have in the commit
log, is the explanations on why you had to override the install
commands, rather than use thje default ones provided by the
cargo-package infra, that are supposed to cover the vast majority of
cases; diverging from that should be explained.

See other comments, below...

> Signed-off-by: Alex Michel <alex.michel@wiedemann-group.com>
> ---
> Changes v2 -> v3:
>   - bump package to 0.10.1-rc
>   - set INSTALL_STAGING
> 
> Changes v1 -> v2:
>   - renamed zenoh-c to libzenoh-c
>   - added myself to DEVELOPERS
>   - fixed LICENSE
>   - install shared libraries to staging and to target

Thanks for the changelog, that's very good and much appreciated! 👍

> ---
>  DEVELOPERS                         |  1 +
>  package/Config.in                  |  1 +
>  package/libzenoh-c/Config.in       |  9 +++++++++
>  package/libzenoh-c/libzenoh-c.hash |  3 +++
>  package/libzenoh-c/libzenoh-c.mk   | 27 +++++++++++++++++++++++++++
>  5 files changed, 41 insertions(+)
>  create mode 100644 package/libzenoh-c/Config.in
>  create mode 100644 package/libzenoh-c/libzenoh-c.hash
>  create mode 100644 package/libzenoh-c/libzenoh-c.mk
> 
> diff --git a/DEVELOPERS b/DEVELOPERS
> index ac277423a1..08c3d9a5a1 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -75,6 +75,7 @@ N:      Alessandro Partesotti <a.partesotti@gmail.com>
>  F:      package/oatpp/
> 
>  N:     Alex Michel <alex.michel@wiedemann-group.com>
> +F:     package/libzenoh-c/

Normally, this file is indented with TABs, but your mail only contains
spaces. Not sure how you are sending it, but using git send-email
ensures it is properly sent.

Of course, that means the patch does not apply...

>  F:     package/libzenoh-pico/
>  F:     package/network-manager-openvpn/
> 
> diff --git a/package/Config.in b/package/Config.in
> index cd687a682b..af1ee30585 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1996,6 +1996,7 @@ menu "Networking"
>         source "package/libwebsock/Config.in"
>         source "package/libwebsockets/Config.in"
>         source "package/libyang/Config.in"
> +       source "package/libzenoh-c/Config.in"
>         source "package/libzenoh-pico/Config.in"
>         source "package/lksctp-tools/Config.in"
>         source "package/mbuffer/Config.in"

Ditto, this file is TAB-indented.

> diff --git a/package/libzenoh-c/Config.in b/package/libzenoh-c/Config.in
> new file mode 100644
> index 0000000000..d22807c047
> --- /dev/null
> +++ b/package/libzenoh-c/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_LIBZENOH_C
> +       bool "libzenoh-c"
> +       depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
> +       select BR2_PACKAGE_HOST_RUSTC
> +       help
> +         This package provides a C binding based on the main
> +         Zenoh implementation written in Rust.
> +
> +         https://github.com/eclipse-zenoh/zenoh-c

Leading TABs are missing here as well... :-/

> diff --git a/package/libzenoh-c/libzenoh-c.hash b/package/libzenoh-c/libzenoh-c.hash
> new file mode 100644
> index 0000000000..8c93a7a091
> --- /dev/null
> +++ b/package/libzenoh-c/libzenoh-c.hash
> @@ -0,0 +1,3 @@
> +# Locally computed
> +sha256  3ede587dd08ccd6b0b7f0b44faeefa466eb5e18826db0b1cd93c51ffc59377ec  libzenoh-c-0.10.1-rc.tar.gz
> +sha256  01a44774f7b1a453595c7c6d7f7308284ba6a1059dc49e14dad6647e1d44a338  LICENSE
> diff --git a/package/libzenoh-c/libzenoh-c.mk b/package/libzenoh-c/libzenoh-c.mk
> new file mode 100644
> index 0000000000..738758e13f
> --- /dev/null
> +++ b/package/libzenoh-c/libzenoh-c.mk
> @@ -0,0 +1,27 @@
> +################################################################################
> +#
> +# libzenoh-c
> +#
> +################################################################################
> +
> +LIBZENOH_C_VERSION = 0.10.1-rc
> +LIBZENOH_C_SITE = $(call github,eclipse-zenoh,zenoh-c,$(LIBZENOH_C_VERSION))
> +LIBZENOH_C_LICENSE = Apache-2.0 or EPL-2.0
> +LIBZENOH_C_LICENSE_FILES = LICENSE
> +LIBZENOH_C_INSTALL_STAGING = YES
> +
> +define LIBZENOH_C_INSTALL_FILES
> +       $(INSTALL) -D -m 644 $(@D)/target/*/release/libzenohc.so $(1)/usr/lib/libzenohc.so

Please wrap the long lines so they are below the 80-ish char length:

    $(INSTALL) -D -m 644 \
        $(@D)/target/*/release/libzenohc.so \
        $(1)/usr/lib/libzenohc.so

Also, I think the 'release' path component will change when
BR2_ENABLE_DEBUG=y, as we do not pass --release in that case.

Aslo, why do we need a '*' path component? Since the destination is a
single file, we do only expect ne inout file, so the '*' is expected to
match a single directory, which we should have a way to know. Can you
explain that as well, please?

> +       mkdir -p $(STAGING_DIR)/usr/include/
> +       cp -dpfr $(@D)/include/* $(STAGING_DIR)/usr/include/
> +endef

This macro is expanded in the INSTALL_TARGET case, which means files
will be installed to staging during the target install. That does not
look right.

So, assuming those overrides are needed (as will be explained in the
commit log ;-)), the shared macro should only be concerned about
installing the common set of files, i.e. the .so files.

The files only installed in staging should be installed with
_INSTALL_STAGING_CMDS.

Regards,
Yann E. MORIN.

> +define LIBZENOH_C_INSTALL_TARGET_CMDS
> +       $(call LIBZENOH_C_INSTALL_FILES,$(TARGET_DIR))
> +endef
> +
> +define LIBZENOH_C_INSTALL_STAGING_CMDS
> +       $(call LIBZENOH_C_INSTALL_FILES,$(STAGING_DIR))
> +endef
> +
> +$(eval $(cargo-package))
> --
> 2.34.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