Re: [PATCH 17/23] kbuild: build external modules in their directory

[Nicolas Schier]

On Tue, Sep 17, 2024 at 11:16:45PM +0900, Masahiro Yamada wrote:
> Currently, Kbuild always operates in the output directory of the kernel,
> even when building external modules. This increases the risk of external
> module Makefiles attempting to write to the kernel directory.
> 
> This commit switches the working directory to the external module
> directory, allowing the removal of the $(KBUILD_EXTMOD)/ prefix from
> some build artifacts.
> 
> The command for building external modules maintains backward
> compatibility, but Makefiles that rely on working in the kernel
> directory may break. In such cases, $(objtree) and $(srctree) should
> be used to refer to the output and source directories of the kernel.
> 
> The appearance of the build log will change as follows:
> 
> [Before]
> 
>   $ make -C /path/to/my/linux M=/path/to/my/externel/module
>   make: Entering directory '/path/to/my/linux'
>     CC [M]  /path/to/my/externel/module/helloworld.o
>     MODPOST /path/to/my/externel/module/Module.symvers
>     CC [M]  /path/to/my/externel/module/helloworld.mod.o
>     CC [M]  /path/to/my/externel/module/.module-common.o
>     LD [M]  /path/to/my/externel/module/helloworld.ko
>   make: Leaving directory '/path/to/my/linux'
> 
> [After]
> 
>   $ make -C /path/to/my/linux M=/path/to/my/externel/module
>   make: Entering directory '/path/to/my/linux'
>   make[1]: Entering directory '/path/to/my/externel/module'
>     CC [M]  helloworld.o
>     MODPOST Module.symvers
>     CC [M]  helloworld.mod.o
>     CC [M]  .module-common.o
>     LD [M]  helloworld.ko
>   make[1]: Leaving directory '/path/to/my/externel/module'
>   make: Leaving directory '/path/to/my/linux'
> 
> Printing "Entering directory" twice is cumbersome. This will be
> addressed later.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
> 
>  Documentation/dev-tools/coccinelle.rst | 19 +++++-------
>  Makefile                               | 40 +++++++++++++++++---------
>  rust/Makefile                          |  4 +--
>  scripts/Makefile.compiler              |  2 +-
>  scripts/Makefile.modpost               |  6 ++--
>  scripts/coccicheck                     |  6 ++--
>  scripts/package/install-extmod-build   |  7 +++++
>  7 files changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/Documentation/dev-tools/coccinelle.rst b/Documentation/dev-tools/coccinelle.rst
> index 535ce126fb4f..80c83ce0babc 100644
> --- a/Documentation/dev-tools/coccinelle.rst
> +++ b/Documentation/dev-tools/coccinelle.rst
> @@ -250,25 +250,20 @@ variables for .cocciconfig is as follows:
>  - Your directory from which spatch is called is processed next
>  - The directory provided with the ``--dir`` option is processed last, if used
>  
> -Since coccicheck runs through make, it naturally runs from the kernel
> -proper dir; as such the second rule above would be implied for picking up a
> -.cocciconfig when using ``make coccicheck``.
> -
>  ``make coccicheck`` also supports using M= targets. If you do not supply
>  any M= target, it is assumed you want to target the entire kernel.
>  The kernel coccicheck script has::
>  
> -    if [ "$KBUILD_EXTMOD" = "" ] ; then
> -        OPTIONS="--dir $srctree $COCCIINCLUDE"
> +    if [ "$VPATH" ] ; then
> +        OPTIONS="--dir $VPATH $COCCIINCLUDE"
>      else
> -        OPTIONS="--dir $KBUILD_EXTMOD $COCCIINCLUDE"
> +        OPTIONS="--dir . $COCCIINCLUDE"
>      fi
>  
> -KBUILD_EXTMOD is set when an explicit target with M= is used. For both cases
> -the spatch ``--dir`` argument is used, as such third rule applies when whether
> -M= is used or not, and when M= is used the target directory can have its own
> -.cocciconfig file. When M= is not passed as an argument to coccicheck the
> -target directory is the same as the directory from where spatch was called.
> +When an explicit target is executed with a separate output directory, VPATH is
> +set to the target source directory. The third rule ensures the spatch reads the

My limited English with German background likes to have "the target's
source directory" here, but I am not sure if this is more correct.

> +.cocciconfig from the target directory. When M= is used, the external module
> +directory can have its own.cocciconfig file.

A space is missing after 'own'.

>  
>  If not using the kernel's coccicheck target, keep the above precedence
>  order logic of .cocciconfig reading. If using the kernel's coccicheck target,
> diff --git a/Makefile b/Makefile
> index 7a76452049ea..4db22c3a8555 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -180,7 +180,20 @@ ifeq ("$(origin O)", "command line")
>    KBUILD_OUTPUT := $(O)
>  endif
>  
> -output := $(KBUILD_OUTPUT)
> +ifdef KBUILD_EXTMOD
> +    ifdef KBUILD_OUTPUT
> +        objtree := $(realpath $(KBUILD_OUTPUT))
> +        $(if $(objtree),,$(error specified kernel directory "$(KBUILD_OUTPUT)" does not exist))
> +    else
> +        objtree := $(CURDIR)
> +    endif
> +    output := $(KBUILD_EXTMOD)
> +else
> +    objtree := .
> +    output := $(KBUILD_OUTPUT)
> +endif
> +
> +export objtree
>  
>  # Do we want to change the working directory?
>  ifneq ($(output),)
> @@ -248,8 +261,6 @@ ifneq ($(KBUILD_ABS_SRCTREE),)
>  srctree := $(abs_srctree)
>  endif
>  
> -objtree		:= .
> -
>  VPATH		:=
>  
>  ifeq ($(KBUILD_EXTMOD),)
> @@ -258,7 +269,7 @@ VPATH		:= $(srctree)
>  endif
>  endif
>  
> -export building_out_of_srctree srctree objtree VPATH
> +export building_out_of_srctree srctree VPATH
>  
>  # To make sure we do not include .config for any of the *config targets
>  # catch them early, and hand them over to scripts/kconfig/Makefile
> @@ -708,7 +719,7 @@ endif
>  # in addition to whatever we do anyway.
>  # Just "make" or "make all" shall build modules as well
>  
> -ifneq ($(filter all modules nsdeps %compile_commands.json clang-%,$(MAKECMDGOALS)),)
> +ifneq ($(filter all modules nsdeps compile_commands.json clang-%,$(MAKECMDGOALS)),)

Nit: I think this would better match to patch "kbuild: remove
extmod_prefix, MODORDER, MODULES_NSDEPS variables", but probably nobody
else will care.

Reviewed-by: Nicolas Schier <nicolas@xxxxxxxxx>