diff mbox series

[v3] package/libzenoh-c: new package

Message ID AS1P250MB06080D04D548B5D4D947C44AA95E2@AS1P250MB0608.EURP250.PROD.OUTLOOK.COM
State Changes Requested
Headers show
Series [v3] package/libzenoh-c: new package | expand

Commit Message

Michel Alex March 1, 2024, 9:56 a.m. UTC 
This package provides a C binding based on the main
Zenoh implementation written in Rust.

https://github.com/eclipse-zenoh/zenoh-c

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

--
2.34.1

Comments

Yann E. MORIN March 1, 2024, 5:49 p.m. UTC | #1 
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
Michel Alex March 6, 2024, 1:19 p.m. UTC | #2 
Yann,

thanks for your comments. See my answer below:

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

Sorry but I am not allowed to use git send-email command, I have to use Outlook instead.
Perhaps outlook auto-corrects all indentation to whitespaces, I don't know. I will provide
the next version of my patch as attachment to this email.

> 
>     $(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?

Fixed, use now BR2_ENABLE_DEBUG  and RUSTC_TARGET_NAME variables.

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

Fixed.

See attachment, thanks.

Regards,
Alex
Michel Alex April 16, 2024, 10:03 a.m. UTC | #3 
Hello,

I'm not sure if you received my last email with new version of my patch as attachment. Below you can find it as plain text. Sorry for that, but I have to use outlook as email client and not allowed to use git send mail. Please let me know if any further changes are necessary.

Here is the patch:

This package provides a C binding based on the main
Zenoh implementation written in Rust.
Because this lib does not provide any binaries or examples,
and the cargo infra does not provide any possibility to
disable the --bins option in cargo install step, we
have to override the INSTALL_STAGING_CMDS and the
INSTALL_TARGET_CMDS macros to prevent failing of the
buildroot installation step.

https://github.com/eclipse-zenoh/zenoh-c

Signed-off-by: Alex Michel <alex.michel@wiedemann-group.com>

---
Changes v3 -> v4:
  - wrapped long lines to 80 characters
  - use BR2_ENABLE_DEBUG variable instead of "release" path component
  - use RUSTC_TARGET_NAME variable instead of "*" path component
  - shared macro installs only the common set of files
  - INSTALL_TARGET_CMDS installs only the lib
  - INSTALL_STAGING_CMDS installs both the lib and include files

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
---
 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   | 35 ++++++++++++++++++++++++++++++
 5 files changed, 49 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/
 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"
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
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..0ad81f3e41
--- /dev/null
+++ b/package/libzenoh-c/libzenoh-c.mk
@@ -0,0 +1,35 @@
+################################################################################
+#
+# 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
+
+ifeq ($(BR2_ENABLE_DEBUG),y)
+LIBZENOH_C_LIB_LOCATION = $(@D)/target/$(RUSTC_TARGET_NAME)/debug
+else
+LIBZENOH_C_LIB_LOCATION = $(@D)/target/$(RUSTC_TARGET_NAME)/release
+endif
+
+define LIBZENOH_C_INSTALL_FILES
+	$(INSTALL) -D -m 644 \
+		$(LIBZENOH_C_LIB_LOCATION)/libzenohc.so \
+		$(1)/usr/lib/libzenohc.so
+endef
+
+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))
+	mkdir -p $(STAGING_DIR)/usr/include/
+	cp -dpfr $(@D)/include/* $(STAGING_DIR)/usr/include/
+endef
+
+$(eval $(cargo-package))
diff mbox series

Patch

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/
 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"
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
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
+       mkdir -p $(STAGING_DIR)/usr/include/
+       cp -dpfr $(@D)/include/* $(STAGING_DIR)/usr/include/
+endef
+
+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))