linux-kernel - Re: [PATCH 19/23] kbuild: support building external modules in a separate build directory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241003-mustard-marmot-of-storm-af36a2@lindesnes>
Date: Thu, 3 Oct 2024 21:47:35 +0200
From: Nicolas Schier <nicolas@...sle.eu>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: linux-kbuild@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH 19/23] kbuild: support building external modules in a
 separate build directory

On Tue, Sep 17, 2024 at 11:16:47PM +0900, Masahiro Yamada wrote:
> There has been a long-standing request to support building external
> modules in a separate build directory.
> 
> This commit introduces a new environment variable, KBUILD_EXTMOD_OUTPUT,
> and its shorthand Make variable, MO.
> 
> A simple usage:
> 
>  $ make -C <kernel-dir> M=<module-src-dir> MO=<module-build-dir>
> 
> Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> ---
> 
>  Documentation/kbuild/kbuild.rst  |  8 +++++-
>  Documentation/kbuild/modules.rst |  5 +++-
>  Makefile                         | 44 +++++++++++++++++++++++---------
>  3 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
> index 716f6fb70829..66a9dc44ea28 100644
> --- a/Documentation/kbuild/kbuild.rst
> +++ b/Documentation/kbuild/kbuild.rst
> @@ -132,12 +132,18 @@ Specify the output directory when building the kernel.
>  This variable can also be used to point to the kernel output directory when
>  building external modules using kernel build artifacts in a separate build
>  directory. Please note that this does NOT specify the output directory for the
> -external modules themselves.
> +external modules themselves. (Use KBUILD_EXTMOD_OUTPUT for that purpose.)
>  
>  The output directory can also be specified using "O=...".
>  
>  Setting "O=..." takes precedence over KBUILD_OUTPUT.
>  
> +KBUILD_EXTMOD_OUTPUT
> +--------------------
> +Specify the output directory for external modules.
> +
> +Setting "MO=..." takes precedence over KBUILD_EXTMOD_OUTPUT.
> +
>  KBUILD_EXTRA_WARN
>  -----------------
>  Specify the extra build checks. The same value can be assigned by passing
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index 3a6e7bdc0889..03347e13eeb5 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -95,7 +95,7 @@ executed to make module versioning work.
>  	of the kernel output directory if the kernel was built in a separate
>  	build directory.)
>  
> -	make -C $KDIR M=$PWD
> +	make -C $KDIR M=$PWD [MO=$BUILD_DIR]
>  
>  	-C $KDIR
>  		The directory that contains the kernel and relevant build
> @@ -109,6 +109,9 @@ executed to make module versioning work.
>  		directory where the external module (kbuild file) is
>  		located.
>  
> +	MO=$BUILD_DIR
> +		Speficies a separate output directory for the external module.

s/Speficies/Specifies/

> +
>  2.3 Targets
>  ===========
>  
> diff --git a/Makefile b/Makefile
> index 9fbf7ef6e394..b654baa0763a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -134,6 +134,10 @@ ifeq ("$(origin M)", "command line")
>    KBUILD_EXTMOD := $(M)
>  endif
>  
> +ifeq ("$(origin MO)", "command line")
> +  KBUILD_EXTMOD_OUTPUT := $(MO)
> +endif
> +
>  $(if $(word 2, $(KBUILD_EXTMOD)), \
>  	$(error building multiple external modules is not supported))

Should we also check against multiple output directories?

>  
> @@ -187,7 +191,11 @@ ifdef KBUILD_EXTMOD
>      else
>          objtree := $(CURDIR)
>      endif
> -    output := $(KBUILD_EXTMOD)
> +    output := $(or $(KBUILD_EXTMOD_OUTPUT),$(KBUILD_EXTMOD))
> +    # KBUILD_EXTMOD might be a relative path. Remember its absolute path before
> +    # Make changes the working directory.
> +    export abs_extmodtree := $(realpath $(KBUILD_EXTMOD))
> +    $(if $(abs_extmodtree),,$(error specified external module directory "$(KBUILD_EXTMOD)" does not exist))
>  else
>      objtree := .
>      output := $(KBUILD_OUTPUT)
> @@ -246,7 +254,6 @@ else # need-sub-make
>  ifeq ($(abs_srctree),$(CURDIR))
>          # building in the source tree
>          srctree := .
> -	building_out_of_srctree :=
>  else
>          ifeq ($(abs_srctree)/,$(dir $(CURDIR)))
>                  # building in a subdirectory of the source tree
> @@ -254,22 +261,23 @@ else
>          else
>                  srctree := $(abs_srctree)
>          endif
> -	building_out_of_srctree := 1
>  endif
>  
>  ifneq ($(KBUILD_ABS_SRCTREE),)
>  srctree := $(abs_srctree)
>  endif
>  
> -VPATH		:=
> +export srctree
>  
> -ifeq ($(KBUILD_EXTMOD),)
> -ifdef building_out_of_srctree
> -VPATH		:= $(srctree)
> -endif
> -endif
> +_vpath = $(or $(abs_extmodtree),$(srctree))
>  
> -export building_out_of_srctree srctree VPATH
> +ifeq ($(realpath $(_vpath)),$(CURDIR))

Just a style consistency question: 'ifeq (,)' with a space after ',' (as a few
lines above) or without as used here?

> +building_out_of_srctree :=
> +VPATH :=
> +else
> +export building_out_of_srctree := 1
> +export VPATH := $(_vpath)
> +endif
>  
>  # 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
> @@ -550,7 +558,7 @@ USERINCLUDE    := \
>  LINUXINCLUDE    := \
>  		-I$(srctree)/arch/$(SRCARCH)/include \
>  		-I$(objtree)/arch/$(SRCARCH)/include/generated \
> -		$(if $(building_out_of_srctree),-I$(srctree)/include) \
> +		-I$(srctree)/include \
>  		-I$(objtree)/include \
>  		$(USERINCLUDE)
>  
> @@ -640,6 +648,7 @@ quiet_cmd_makefile = GEN     Makefile
>  	} > Makefile
>  
>  outputmakefile:
> +ifeq ($(KBUILD_EXTMOD),)
>  	@if [ -f $(srctree)/.config -o \
>  		 -d $(srctree)/include/config -o \
>  		 -d $(srctree)/arch/$(SRCARCH)/include/generated ]; then \
> @@ -649,7 +658,16 @@ outputmakefile:
>  		echo >&2 "***"; \
>  		false; \
>  	fi
> -	$(Q)ln -fsn $(srctree) source
> +else
> +	@if [ -f $(KBUILD_EXTMOD)/modules.order ]; then \
> +		echo >&2 "***"; \
> +		echo >&2 "*** The external module source tree is not clean."; \
> +		echo >&2 "*** Please run 'make -C $(abs_srctree) M=$(realpath $(KBUILD_EXTMOD)) clean'"; \
> +		echo >&2 "***"; \
> +		false; \
> +	fi
> +endif
> +	$(Q)ln -fsn $(_vpath) source
>  	$(call cmd,makefile)
>  	$(Q)test -e .gitignore || \
>  	{ echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
> @@ -1926,6 +1944,8 @@ KBUILD_MODULES := 1
>  
>  endif
>  
> +prepare: outputmakefile
> +
>  # Preset locale variables to speed up the build process. Limit locale
>  # tweaks to this spot to avoid wrong language settings when running
>  # make menuconfig etc.
> -- 
> 2.43.0
> 

Thanks, this feature is really appreciated by a lot of my colleagues, and I think you found quite a nice solution!
I'm a bit surprised, that there are not some more testers ...

Reviewed-by: Nicolas Schier <nicolas@...sle.eu>