Re: [Buildroot] [PATCH] package/ledmon: new package

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

Hello Maksim,

First of all, thanks a lot for your patch, and sorry for the very long
delay to provide feedback. We are currently doing an effort of handling
our backlog of patches, and therefore reviewing/merging old patches.

Your patch is almost ready to merge with some tweaks, but one of them
needs your support. See below.

On Mon, 27 Mar 2023 19:54:23 +0300
Maksim Kiselev <bigunclemax@gmail.com> wrote:

> Enclosure LED Utilities
> 
> ledmon and ledctl are userspace tools designed to control storage
> enclosure LEDs. The user must have root privileges to use these tools.
> 
> These tools use the SGPIO and SES-2 protocols to monitor and control LEDs.
> They been verified to work with Intel(R) storage controllers (i.e. the
> Intel(R) AHCI controller) and have not been tested with storage controllers of
> other vendors (especially SAS/SCSI controllers).
> 
> For backplane enclosures attached to ISCI controllers, support is limited to
> Intel(R) Intelligent Backplanes.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---
>  package/Config.in                             |  1 +
>  .../ledmon/0001-Add-disable-doc-option.patch  | 57 +++++++++++++++++++
>  package/ledmon/Config.in                      | 11 ++++
>  package/ledmon/ledmon.hash                    |  3 +
>  package/ledmon/ledmon.mk                      | 14 +++++
>  5 files changed, 86 insertions(+)
>  create mode 100644 package/ledmon/0001-Add-disable-doc-option.patch
>  create mode 100644 package/ledmon/Config.in
>  create mode 100644 package/ledmon/ledmon.hash
>  create mode 100644 package/ledmon/ledmon.mk

We need you to add an entry in the DEVELOPERS file for this package, so
that you're notified by e-mail when:

- There are build failures affecting your package
- A new version becomes available upstream and the package needs to be updated
- A CVE is reported on your package and needs to be fixed

> diff --git a/package/ledmon/0001-Add-disable-doc-option.patch b/package/ledmon/0001-Add-disable-doc-option.patch
> new file mode 100644
> index 0000000000..4803c37698
> --- /dev/null
> +++ b/package/ledmon/0001-Add-disable-doc-option.patch
> @@ -0,0 +1,57 @@
> +From 4c356662faaa5aa2dc0b0eb713dc5134a70eb3b0 Mon Sep 17 00:00:00 2001
> +From: Maksim Kiselev <bigunclemax@gmail.com>
> +Date: Mon, 27 Mar 2023 17:45:15 +0300
> +Subject: [PATCH] Add '--disable-doc' option
> +
> +Introduce a configure option to disable documentation installation in case if it is not required.
> +
> +Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>

We need all patches to have an Upstream: tag that indicates where the
patch has been submitted upstream. This project has a Github repo, so
it should be trivial to submit this patch through a pull request, and
indicate the URL of that pull request as the value of the Upstream: tag.

> diff --git a/package/ledmon/Config.in b/package/ledmon/Config.in
> new file mode 100644
> index 0000000000..467a75fa56
> --- /dev/null
> +++ b/package/ledmon/Config.in
> @@ -0,0 +1,11 @@
> +config BR2_PACKAGE_LEDMON
> +	bool "ledmon"
> +	depends on BR2_PACKAGE_PCIUTILS
> +	depends on BR2_PACKAGE_SG3_UTILS
> +	depends on BR2_PACKAGE_HAS_UDEV

For pciutils and sg3_utils, we definitely want to use "select" instead
of "depends on".

So, we need:

	depends on BR2_PACKAGE_HAS_UDEV
	depends on BR2_TOOLCHAIN_HAS_THREADS # sg3_utils
	select BR2_PACKAGE_PCIUTILS
	select BR2_PACKAGE_SG3_UTILS

> +	help
> +	  Enclosure LED Utilities. The ledmon application is
> +	  a daemon process used to monitor a state of software
> +	  RAID devices (md only) or a state of block devices.
> +
> +	  https://github.com/intel/ledmon

And then:

comment "ledmon needs udev and a toolchain w/ threads"
	depends on !BR2_PACKAGE_UDEV || !BR2_TOOLCHAIN_HAS_THREADS

Another aspect: did you try to build ledmon with different toolchains
in Buildroot? In particular I see a pull request at
https://github.com/intel/ledmon/pull/139 that fixes build issues with
the Musl C library, so I'm wondering if it builds with musl. I can
suggest you to do a run of ./utils/test-pkg for ledmon to verify that
it builds with a set of toolchains.

> diff --git a/package/ledmon/ledmon.mk b/package/ledmon/ledmon.mk
> new file mode 100644
> index 0000000000..dba8a99b8c
> --- /dev/null
> +++ b/package/ledmon/ledmon.mk
> @@ -0,0 +1,14 @@
> +################################################################################
> +#
> +# ledmon
> +#
> +################################################################################
> +
> +LEDMON_VERSION = v0.96
> +LEDMON_SITE = $(call github,intel,ledmon,$(LEDMON_VERSION))

Please use:

LEDMON_VERSION = 0.96
LEDMON_SITE = $(call github,intel,ledmon,v$(LEDMON_VERSION))

So that the LEDMON_VERSION variable only includes the version, without
the "v" prefix. This will require a small change in the .hash file, as
the tarball name will change.

> +LEDMON_DEPENDENCIES = pciutils sg3_utils udev

You need to add host-pkgconf to this list, as the configure script uses
pkg-config.

> +LEDMON_LICENSE = GPL-2.0
> +LEDMON_LICENSE_FILES = COPYING

Perhaps we can add a comment here that says:

# The code base also include a COPYING.LIB file with the LGPL-2.1 text,
# and some source files are published under LGPL-2.1, but all of them are
# at some point linked with GPL-2.0 code, making the resulting binaries
# GPL-2.0 licensed

(hoping that this statement is correct of course)

Could you rework those different details and send an updated version of
your patch? It would be very useful!

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