Re: [PATCH 00/23] kbuild: support building external modules in a separate build directory

[Nicolas Schier <n.schier@avm.de>]

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