Re: [Buildroot] [PATCH 1/1] package/qbee-agent: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Jon,

Thanks a lot for this contribution. See below a small number of
comments/questions.

On Thu,  9 Nov 2023 09:09:19 +0100
Jon Henrik Bjørnstad via buildroot <buildroot@buildroot.org> wrote:

> This patch add qbee-agent, an open source device management software
> for Linux devices.
> 
> Signed-off-by: Jon Henrik Bjørnstad <jonhenrik@qbee.io>
> ---
>  package/Config.in                  |  1 +
>  package/qbee-agent/Config.in       | 16 +++++++++++
>  package/qbee-agent/qbee-agent.hash |  3 ++
>  package/qbee-agent/qbee-agent.mk   | 45 ++++++++++++++++++++++++++++++
>  4 files changed, 65 insertions(+)

Could you add an entry in the DEVELOPERS file for this package, that
associates your name/e-mail with the package, so that we can notify you
when there are build failures, or needed security updates?

> diff --git a/package/Config.in b/package/Config.in
> index ce46d30fed..1fef365c35 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -2741,6 +2741,7 @@ menu "System tools"
>  	source "package/procs/Config.in"
>  	source "package/psmisc/Config.in"
>  	source "package/pwgen/Config.in"
> +  source "package/qbee-agent/Config.in"

The indentation is not correct here.

>  	source "package/quota/Config.in"
>  	source "package/quotatool/Config.in"
>  	source "package/rauc/Config.in"
> diff --git a/package/qbee-agent/Config.in b/package/qbee-agent/Config.in
> new file mode 100644
> index 0000000000..9cc7f5c88f
> --- /dev/null
> +++ b/package/qbee-agent/Config.in
> @@ -0,0 +1,16 @@
> +config BR2_PACKAGE_QBEE_AGENT
> +	bool "qbee-agent"
> +	select BR2_PACKAGE_OPENSSH if !BR2_PACKAGE_DROPBEAR_CLIENT  # runtime
> +	select BR2_PACKAGE_IPTABLES  # runtime
> +	select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS  # runtime
> +	select BR2_PACKAGE_SHADOW  # runtime
> +	select BR2_PACKAGE_BASH  # runtime

Minor nit: only one space before the "# runtime" comment.

> +	help
> +	  Qbee is a device management platform that comprise
> +	  of an open-source agent and a hosted SaaS backend.
> +	  This config installs pre-built static binaries made
> +	  with the official public signing key for qbee.io. The
> +	  binaries will work seamlessly with the qbee.io device
> +	  management backend.

Please put one empty line betwen the help text and the upstream URL.

But the bigger question is why do we package a pre-built static binary,
rather than building from source?

Also, if this package is provided with pre-built binaries, then most
likely it is only supported for a specific subset of CPU architectures,
so you need some "depends on". If you however decide to build if from
source, it should have:

	depends on BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS

> +	  https://github.com/qbee-io/qbee-agent
> +	  https://qbee.io
> diff --git a/package/qbee-agent/qbee-agent.hash b/package/qbee-agent/qbee-agent.hash
> new file mode 100644
> index 0000000000..e3a81298e0
> --- /dev/null
> +++ b/package/qbee-agent/qbee-agent.hash
> @@ -0,0 +1,3 @@

Please add a comment that says where the hash comes from. If it's just
calculated locally by you, then:

# Locally calculated

> +md5  6e4dc7323b99d8c5a981b7b2c5da4f66  qbee-agent-2023.44.tar.gz
> +sha1  a0d02c1180fd97228f23859a8dea91fe81a31626  qbee-agent-2023.44.tar.gz
> +sha256  edcf319c4ce17e9844df598fa796aa87303a1bf238299dbeeb41c94ff5de5e1d  qbee-agent-2023.44.tar.gz

The sha256 hash is sufficient.

> diff --git a/package/qbee-agent/qbee-agent.mk b/package/qbee-agent/qbee-agent.mk
> new file mode 100644
> index 0000000000..4e261d6bf4
> --- /dev/null
> +++ b/package/qbee-agent/qbee-agent.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# qbee-agent package
> +#
> +################################################################################
> +
> +QBEE_AGENT_VERSION = 2023.44
> +#QBEE_AGENT_SOURCE = qbee-agent-$(QBEE_AGENT_VERSION).tar.gz
> +QBEE_AGENT_SITE = https://cdn.qbee.io/software/qbee-agent/$(QBEE_AGENT_VERSION)/binaries
> +QBEE_AGENT_LICENSE = Apache-2.0

Is there are license file?

> +
> +ifeq ($(BR2_arm),y)
> +QBEE_AGENT_GOARCH = arm
> +else ifeq ($(BR2_aarch64),y)
> +QBEE_AGENT_GOARCH = arm64
> +else ifeq ($(BR2_i386),y)
> +QBEE_AGENT_GOARCH = 386
> +else ifeq ($(BR2_x86_64),y)
> +QBEE_AGENT_GOARCH = amd64
> +endif

Perhaps you can directly use $(GO_GOARCH) ?

> +define QBEE_AGENT_BUILD_CMDS
> +endef

Just don't define build commands in this case.

> +define QBEE_AGENT_INSTALL_TARGET_CMDS
> +	$(INSTALL) -m 0755 $(@D)/qbee-agent-$(QBEE_AGENT_VERSION)/qbee-agent-$(QBEE_AGENT_GOARCH) $(TARGET_DIR)/usr/bin/qbee-agent

It's a bit surprising, why do you have a
qbee-agent-$(QBEE_AGENT_VERSION) ? It looks like your tarball has one
too many path embedded maybe? If so, you can do:

QBEE_AGENT_STRIP_COMPONENTS = 2

this will simplify the paths.

> +	$(INSTALL) -d -m 0700 $(TARGET_DIR)/etc/qbee/ppkeys

No need to do this...

> +	$(INSTALL) -m 0600 $(@D)/qbee-agent-$(QBEE_AGENT_VERSION)/share/ssl/ca.cert $(TARGET_DIR)/etc/qbee/ppkeys/ca.cert

... just add -D as an option here, which will create all needed
destination directories.

> +endef
> +
> +define QBEE_AGENT_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -D -m 0644 $(@D)/qbee-agent-$(QBEE_AGENT_VERSION)/init-scripts/systemd/qbee-agent.service \
> +	  $(TARGET_DIR)/usr/lib/systemd/system/qbee-agent.service

Tab for the indentation.

> +endef
> +
> +define QBEE_AGENT_INSTALL_INIT_SYSV
> +	$(INSTALL) -D -m 755 $(@D)/qbee-agent-$(QBEE_AGENT_VERSION)/init-scripts/sysvinit/qbee-agent \
> +	  $(TARGET_DIR)/etc/init.d/S99qbee-agent

Likewise.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot