| Message ID | 20240917141725.466514-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | kbuild: support building external modules in a separate build directory | expand |
On Tue, Sep 17, 2024 at 11:16:28PM +0900, Masahiro Yamada wrote: > > There has been a long-standing request to support building external > modules in a separate build directory. Thanks a lot, you are making several of my colleages very happy with your patch set! > The first half is cleanups of documents and Makefiles. > > The last part adds KBUILD_EXTMOD_OUTPUT (MO=). > This is too big changes, and too late for the current MW. > (I did not test kselftest at all.) > I hope people test this and may uncover some issues. I'm not through all the patches in detail yet, just one observation beforehand: $ make KBUILD_OUTPUT=build allnoconfig $ ./scripts/config --file build/.config --enable modules --enable accessibility $ make KBUILD_OUTPUT=build olddefconfig $ make KBUILD_OUTPUT=build $ make KBUILD_OUTPUT=build CONFIG_SPEAKUP=m MO=/tmp/build M=~+/drivers/accessibility/speakup modules /home/nschier/src/kbuild-review/drivers/accessibility/speakup/genmap.c:23:10: fatal error: mapdata.h: No such file or directory 23 | #include "mapdata.h" | ^~~~~~~~~~~ compilation terminated. make[3]: *** [/home/nschier/src/kbuild-review/scripts/Makefile.host:133: genmap.o] Error 1 make[3]: *** Waiting for unfinished jobs.... make[2]: *** [/home/nschier/src/kbuild-review/Makefile:1971: .] Error 2 make[1]: *** [/home/nschier/src/kbuild-review/Makefile:251: __sub-make] Error 2 make: *** [Makefile:251: __sub-make] Error 2 [exit code 2] If I add "EXTRA_CFLAGS=-I${MO} and EXTRA_HOSTCFLAGS=-I${MO}" to the module build command, it works as expected. Patching this into kbuild works for me, too, but I haven't checked whether it breaks some other scenarios: diff --git a/scripts/Makefile.host b/scripts/Makefile.host index e01c13a588dd..056c7da2776f 100644 --- a/scripts/Makefile.host +++ b/scripts/Makefile.host @@ -97,10 +97,13 @@ hostrust_flags = --out-dir $(dir $@) --emit=dep-info=$(depfile) \ $(HOSTRUSTFLAGS_$(target-stem)) # $(objtree)/$(obj) for including generated headers from checkin source files -ifeq ($(KBUILD_EXTMOD),) ifdef building_out_of_srctree +ifeq ($(KBUILD_EXTMOD),) hostc_flags += -I $(objtree)/$(obj) hostcxx_flags += -I $(objtree)/$(obj) +else +hostc_flags += -I $(CURDIR) +hostcxx_flags += -I $(CURDIR) endif endif diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 1bdd77f42289..428a9eb74381 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -190,11 +190,15 @@ endif # $(src) for including checkin headers from generated source files # $(obj) for including generated headers from checkin source files -ifeq ($(KBUILD_EXTMOD),) ifdef building_out_of_srctree +ifeq ($(KBUILD_EXTMOD),) _c_flags += $(addprefix -I, $(src) $(obj)) _a_flags += $(addprefix -I, $(src) $(obj)) _cpp_flags += $(addprefix -I, $(src) $(obj)) +else +_c_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) +_a_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) +_cpp_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) endif endif Is '-I$(MO)' in CFLAGS/HOSTCFLAGS is something we should support by default, or should this be added to the external module's Makefile by the respective developers themselves? Kind regards, Nicolas
On Fri, Sep 20, 2024 at 7:39 PM Nicolas Schier <n.schier@avm.de> wrote: > > On Tue, Sep 17, 2024 at 11:16:28PM +0900, Masahiro Yamada wrote: > > > > There has been a long-standing request to support building external > > modules in a separate build directory. > > Thanks a lot, you are making several of my colleages very happy with > your patch set! > > > The first half is cleanups of documents and Makefiles. > > > > The last part adds KBUILD_EXTMOD_OUTPUT (MO=). > > This is too big changes, and too late for the current MW. > > (I did not test kselftest at all.) > > I hope people test this and may uncover some issues. > > I'm not through all the patches in detail yet, just one observation beforehand: > > $ make KBUILD_OUTPUT=build allnoconfig > $ ./scripts/config --file build/.config --enable modules --enable accessibility > $ make KBUILD_OUTPUT=build olddefconfig > $ make KBUILD_OUTPUT=build > $ make KBUILD_OUTPUT=build CONFIG_SPEAKUP=m MO=/tmp/build M=~+/drivers/accessibility/speakup modules > /home/nschier/src/kbuild-review/drivers/accessibility/speakup/genmap.c:23:10: fatal error: mapdata.h: No such file or directory > 23 | #include "mapdata.h" > | ^~~~~~~~~~~ > compilation terminated. > make[3]: *** [/home/nschier/src/kbuild-review/scripts/Makefile.host:133: genmap.o] Error 1 > make[3]: *** Waiting for unfinished jobs.... > make[2]: *** [/home/nschier/src/kbuild-review/Makefile:1971: .] Error 2 > make[1]: *** [/home/nschier/src/kbuild-review/Makefile:251: __sub-make] Error 2 > make: *** [Makefile:251: __sub-make] Error 2 > [exit code 2] > > If I add "EXTRA_CFLAGS=-I${MO} and EXTRA_HOSTCFLAGS=-I${MO}" to the module > build command, it works as expected. > > Patching this into kbuild works for me, too, but I haven't checked whether it > breaks some other scenarios: > > diff --git a/scripts/Makefile.host b/scripts/Makefile.host > index e01c13a588dd..056c7da2776f 100644 > --- a/scripts/Makefile.host > +++ b/scripts/Makefile.host > @@ -97,10 +97,13 @@ hostrust_flags = --out-dir $(dir $@) --emit=dep-info=$(depfile) \ > $(HOSTRUSTFLAGS_$(target-stem)) > > # $(objtree)/$(obj) for including generated headers from checkin source files > -ifeq ($(KBUILD_EXTMOD),) > ifdef building_out_of_srctree > +ifeq ($(KBUILD_EXTMOD),) > hostc_flags += -I $(objtree)/$(obj) > hostcxx_flags += -I $(objtree)/$(obj) > +else > +hostc_flags += -I $(CURDIR) > +hostcxx_flags += -I $(CURDIR) > endif > endif > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 1bdd77f42289..428a9eb74381 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -190,11 +190,15 @@ endif > > # $(src) for including checkin headers from generated source files > # $(obj) for including generated headers from checkin source files > -ifeq ($(KBUILD_EXTMOD),) > ifdef building_out_of_srctree > +ifeq ($(KBUILD_EXTMOD),) > _c_flags += $(addprefix -I, $(src) $(obj)) > _a_flags += $(addprefix -I, $(src) $(obj)) > _cpp_flags += $(addprefix -I, $(src) $(obj)) > +else > +_c_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > +_a_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > +_cpp_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > endif > endif > > Is '-I$(MO)' in CFLAGS/HOSTCFLAGS is something we should support by > default, or should this be added to the external module's Makefile by > the respective developers themselves? > > Kind regards, > Nicolas We can fix it more simply. diff --git a/scripts/Makefile.host b/scripts/Makefile.host index e01c13a588dd..c1dedf646a39 100644 --- a/scripts/Makefile.host +++ b/scripts/Makefile.host @@ -96,12 +96,10 @@ hostrust_flags = --out-dir $(dir $@) --emit=dep-info=$(depfile) \ $(KBUILD_HOSTRUSTFLAGS) $(HOST_EXTRARUSTFLAGS) \ $(HOSTRUSTFLAGS_$(target-stem)) -# $(objtree)/$(obj) for including generated headers from checkin source files -ifeq ($(KBUILD_EXTMOD),) +# $(obj) for including generated headers from checkin source files ifdef building_out_of_srctree -hostc_flags += -I $(objtree)/$(obj) -hostcxx_flags += -I $(objtree)/$(obj) -endif +hostc_flags += -I $(obj) +hostcxx_flags += -I $(obj) endif ##### diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 1bdd77f42289..d8ce0f59fd17 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -190,13 +190,11 @@ endif # $(src) for including checkin headers from generated source files # $(obj) for including generated headers from checkin source files -ifeq ($(KBUILD_EXTMOD),) ifdef building_out_of_srctree _c_flags += $(addprefix -I, $(src) $(obj)) _a_flags += $(addprefix -I, $(src) $(obj)) _cpp_flags += $(addprefix -I, $(src) $(obj)) endif -endif # If $(is-kernel-object) is 'y', this object will be linked to vmlinux or modules is-kernel-object = $(or $(part-of-builtin),$(part-of-module)) However, I'd rather fix each Makefile to add necessary include paths explicitly.
On Fri, Sep 20, 2024 at 09:58:47PM +0900, Masahiro Yamada wrote: > On Fri, Sep 20, 2024 at 7:39 PM Nicolas Schier <n.schier@avm.de> wrote: > > > > On Tue, Sep 17, 2024 at 11:16:28PM +0900, Masahiro Yamada wrote: > > > > > > There has been a long-standing request to support building external > > > modules in a separate build directory. > > > > Thanks a lot, you are making several of my colleages very happy with > > your patch set! > > > > > The first half is cleanups of documents and Makefiles. > > > > > > The last part adds KBUILD_EXTMOD_OUTPUT (MO=). > > > This is too big changes, and too late for the current MW. > > > (I did not test kselftest at all.) > > > I hope people test this and may uncover some issues. > > > > I'm not through all the patches in detail yet, just one observation beforehand: > > > > $ make KBUILD_OUTPUT=build allnoconfig > > $ ./scripts/config --file build/.config --enable modules --enable accessibility > > $ make KBUILD_OUTPUT=build olddefconfig > > $ make KBUILD_OUTPUT=build > > $ make KBUILD_OUTPUT=build CONFIG_SPEAKUP=m MO=/tmp/build M=~+/drivers/accessibility/speakup modules > > /home/nschier/src/kbuild-review/drivers/accessibility/speakup/genmap.c:23:10: fatal error: mapdata.h: No such file or directory > > 23 | #include "mapdata.h" > > | ^~~~~~~~~~~ > > compilation terminated. > > make[3]: *** [/home/nschier/src/kbuild-review/scripts/Makefile.host:133: genmap.o] Error 1 > > make[3]: *** Waiting for unfinished jobs.... > > make[2]: *** [/home/nschier/src/kbuild-review/Makefile:1971: .] Error 2 > > make[1]: *** [/home/nschier/src/kbuild-review/Makefile:251: __sub-make] Error 2 > > make: *** [Makefile:251: __sub-make] Error 2 > > [exit code 2] > > > > If I add "EXTRA_CFLAGS=-I${MO} and EXTRA_HOSTCFLAGS=-I${MO}" to the module > > build command, it works as expected. > > > > Patching this into kbuild works for me, too, but I haven't checked whether it > > breaks some other scenarios: > > > > diff --git a/scripts/Makefile.host b/scripts/Makefile.host > > index e01c13a588dd..056c7da2776f 100644 > > --- a/scripts/Makefile.host > > +++ b/scripts/Makefile.host > > @@ -97,10 +97,13 @@ hostrust_flags = --out-dir $(dir $@) --emit=dep-info=$(depfile) \ > > $(HOSTRUSTFLAGS_$(target-stem)) > > > > # $(objtree)/$(obj) for including generated headers from checkin source files > > -ifeq ($(KBUILD_EXTMOD),) > > ifdef building_out_of_srctree > > +ifeq ($(KBUILD_EXTMOD),) > > hostc_flags += -I $(objtree)/$(obj) > > hostcxx_flags += -I $(objtree)/$(obj) > > +else > > +hostc_flags += -I $(CURDIR) > > +hostcxx_flags += -I $(CURDIR) > > endif > > endif > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > > index 1bdd77f42289..428a9eb74381 100644 > > --- a/scripts/Makefile.lib > > +++ b/scripts/Makefile.lib > > @@ -190,11 +190,15 @@ endif > > > > # $(src) for including checkin headers from generated source files > > # $(obj) for including generated headers from checkin source files > > -ifeq ($(KBUILD_EXTMOD),) > > ifdef building_out_of_srctree > > +ifeq ($(KBUILD_EXTMOD),) > > _c_flags += $(addprefix -I, $(src) $(obj)) > > _a_flags += $(addprefix -I, $(src) $(obj)) > > _cpp_flags += $(addprefix -I, $(src) $(obj)) > > +else > > +_c_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > > +_a_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > > +_cpp_flags += $(addprefix -I, $(src) $(obj) $(CURDIR)) > > endif > > endif > > > > Is '-I$(MO)' in CFLAGS/HOSTCFLAGS is something we should support by > > default, or should this be added to the external module's Makefile by > > the respective developers themselves? > > > > Kind regards, > > Nicolas > > > > We can fix it more simply. Ah, yes. > diff --git a/scripts/Makefile.host b/scripts/Makefile.host > index e01c13a588dd..c1dedf646a39 100644 > --- a/scripts/Makefile.host > +++ b/scripts/Makefile.host > @@ -96,12 +96,10 @@ hostrust_flags = --out-dir $(dir $@) > --emit=dep-info=$(depfile) \ > $(KBUILD_HOSTRUSTFLAGS) $(HOST_EXTRARUSTFLAGS) \ > $(HOSTRUSTFLAGS_$(target-stem)) > > -# $(objtree)/$(obj) for including generated headers from checkin source files > -ifeq ($(KBUILD_EXTMOD),) > +# $(obj) for including generated headers from checkin source files > ifdef building_out_of_srctree > -hostc_flags += -I $(objtree)/$(obj) > -hostcxx_flags += -I $(objtree)/$(obj) > -endif > +hostc_flags += -I $(obj) > +hostcxx_flags += -I $(obj) > endif > > ##### > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 1bdd77f42289..d8ce0f59fd17 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -190,13 +190,11 @@ endif > > # $(src) for including checkin headers from generated source files > # $(obj) for including generated headers from checkin source files > -ifeq ($(KBUILD_EXTMOD),) > ifdef building_out_of_srctree > _c_flags += $(addprefix -I, $(src) $(obj)) > _a_flags += $(addprefix -I, $(src) $(obj)) > _cpp_flags += $(addprefix -I, $(src) $(obj)) > endif > -endif > > # If $(is-kernel-object) is 'y', this object will be linked to > vmlinux or modules > is-kernel-object = $(or $(part-of-builtin),$(part-of-module)) > > > > > > However, I'd rather fix each Makefile > to add necessary include paths explicitly. Sure, clearly makes sense. Especially as the explicit include path addition is also as simple as: HOSTCFLAGS_genmap.o += -I $(obj) CFLAGS_main.o += -I $(obj) Kind regards, Nicolas
On Tue, Sep 17, 2024 at 11:17 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > There has been a long-standing request to support building external > modules in a separate build directory. > > The first half is cleanups of documents and Makefiles. > > The last part adds KBUILD_EXTMOD_OUTPUT (MO=). > This is too big changes, and too late for the current MW. > (I did not test kselftest at all.) > I hope people test this and may uncover some issues. > > > > Masahiro Yamada (23): > kbuild: doc: update the description about Kbuild/Makefile split > kbuild: doc: remove description about grepping CONFIG options > kbuild: doc: remove outdated description of the limitation on -I usage > kbuild: doc: remove the description about shipped files > kbuild: doc: describe the -C option precisely for external module > builds > kbuild: doc: replace "gcc" in external module description > kbuild: remove unnecessary prune of rust/alloc for rustfmt > kbuild: simplify find command for rustfmt > speakup: use SPKDIR=$(src) to specify the source directory > kbuild: refactor the check for missing config files > kbuild: check the presence of include/generated/rustc_cfg > scripts/nsdeps: use VPATH as src_prefix > kbuild: replace two $(abs_objtree) with $(CURDIR) in top Makefile > kbuild: add $(objtree)/ prefix to some in-kernel build artifacts > kbuild: rename abs_objtree to abs_output > kbuild: use 'output' variable to create the output directory > kbuild: build external modules in their directory > kbuild: remove extmod_prefix, MODORDER, MODULES_NSDEPS variables > kbuild: support building external modules in a separate build > directory > kbuild: support -fmacro-prefix-map for external modules > kbuild: use absolute path in the generated wrapper Makefile > kbuild: make wrapper Makefile more convenient for external modules > kbuild: allow to start building external module in any directory I CC'ed rust ML because Ack for the following patches are appreciated. [07/23] kbuild: remove unnecessary prune of rust/alloc for rustfmt [08/23] kbuild: simplify find command for rustfmt [11/23] kbuild: check the presence of include/generated/rustc_cfg
On Sat, Sep 28, 2024 at 8:50 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > I CC'ed rust ML because Ack for the following patches are appreciated. > > [07/23] kbuild: remove unnecessary prune of rust/alloc for rustfmt > [08/23] kbuild: simplify find command for rustfmt > [11/23] kbuild: check the presence of include/generated/rustc_cfg Sorry, it was in my backlog after the conferences. I am not sure what the base of the series was, but ran my usual tests on top of v6.11 and then on top of `rust-fixes` after some manual adjustment in both cases, and things appear to still work fine (i.e. what I usually build, without taking advantage of the separate build directory support). Moreover, I tested the separate build directory support (`MO=`), for a trivial Rust out-of-tree module, with a subdir as well as with a directory outside the out-of-tree source code. I also tested the new approach suggested for the out-of-tree `Makefile` (i.e. `export KBUILD_EXTMOD` and `include $(KDIR)/Makefile`), and it all worked as expected, so: Tested-by: Miguel Ojeda <ojeda@kernel.org> ...except for arm64, where I found that I needed this bit: diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index b058c4803efb..4cd9a1f2ec3d 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -71,7 +71,7 @@ stack_protector_prepare: prepare0 -mstack-protector-guard-reg=sp_el0 \ -mstack-protector-guard-offset=$(shell \ awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \ - include/generated/asm-offsets.h)) + $(objtree)/include/generated/asm-offsets.h)) endif ifeq ($(CONFIG_ARM64_BTI_KERNEL),y) Cheers, Miguel
On Tue, Sep 17, 2024 at 11:16:28PM +0900, Masahiro Yamada wrote: > > There has been a long-standing request to support building external > modules in a separate build directory. > > The first half is cleanups of documents and Makefiles. > > The last part adds KBUILD_EXTMOD_OUTPUT (MO=). > This is too big changes, and too late for the current MW. > (I did not test kselftest at all.) > I hope people test this and may uncover some issues. thanks again for the whole series. I really appreciated to go through this patch set and am sorry for that it took so long. I have tested only with some in-tree kmods and with kmods' testsuite modules (but only on amd64) and could not find any major issue, only the minor things reported. I also did not test the kselftest or similar. Kind regards, Nicolas
There has been a long-standing request to support building external modules in a separate build directory. The first half is cleanups of documents and Makefiles. The last part adds KBUILD_EXTMOD_OUTPUT (MO=). This is too big changes, and too late for the current MW. (I did not test kselftest at all.) I hope people test this and may uncover some issues. Masahiro Yamada (23): kbuild: doc: update the description about Kbuild/Makefile split kbuild: doc: remove description about grepping CONFIG options kbuild: doc: remove outdated description of the limitation on -I usage kbuild: doc: remove the description about shipped files kbuild: doc: describe the -C option precisely for external module builds kbuild: doc: replace "gcc" in external module description kbuild: remove unnecessary prune of rust/alloc for rustfmt kbuild: simplify find command for rustfmt speakup: use SPKDIR=$(src) to specify the source directory kbuild: refactor the check for missing config files kbuild: check the presence of include/generated/rustc_cfg scripts/nsdeps: use VPATH as src_prefix kbuild: replace two $(abs_objtree) with $(CURDIR) in top Makefile kbuild: add $(objtree)/ prefix to some in-kernel build artifacts kbuild: rename abs_objtree to abs_output kbuild: use 'output' variable to create the output directory kbuild: build external modules in their directory kbuild: remove extmod_prefix, MODORDER, MODULES_NSDEPS variables kbuild: support building external modules in a separate build directory kbuild: support -fmacro-prefix-map for external modules kbuild: use absolute path in the generated wrapper Makefile kbuild: make wrapper Makefile more convenient for external modules kbuild: allow to start building external module in any directory Documentation/dev-tools/coccinelle.rst | 19 +-- Documentation/kbuild/kbuild.rst | 11 ++ Documentation/kbuild/modules.rst | 104 ++++---------- Makefile | 184 ++++++++++++++++--------- drivers/accessibility/speakup/Makefile | 4 +- rust/Makefile | 4 +- scripts/Kbuild.include | 2 +- scripts/Makefile.build | 4 +- scripts/Makefile.compiler | 2 +- scripts/Makefile.modfinal | 16 +-- scripts/Makefile.modinst | 4 +- scripts/Makefile.modpost | 24 ++-- scripts/coccicheck | 6 +- scripts/depmod.sh | 4 +- scripts/nsdeps | 6 +- scripts/package/install-extmod-build | 7 + 16 files changed, 209 insertions(+), 192 deletions(-)