Re: [Buildroot] [PATCH] package/linux-tools: add USB tools (testusb and ffs-test)

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

Hi Yann, Hervé, Thomas,

On Fri, 24 Mar 2023 11:27:02 +0100
<yann.morin@orange.com> wrote:

> Luca, All,
> 
> On 2023-03-24 10:33 +0100, Luca Ceresoli via buildroot spake thusly:
> > Extend the linux-tools package to also build the userspace USB tools, which
> > currently include testusb and ffs-test.
> > 
> > These tools are in the kernel tree since a long time, but a Makefile was
> > added only in 5.9 to allow building in the same way as other tools provided
> > with the kernel. Here we use the Makefile and thus version >= 5.9 is
> > required. Support for older kernels may be added later if needed.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---  
> [--SNIP--]
> > diff --git a/package/linux-tools/linux-tool-usbtools.mk.in b/package/linux-tools/linux-tool-usbtools.mk.in
> > new file mode 100644
> > index 000000000000..28b3aff25cc2
> > --- /dev/null
> > +++ b/package/linux-tools/linux-tool-usbtools.mk.in
> > @@ -0,0 +1,42 @@
> > +################################################################################
> > +#
> > +# usbtools
> > +#
> > +################################################################################
> > +
> > +LINUX_TOOLS += usbtools
> > +
> > +USBTOOLS_MAKE_OPTS = $(LINUX_MAKE_FLAGS) LDFLAGS="$(TARGET_LDFLAGS)"
> > +
> > +define USBTOOLS_BUILD_CMDS
> > +	$(Q)if ! grep install $(LINUX_DIR)/tools/usb/Makefile >/dev/null 2>&1 ; then \
> > +		echo "Your kernel version is too old to build the USB tools." ; \
> > +		echo "At least kernel 5.9 must be used." ; \
> > +		exit 1 ; \
> > +	fi
> > +
> > +	$(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb \
> > +		$(USBTOOLS_MAKE_OPTS) \
> > +		all
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_LINUX_TOOLS_USBTOOLS_TESTUSB),)
> > +define USBTOOLS_INSTALL_REMOVE_TESTUSB
> > +	$(RM) -rf $(TARGET_DIR)/usr/bin/testusb
> > +endef
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_LINUX_TOOLS_USBTOOLS_FFSTEST),)
> > +define USBTOOLS_INSTALL_REMOVE_FFSTEST
> > +	$(RM) -rf $(TARGET_DIR)/usr/bin/ffs-test
> > +endef
> > +endif
> > +
> > +define USBTOOLS_INSTALL_TARGET_CMDS
> > +	$(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb \
> > +		$(USBTOOLS_MAKE_OPTS) \
> > +		DESTDIR=$(TARGET_DIR) \
> > +		install
> > +	$(USBTOOLS_INSTALL_REMOVE_TESTUSB)
> > +	$(USBTOOLS_INSTALL_REMOVE_FFSTEST)
> > +endef  
> 
> Rather than installing everything (two files) and removing the one that
> is not needed, h=why not just install the one(s) that is(are) needed?
> 
>     LINUX_TOOLS_USBTOOLS_LIST = \
>         $(if $(BR2_PACKAGE_LINUX_TOOLS_USBTOOLS_TESTUSB),$(LINUX_DIR)/tools/usb/testusb) \
>         $(if $(BR2_PACKAGE_LINUX_TOOLS_USBTOOLS_FFSTEST),$(LINUX_DIR)/tools/usb/ffs-test)
> 
>     define USBTOOLS_INSTALL_TARGET_CMDS
>         $(foreach f,$(LINUX_TOOLS_USBTOOLS_LIST),$(INSTALL) -D -m 0755 $(t) $(TARGET_DIR)/usr/bin/$(notdir $(t)))
>     endef

Thanks for your review! I generally prefer using the upstream
installation procedure when one exists, instead of (re)writing it. But
I must agree the removal logic is ugly.

Prompted by your comments, I noticed the upstream Makefile builds and
installs the tools listed in the ALL_TARGETS variable, so in v2 I'm
just setting it, and the upstream logic works just fine with it.

As a side effect, 'RM' is not used anymore, getting rid of all those
unneeded flags reported by Hervé and Thomas! :-)

v2 incoming.

Luca
-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot