Re: [RFC PATCH v2 2/7] kbuild: add "make tools_build" to build userspace tools from tools/

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kbuild: flawed dry-run detection breaks incremental rebuilds
--

commit af4d0e2311f445e8ceb17d3fd81b05fa2c242614
Author: Sasha Levin <sashal@kernel.org>

kbuild: add "make tools_build" to build userspace tools from tools/

This commit adds a new target to build userspace tools from the tools/
directory as part of the kernel build process. It introduces a Kconfig
menu and a Makefile for building the selected programs independently.

> diff --git a/tools/Makefile.kbuild b/tools/Makefile.kbuild
> new file mode 100644
> index 0000000000000..8cfa7f80039cd
> --- /dev/null
> +++ b/tools/Makefile.kbuild

[ ... ]

> +# $(call build,<output directory>,<make arguments>,<files to check>,<CONFIG_ symbol>)
> +# Builds in an empty directory when the make arguments or LDFLAGS changed, or
> +# when one of the files is newer than the last build. "make -n" only shows
> +# the build.
> +build-key = $(call escsq,$(2) LDFLAGS=$(USERLDFLAGS))
> +dry-run := $(findstring n,$(firstword -$(MAKEFLAGS)))

[Severity: Medium]
Could this dry-run detection logic produce a false positive for common make
arguments?

If MAKEFLAGS starts with a long option containing 'n' (such as Kbuild's
frequent default --no-print-directory), -$(MAKEFLAGS) evaluates to something
like ---no-print-directory, and findstring erroneously matches the letter 'n'.

When evaluated as a dry run, the Makefile silently skips creating the
.tools_build tracking file and refuses to clear out stale objects, which breaks
incremental rebuilds.

Is it possible to use the standard Kbuild idiom here, like
$(findstring n,$(filter-out --%,$(MAKEFLAGS))), to properly check the flags?

> +
> +define build
> +	+$(Q)$(if $(dry-run),,if [ "$$(cat $(1)/.tools_build 2>/dev/null)" != '$(build-key)' ] $(if $(3),|| \
> +	   [ -n "$$(find $(3) -newer $(1)/.tools_build -print -quit 2>/dev/null)" ]); then \
> +		rm -rf $(1); \
> +	fi;) \
> +	mkdir -p $(1) && $(MAKE) $(2) && \
> +	$(if $(dry-run),:,printf '%s' '$(build-key)' > $(1)/.tools_build) || \
> +	{ echo >&2 "tools_build: $(patsubst $(outdir)/%,%,$(1)) failed to build; set CONFIG_$(4)=n to skip it"; false; }
> +endef

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016143143.3613501-1-sashal@kernel.org?part=2