Re: [Buildroot] [PATCH 1/2] package/ndctl: new package

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

Hi Pei,

Thanks for the patch. I have few comments, see below.

On 06/03/2026 04:44, cp0613@linux.alibaba.com wrote:
> From: Chen Pei <cp0613@linux.alibaba.com>
> 
> A "device memory" enabling project encompassing tools and
> libraries for CXL, NVDIMMs, DAX, memory tiering and other
> platform memory device topics.
> 
> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
> ---
[...]
> diff --git a/package/ndctl/Config.in b/package/ndctl/Config.in
> new file mode 100644
> index 0000000000..841e9981b8
> --- /dev/null
> +++ b/package/ndctl/Config.in
> @@ -0,0 +1,21 @@
> +config BR2_PACKAGE_NDCTL
> +	bool "ndctl"
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on BR2_USE_MMU
> +	depends on BR2_LINUX_KERNEL

This "depends on BR2_LINUX_KERNEL" is not needed. It can be
removed. ndctl can compile without the kernel being selected.
The kernel-headers are in the toolchain.

> +	depends on BR2_PACKAGE_HAS_UDEV

There is possibly a missing "depends on 
BR2_TOOLCHAIN_HEADERS_AT_LEAST_..." here.

ndctl cxl/fwctl is using __struct_group(). See:
https://github.com/pmem/ndctl/blob/v83/cxl/fwctl/features.h#L108

This macro was introduced in Kernel 5.16 in:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?&id=50d7bd38c3aafc4749e05e8d7fcb616979143602

So this cannot compile with toolchains with older kernel headers.
When using the Arm external toolchain, compilation fails with error:

In file included from ../test/fwctl.c:17:
../cxl/fwctl/features.h:108:9: error: expected specifier-qualifier-list 
before ‘__struct_group’

The fwctl support is a ndctl meson option, but disabling it
breaks at configuration time. You could probably propose a
fix upstream for that (fixing ndctl when configured with
-Dtest=enabled and -Dfwctl=disabled).

Or you can always pass -Dfwctl=enabled in Buildroot, and add this
kernel header requirement. See the kmemd package for example:
https://gitlab.com/buildroot.org/buildroot/-/blob/master/package/kmemd/Config.in

> +	select BR2_PACKAGE_KMOD
> +	select BR2_PACKAGE_JSON_C
> +	select BR2_PACKAGE_UTIL_LINUX
> +	select BR2_PACKAGE_UTIL_LINUX_LIBS
> +	select BR2_PACKAGE_UTIL_LINUX_LIBUUID
> +	select BR2_PACKAGE_LIBTRACEEVENT
> +	select BR2_PACKAGE_LIBTRACEFS

libtracefs seems to be an optional dependency. See:
https://github.com/pmem/ndctl/blob/v83/meson_options.txt#L5

In Buildroot, we try to keep package dependencies
minimal. So, unless there is a good reason to always enable
it in Buildroot (e.g. upstream support incomplete or broken),
this should be kept optional. So those "select BR2_PACKAGE_..."
should be removed, and the optional dependency should go in
ndctl.mk (like you did for systemd). For example:

ifeq ($(BR2_PACKAGE_TRACEFS)$(BR2_PACKAGE_TRACEEVENT),yy)
NDCTL_CONF_OPTS += -Dlibtracefs=enabled
NDCTL_DEPENDENCIES += libtraceevent libtracefs
else
NDCTL_CONF_OPTS += -Dlibtracefs=disabled
endif

> +	select BR2_PACKAGE_KEYUTILS

The keyutils is optional:
https://github.com/pmem/ndctl/blob/v83/meson_options.txt#L7

Again, trying to build with -Dkeyutils=disabled fails with error:

../ndctl/dimm.c:1030:16: error: too many arguments to function 
‘ndctl_dimm_remove_key’; expected 1, have 2

Ideally, this should be fixed upstream to keep it optional in Buildroot.

If you decide to not fix it upstream, you can add a comment that
building with keyutils support disabled is currently broken upstream.
You can also add -Dkeyutils=enabled in ndctl.mk to show it is always
enabled.

> +	select BR2_PACKAGE_INIPARSER

After those changes, could you sort all the remaining
"select BR2_PACKAGE_" line alphabetically?

> +	help
> +	  A "device memory" enabling project encompassing tools and
> +	  libraries for CXL, NVDIMMs, DAX, memory tiering and other
> +	  platform memory device topics.
> +
> +	  https://github.com/pmem/ndctl

Since the package has specific "depends on" requirements, you
should add here a Kconfig comment when they are not met. See for
example:
https://gitlab.com/buildroot.org/buildroot/-/blob/2026.02/package/collectd/Config.in?ref_type=tags#L617

[...]
> diff --git a/package/ndctl/ndctl.mk b/package/ndctl/ndctl.mk
> new file mode 100644
> index 0000000000..5c8ed86da7
> --- /dev/null
> +++ b/package/ndctl/ndctl.mk
> @@ -0,0 +1,34 @@
> +################################################################################
> +#
> +# ndctl
> +#
> +################################################################################
> +
> +NDCTL_VERSION = 83
> +NDCTL_SITE = $(call github,pmem,ndctl,v$(NDCTL_VERSION))
> +NDCTL_LICENSE = LGPL-2.1+, GPL-2.0+

Reading the file:
https://github.com/pmem/ndctl/blob/v83/COPYING
there is more details of components and extra licenses.

You could add those info in _LICENSE, for ex:

NDCTL_LICENSE = \
	CC0-1.0 (helper routines), \
	GPL-2.0+ (tools), \
	LGPL-2.1+ (libraries), \
	MIT (helper routines)

> +NDCTL_LICENSE_FILES = COPYING

It would also be preferable to include the actual license
files too:

NDCTL_LICENSE_FILES = \
	COPYING \
	LICENSES/other/CC0-1.0 \
	LICENSES/other/MIT \
	LICENSES/preferred/GPL-2.0 \
	LICENSES/preferred/LGPL-2.1

and update the ndctl.hash accordingly.

> +
> +NDCTL_DEPENDENCIES = \
> +	host-meson \
> +	host-ninja \

The host-meson and host-ninja can be removed. They are
automatically added by the package infra in the last line
"$(eval $(meson-package))"

> +	util-linux-libs \
> +	kmod \
> +	json-c \
> +	libtraceevent \
> +	libtracefs \

libtraceevent and libtracefs can be removed from here, and
added in a conditional block to handle those as optional
dependencies.

> +	keyutils \

Same comment for keyutils (if you decide to fix it upstream).

> +	iniparser

You miss a dependency to "udev" in this list. Without it, the
configuration can fail depending the order.

Could you also sort the _DEPENDENCY list alphabetically please?

> +
> +NDCTL_CONF_OPTS = \
> +	-Ddocs=disabled \
> +	-Diniparserdir=$(TARGET_DIR)/usr/include/iniparser

The compilation should not point to $(TARGET_DIR).
You should use $(STAGING_DIR) instead.

> +
> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> +NDCTL_CONF_OPTS += -Dsystemd=enabled
> +NDCTL_DEPENDENCIES += systemd
> +else
> +NDCTL_CONF_OPTS += -Dsystemd=disabled
> +endif
> +
> +$(eval $(meson-package))
> --
> 2.50.1

After those changes, could you also check it passes the compilation
tests with the command:
utils/test-pkg -a -p ndctl

and it also passes checks with the command:
utils/docker-run make check-package

Could you send an updated patch with those changes, please?

Best regards,

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