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

[cp0613@linux.alibaba.com]

On Sat, 07 Mar 2026 13:46:56 +0100, ju.o@free.fr 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

Hi, Julien,

Thank you for your very detailed review. I will revise according to your
feedback.

Thanks,
Pei

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