Re: [Buildroot] [PATCH v3 1/2] package/rasdaemon: Add rasdaemon package

[Julien Olivain via buildroot <buildroot@buildroot.org>]

Hi Bastien,

Thanks for the patch.  I have few comments. See below.

On 01/12/2025 15:08, Bastien Curutchet via buildroot wrote:
> Rasdaemon is a tool that aims to replace the edac-tool and provide a 
> way
> to collect all hardware error events reported by the Linux kernel in a
> common framework.
> 
> Add a new package to support rasdaemon in the 'Hardware handling'
> section. It depends on libtraceevents to detect the ftrace events
> generated by the kernel and on sqlite to store these events.
> 
> Support for the PCIe AER events is optionnal and implies a dependency 
> on
> pciutils so also add a dedicated 'sub-option' to enable it.
> 
> Add a SYSV init script to start / stop the daemon
> 
> Add myself to the DEVELOPERS file.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
[...]
> diff --git a/package/rasdaemon/Config.in b/package/rasdaemon/Config.in
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..68cef3a313bf2b82a8306ed5e0ec581cd618531d
> --- /dev/null
> +++ b/package/rasdaemon/Config.in
> @@ -0,0 +1,25 @@
> +config BR2_PACKAGE_RASDAEMON
> +	bool "rasdaemon"
> +	depends on !BR2_STATIC_LIBS # libtraceevent

rasdaemon seems to use unconditionally threads. See:
https://github.com/mchehab/rasdaemon/blob/v0.8.4/ras-events.h#L10

So you probably miss here a "depends on BR2_TOOLCHAIN_HAS_THREADS".
The Config.in comment also needs to be updated at the end of the file.

> +	select BR2_PACKAGE_LIBTRACEEVENT
> +	select BR2_PACKAGE_SQLITE

SQLite is an optional dependency. See:
https://github.com/mchehab/rasdaemon/blob/v0.8.4/configure.ac#L28

So you don't need to select BR2_PACKAGE_SQLITE here, and you could
preferably have in the .mk file something like:

ifeq ($(BR2_PACKAGE_SQLITE),y)
RASDAEMON_CONF_OPTS += --enable-sqlite3
RASDAEMON_DEPENDENCIES += sqlite
else
RASDAEMON_CONF_OPTS += --disable-sqlite3
endif

> +	select BR2_PACKAGE_ARGP_STANDALONE if BR2_TOOLCHAIN_USES_UCLIBC || 
> BR2_TOOLCHAIN_USES_MUSL
> +	help
> +	  rasdaemon is a tool that collects all hardware error events
> +	  reported by the Linux Kernel from several sources (EDAC, MCE
> +	  PCI, ...) into one common framework.

Could format the help string like other Buildroot packages,
please?

You should have a blank line, followed by just the homepage url:

https://github.com/mchehab/rasdaemon

> +	  See https://github.com/mchehab/rasdaemon for details.
> +
> +if BR2_PACKAGE_RASDAEMON
> +
> +config BR2_PACKAGE_RASDAEMON_AER
> +	bool "Enable AER support"
> +	select BR2_PACKAGE_PCIUTILS
> +	help
> +	  Enable handling of AER (Advanced Error Reporting) events on
> +	  PCIe
> +
> +endif
> +
> +comment "rasdaemon needs a toolchain w/ dynamic library"

The comment should become:
comment "rasdaemon needs a toolchain w/ threads, dynamic library"

> +	depends on BR2_STATIC_LIBS

Here the condition should become:
depends on !BR2_TOOLCHAIN_HAS_THREADS || BR2_STATIC_LIBS

> diff --git a/package/rasdaemon/rasdaemon.mk 
> b/package/rasdaemon/rasdaemon.mk
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..50ac97a37dcbe63443bddca505ebc7c929116e53
> --- /dev/null
> +++ b/package/rasdaemon/rasdaemon.mk
> @@ -0,0 +1,33 @@
> +################################################################################
> +#
> +# rasdaemon
> +#
> +################################################################################
> +
> +RASDAEMON_VERSION = 0.8.4
> +RASDAEMON_SITE = $(call 
> github,mchehab,rasdaemon,v$(RASDAEMON_VERSION))
> +RASDAEMON_LICENSE = GPL-2.0+
> +RASDAEMON_LICENSE_FILES = COPYING
> +RASDAEMON_AUTORECONF = YES
> +
> +RASDAEMON_DEPENDENCIES = libtraceevent sqlite

The "sqlite" dependency could be removed and added in a conditional
block, as I suggested.

> +# rasdaemon uses argp.h which is not provided by uclibc or musl by 
> default.
> +# Use the argp-standalone package to provide this.
> +ifeq ($(BR2_PACKAGE_ARGP_STANDALONE),y)
> +RASDAEMON_DEPENDENCIES += argp-standalone
> +RASDAEMON_CONF_ENV += LIBS="-largp"
> +endif
> +
> +ifeq ($(BR2_PACKAGE_RASDAEMON_AER),y)
> +RASDAEMON_DEPENDENCIES += pciutils
> +RASDAEMON_CONF_OPTS += --enable-aer
> +else
> +RASDAEMON_CONF_OPTS += --disable-aer
> +endif
> +
> +define RASDAEMON_INSTALL_INIT_SYSV
> +	$(INSTALL) -m 0755 -D package/rasdaemon/S95rasdaemon \
> +		$(TARGET_DIR)/etc/init.d/S95rasdaemon
> +endef
> +
> +$(eval $(autotools-package))
> 
> --
> 2.51.1

Could you send an updated patch, please?

Best regards,

Julien.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot