[v1,2/3] selftests/nolibc: fix up O= option support

Message ID 759307255e4ca3b37b67321d333967a21122c422.1691073180.git.falcon@tinylab.org
State New
Headers
Series selftests/nolibc: add misc improvments |

Commit Message

Zhangjin Wu Aug. 3, 2023, 2:45 p.m. UTC
  To avoid pollute the source code tree and avoid mrproper for every
architecture switch, the O= argument must be supported.

Both IMAGE and .config are from the building directory, let's use
objtree instead of srctree for them.

If no O= option specified, means building kernel in source code tree,
objtree should be srctree in such case.

Suggested-by: Willy Tarreau <w@1wt.eu>
Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

Comments

Thomas Weißschuh Aug. 4, 2023, 6:25 a.m. UTC | #1
On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> To avoid pollute the source code tree and avoid mrproper for every
> architecture switch, the O= argument must be supported.
> 
> Both IMAGE and .config are from the building directory, let's use
> objtree instead of srctree for them.
> 
> If no O= option specified, means building kernel in source code tree,
> objtree should be srctree in such case.
> 
> Suggested-by: Willy Tarreau <w@1wt.eu>
> Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/Makefile | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index 51fef5e6a152..af590aee063a 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -9,6 +9,9 @@ ifeq ($(srctree),)
>  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
>  endif
>  
> +# add objtree for O= argument, required by IMAGE and .config
> +objtree ?= $(srctree)

Isn't this already set by the included tools/scripts/Makefile.include?

Also I'm not entirely if O= works as intended currently.
When using O=foo in the $LINUX/tools/testing/selftests/nolibc directory
the build instead is happening in $LINUX/foo. But the Makefile first
validates that $LINUX/tools/testing/selftests/nolibc/foo exists.

It seems we need to pass $(COMMAND_O) to the recursive calls to $(MAKE),
too?

> +
>  ifeq ($(ARCH),)
>  include $(srctree)/scripts/subarch.include
>  ARCH = $(SUBARCH)
> @@ -217,12 +220,12 @@ kernel: initramfs
>  
>  # run the tests after building the kernel
>  run: kernel
> -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
>  	$(Q)$(REPORT) $(CURDIR)/run.out
>  
>  # re-run the tests from an existing kernel
>  rerun:
> -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
>  	$(Q)$(REPORT) $(CURDIR)/run.out
>  
>  # report with existing test log
> -- 
> 2.25.1
>
  
Zhangjin Wu Aug. 4, 2023, 7:43 a.m. UTC | #2
Hi, Thomas

> On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > To avoid pollute the source code tree and avoid mrproper for every
> > architecture switch, the O= argument must be supported.
> > 
> > Both IMAGE and .config are from the building directory, let's use
> > objtree instead of srctree for them.
> > 
> > If no O= option specified, means building kernel in source code tree,
> > objtree should be srctree in such case.
> > 
> > Suggested-by: Willy Tarreau <w@1wt.eu>
> > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > index 51fef5e6a152..af590aee063a 100644
> > --- a/tools/testing/selftests/nolibc/Makefile
> > +++ b/tools/testing/selftests/nolibc/Makefile
> > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> >  endif
> >  
> > +# add objtree for O= argument, required by IMAGE and .config
> > +objtree ?= $(srctree)
> 
> Isn't this already set by the included tools/scripts/Makefile.include?
>

Good question, but it is empty if no O= specified, checked it several
times before ;-)

> Also I'm not entirely if O= works as intended currently.
> When using O=foo in the $LINUX/tools/testing/selftests/nolibc directory

It does work, I have used it to test all of the run targets of the
tinyconfig patches like this:

    $ make run O=kernel-$arch RUN_OUT=run.$arch.out ...

Everything about the kernel will be built in $(O).

Just rechecked the O variable in top-level Makefile, selftests/nolibc Makefile
and tools/nolibc Makefile, all of them get the right O value from command line.

From my Makefile experience, an option from command line will be passed
to every sub Makefile via $(-*-command-variables-*-), it has the highest
priority then the others, except when we use 'override' keyword
internally.

> the build instead is happening in $LINUX/foo. But the Makefile first
> validates that $LINUX/tools/testing/selftests/nolibc/foo exists.
>

Sorry, I didn't get your meaning above?

Do you mean this line:

    srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))

by removing tools/testing/selftests/ of curdir's dir (means no nolibc itself),
srctree above is just the top-level kernel source code tree.

> It seems we need to pass $(COMMAND_O) to the recursive calls to $(MAKE),
> too?
>

So, no need to pass O or COMMAND_O.

Thanks,
Zhangjin

> > +
> >  ifeq ($(ARCH),)
> >  include $(srctree)/scripts/subarch.include
> >  ARCH = $(SUBARCH)
> > @@ -217,12 +220,12 @@ kernel: initramfs
> >  
> >  # run the tests after building the kernel
> >  run: kernel
> > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> >  	$(Q)$(REPORT) $(CURDIR)/run.out
> >  
> >  # re-run the tests from an existing kernel
> >  rerun:
> > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> >  	$(Q)$(REPORT) $(CURDIR)/run.out
> >  
> >  # report with existing test log
> > -- 
> > 2.25.1
> >
  
Thomas Weißschuh Aug. 4, 2023, 3:05 p.m. UTC | #3
On 2023-08-04 15:43:42+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > > To avoid pollute the source code tree and avoid mrproper for every
> > > architecture switch, the O= argument must be supported.
> > > 
> > > Both IMAGE and .config are from the building directory, let's use
> > > objtree instead of srctree for them.
> > > 
> > > If no O= option specified, means building kernel in source code tree,
> > > objtree should be srctree in such case.
> > > 
> > > Suggested-by: Willy Tarreau <w@1wt.eu>
> > > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > ---
> > >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > index 51fef5e6a152..af590aee063a 100644
> > > --- a/tools/testing/selftests/nolibc/Makefile
> > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> > >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> > >  endif
> > >  
> > > +# add objtree for O= argument, required by IMAGE and .config
> > > +objtree ?= $(srctree)
> > 
> > Isn't this already set by the included tools/scripts/Makefile.include?
> >
> 
> Good question, but it is empty if no O= specified, checked it several
> times before ;-)

For me it is not empty when I am in tools/testing/selftests/nolibc/.

> > Also I'm not entirely if O= works as intended currently.
> > When using O=foo in the $LINUX/tools/testing/selftests/nolibc directory
> 
> It does work, I have used it to test all of the run targets of the
> tinyconfig patches like this:
> 
>     $ make run O=kernel-$arch RUN_OUT=run.$arch.out ...
> 
> Everything about the kernel will be built in $(O).

It does not work for me:

$ cd $HOME/src/linux/tools/testing/selftests/nolibc
$ make O=out kernel
../../../scripts/Makefile.include:4: *** O=out does not exist.  Stop.
$ mkdir out
$ make O=out kernel
... Stuff happens
... all the output is in $HOME/src/linux/out not in
    tools/testing/selftests/nolibc/out where I would expect it.
    The new out directory for which an error was reported before is
    completely empty.

> Just rechecked the O variable in top-level Makefile, selftests/nolibc Makefile
> and tools/nolibc Makefile, all of them get the right O value from command line.

But the one from the command line is a relative path. And it seems to be
resolved from the kernel source tree instead of from where make is
executed.

> From my Makefile experience, an option from command line will be passed
> to every sub Makefile via $(-*-command-variables-*-), it has the highest
> priority then the others, except when we use 'override' keyword
> internally.
> 
> > the build instead is happening in $LINUX/foo. But the Makefile first
> > validates that $LINUX/tools/testing/selftests/nolibc/foo exists.
> >
> 
> Sorry, I didn't get your meaning above?
> 
> Do you mean this line:
> 
>     srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> 
> by removing tools/testing/selftests/ of curdir's dir (means no nolibc itself),
> srctree above is just the top-level kernel source code tree.

No, srctree should be fine.

> > It seems we need to pass $(COMMAND_O) to the recursive calls to $(MAKE),
> > too?
> >
> 
> So, no need to pass O or COMMAND_O.

As above, it doesn't work for me.

If I add COMMAND_O to all the $(MAKE) commands that are executed
explicitly, how it is also done in Makefile.include itself, things start
to work as I would expect:
All the kernel build output is in out/ in the current directory.

> Thanks,
> Zhangjin
> 
> > > +
> > >  ifeq ($(ARCH),)
> > >  include $(srctree)/scripts/subarch.include
> > >  ARCH = $(SUBARCH)
> > > @@ -217,12 +220,12 @@ kernel: initramfs
> > >  
> > >  # run the tests after building the kernel
> > >  run: kernel
> > > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > >  	$(Q)$(REPORT) $(CURDIR)/run.out
> > >  
> > >  # re-run the tests from an existing kernel
> > >  rerun:
> > > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > >  	$(Q)$(REPORT) $(CURDIR)/run.out
> > >  
> > >  # report with existing test log
> > > -- 
> > > 2.25.1
> > >
  
Thomas Weißschuh Aug. 4, 2023, 4:02 p.m. UTC | #4
On 2023-08-04 23:52:18+0800, Zhangjin Wu wrote:
> > On 2023-08-04 15:43:42+0800, Zhangjin Wu wrote:
> > > Hi, Thomas
> > > 
> > > > On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > > > > To avoid pollute the source code tree and avoid mrproper for every
> > > > > architecture switch, the O= argument must be supported.
> > > > > 
> > > > > Both IMAGE and .config are from the building directory, let's use
> > > > > objtree instead of srctree for them.
> > > > > 
> > > > > If no O= option specified, means building kernel in source code tree,
> > > > > objtree should be srctree in such case.
> > > > > 
> > > > > Suggested-by: Willy Tarreau <w@1wt.eu>
> > > > > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > > > ---
> > > > >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > > > index 51fef5e6a152..af590aee063a 100644
> > > > > --- a/tools/testing/selftests/nolibc/Makefile
> > > > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > > > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> > > > >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> > > > >  endif
> > > > >  
> > > > > +# add objtree for O= argument, required by IMAGE and .config
> > > > > +objtree ?= $(srctree)
> > > > 
> > > > Isn't this already set by the included tools/scripts/Makefile.include?
> > > >
> > > 
> > > Good question, but it is empty if no O= specified, checked it several
> > > times before ;-)
> > 
> > For me it is not empty when I am in tools/testing/selftests/nolibc/.
> >
> 
> Interesting, here is the code I added to check the value:
> 
>     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>     index 22f1e1d73fa8..1ae19e896e24 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -12,6 +12,8 @@ include $(srctree)/scripts/subarch.include
>      ARCH = $(SUBARCH)
>      endif
>     
>     +$(error objtree=$(objtree), srctree=$(srctree))
>     +
> 
> Whenever I do defconfig or run,
> 
>     $ make help
>     Makefile:15: *** objtree=, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> 
> It is only not empty when we pass O explicitly:
> 
>     $ mkdir out
>     $ make help O=out
>     Makefile:15: *** objtree=out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
>     $ make help O=$PWD/out
>     Makefile:15: *** objtree=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out, srctree=/labs/linux-lab/src/linux-stable.  Stop.

Welp, now it's the same for me.
I guess I messed it up before, maybe I forgot to remove your changes
while testing?

Anyways instead of having to manually do stuff with $(objtree) we could
also use $(OUTPUT)$(IMAGE) to always get the correct image.

> [..]
> 
> After align the empty objtree value with you, will renew this patch.

Thanks!
Thomas
  
Zhangjin Wu Aug. 4, 2023, 4:29 p.m. UTC | #5
> On 2023-08-04 23:52:18+0800, Zhangjin Wu wrote:
> > > On 2023-08-04 15:43:42+0800, Zhangjin Wu wrote:
> > > > Hi, Thomas
> > > > 
> > > > > On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > > > > > To avoid pollute the source code tree and avoid mrproper for every
> > > > > > architecture switch, the O= argument must be supported.
> > > > > > 
> > > > > > Both IMAGE and .config are from the building directory, let's use
> > > > > > objtree instead of srctree for them.
> > > > > > 
> > > > > > If no O= option specified, means building kernel in source code tree,
> > > > > > objtree should be srctree in such case.
> > > > > > 
> > > > > > Suggested-by: Willy Tarreau <w@1wt.eu>
> > > > > > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > > > > ---
> > > > > >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > > > > index 51fef5e6a152..af590aee063a 100644
> > > > > > --- a/tools/testing/selftests/nolibc/Makefile
> > > > > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > > > > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> > > > > >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> > > > > >  endif
> > > > > >  
> > > > > > +# add objtree for O= argument, required by IMAGE and .config
> > > > > > +objtree ?= $(srctree)
> > > > > 
> > > > > Isn't this already set by the included tools/scripts/Makefile.include?
> > > > >
> > > > 
> > > > Good question, but it is empty if no O= specified, checked it several
> > > > times before ;-)
> > > 
> > > For me it is not empty when I am in tools/testing/selftests/nolibc/.
> > >
> > 
> > Interesting, here is the code I added to check the value:
> > 
> >     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> >     index 22f1e1d73fa8..1ae19e896e24 100644
> >     --- a/tools/testing/selftests/nolibc/Makefile
> >     +++ b/tools/testing/selftests/nolibc/Makefile
> >     @@ -12,6 +12,8 @@ include $(srctree)/scripts/subarch.include
> >      ARCH = $(SUBARCH)
> >      endif
> >     
> >     +$(error objtree=$(objtree), srctree=$(srctree))
> >     +
> > 
> > Whenever I do defconfig or run,
> > 
> >     $ make help
> >     Makefile:15: *** objtree=, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > 
> > It is only not empty when we pass O explicitly:
> > 
> >     $ mkdir out
> >     $ make help O=out
> >     Makefile:15: *** objtree=out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> >     $ make help O=$PWD/out
> >     Makefile:15: *** objtree=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> 
> Welp, now it's the same for me.
> I guess I messed it up before, maybe I forgot to remove your changes
> while testing?
> 
> Anyways instead of having to manually do stuff with $(objtree) we could
> also use $(OUTPUT)$(IMAGE) to always get the correct image.
>

Do you mean here?

    # kernel image names by architecture
    IMAGE_i386    = arch/x86/boot/bzImage
    IMAGE_x86     = arch/x86/boot/bzImage
    IMAGE_arm64   = arch/arm64/boot/Image
    IMAGE_arm     = arch/arm/boot/zImage
    IMAGE_mips    = vmlinuz
    IMAGE_riscv   = arch/riscv/boot/Image
    IMAGE         = $(IMAGE_$(ARCH))
    IMAGE_NAME    = $(notdir $(IMAGE))

It does save another KERNEL_IMAGE macro in my future patch ;-)

But without O=, OUTPUT is also empty like objtree and when empty, it is
assigned as $(CURDIR), not $(srctree) as we expected for IMAGE and .config. To
be cleaner, objtree should also be used:

    - IMAGE         = $(IMAGE_$(ARCH))
    + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))

Is this what you want?

Thanks!
Zhangjin

> > [..]
> > 
> > After align the empty objtree value with you, will renew this patch.
> 
> Thanks!
> Thomas
  
Thomas Weißschuh Aug. 4, 2023, 5:51 p.m. UTC | #6
On 2023-08-05 00:29:10+0800, Zhangjin Wu wrote:
> > On 2023-08-04 23:52:18+0800, Zhangjin Wu wrote:
> > > > On 2023-08-04 15:43:42+0800, Zhangjin Wu wrote:
> > > > > Hi, Thomas
> > > > > 
> > > > > > On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > > > > > > To avoid pollute the source code tree and avoid mrproper for every
> > > > > > > architecture switch, the O= argument must be supported.
> > > > > > > 
> > > > > > > Both IMAGE and .config are from the building directory, let's use
> > > > > > > objtree instead of srctree for them.
> > > > > > > 
> > > > > > > If no O= option specified, means building kernel in source code tree,
> > > > > > > objtree should be srctree in such case.
> > > > > > > 
> > > > > > > Suggested-by: Willy Tarreau <w@1wt.eu>
> > > > > > > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > > > > > ---
> > > > > > >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > > > > > index 51fef5e6a152..af590aee063a 100644
> > > > > > > --- a/tools/testing/selftests/nolibc/Makefile
> > > > > > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > > > > > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> > > > > > >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> > > > > > >  endif
> > > > > > >  
> > > > > > > +# add objtree for O= argument, required by IMAGE and .config
> > > > > > > +objtree ?= $(srctree)
> > > > > > 
> > > > > > Isn't this already set by the included tools/scripts/Makefile.include?
> > > > > >
> > > > > 
> > > > > Good question, but it is empty if no O= specified, checked it several
> > > > > times before ;-)
> > > > 
> > > > For me it is not empty when I am in tools/testing/selftests/nolibc/.
> > > >
> > > 
> > > Interesting, here is the code I added to check the value:
> > > 
> > >     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > >     index 22f1e1d73fa8..1ae19e896e24 100644
> > >     --- a/tools/testing/selftests/nolibc/Makefile
> > >     +++ b/tools/testing/selftests/nolibc/Makefile
> > >     @@ -12,6 +12,8 @@ include $(srctree)/scripts/subarch.include
> > >      ARCH = $(SUBARCH)
> > >      endif
> > >     
> > >     +$(error objtree=$(objtree), srctree=$(srctree))
> > >     +
> > > 
> > > Whenever I do defconfig or run,
> > > 
> > >     $ make help
> > >     Makefile:15: *** objtree=, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > > 
> > > It is only not empty when we pass O explicitly:
> > > 
> > >     $ mkdir out
> > >     $ make help O=out
> > >     Makefile:15: *** objtree=out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > >     $ make help O=$PWD/out
> > >     Makefile:15: *** objtree=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > 
> > Welp, now it's the same for me.
> > I guess I messed it up before, maybe I forgot to remove your changes
> > while testing?
> > 
> > Anyways instead of having to manually do stuff with $(objtree) we could
> > also use $(OUTPUT)$(IMAGE) to always get the correct image.
> >
> 
> Do you mean here?
> 
>     # kernel image names by architecture
>     IMAGE_i386    = arch/x86/boot/bzImage
>     IMAGE_x86     = arch/x86/boot/bzImage
>     IMAGE_arm64   = arch/arm64/boot/Image
>     IMAGE_arm     = arch/arm/boot/zImage
>     IMAGE_mips    = vmlinuz
>     IMAGE_riscv   = arch/riscv/boot/Image
>     IMAGE         = $(IMAGE_$(ARCH))
>     IMAGE_NAME    = $(notdir $(IMAGE))
> 
> It does save another KERNEL_IMAGE macro in my future patch ;-)
> 
> But without O=, OUTPUT is also empty like objtree and when empty, it is
> assigned as $(CURDIR), not $(srctree) as we expected for IMAGE and .config. To
> be cleaner, objtree should also be used:
> 
>     - IMAGE         = $(IMAGE_$(ARCH))
>     + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> 
> Is this what you want?

More like:

-	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
+	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(OUTPUT)$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"

My assumption was that it's weird that we need to define such variables
ourselves.

Using an empty $(OUTPUT) would have been fine if make is run from the
root of the kernel tree. But that is not the case.

It still feels weird but I can't think of a nicer way, and it's not
a big issue. So let's keep that part the same.

Or maybe Willy has a better idea.
  
Willy Tarreau Aug. 4, 2023, 5:54 p.m. UTC | #7
On Fri, Aug 04, 2023 at 07:51:50PM +0200, Thomas Weißschuh wrote:
> On 2023-08-05 00:29:10+0800, Zhangjin Wu wrote:
> > > On 2023-08-04 23:52:18+0800, Zhangjin Wu wrote:
> > > > > On 2023-08-04 15:43:42+0800, Zhangjin Wu wrote:
> > > > > > Hi, Thomas
> > > > > > 
> > > > > > > On 2023-08-03 22:45:52+0800, Zhangjin Wu wrote:
> > > > > > > > To avoid pollute the source code tree and avoid mrproper for every
> > > > > > > > architecture switch, the O= argument must be supported.
> > > > > > > > 
> > > > > > > > Both IMAGE and .config are from the building directory, let's use
> > > > > > > > objtree instead of srctree for them.
> > > > > > > > 
> > > > > > > > If no O= option specified, means building kernel in source code tree,
> > > > > > > > objtree should be srctree in such case.
> > > > > > > > 
> > > > > > > > Suggested-by: Willy Tarreau <w@1wt.eu>
> > > > > > > > Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/
> > > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > > > > > > ---
> > > > > > > >  tools/testing/selftests/nolibc/Makefile | 7 +++++--
> > > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > > > > > > index 51fef5e6a152..af590aee063a 100644
> > > > > > > > --- a/tools/testing/selftests/nolibc/Makefile
> > > > > > > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > > > > > > @@ -9,6 +9,9 @@ ifeq ($(srctree),)
> > > > > > > >  srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
> > > > > > > >  endif
> > > > > > > >  
> > > > > > > > +# add objtree for O= argument, required by IMAGE and .config
> > > > > > > > +objtree ?= $(srctree)
> > > > > > > 
> > > > > > > Isn't this already set by the included tools/scripts/Makefile.include?
> > > > > > >
> > > > > > 
> > > > > > Good question, but it is empty if no O= specified, checked it several
> > > > > > times before ;-)
> > > > > 
> > > > > For me it is not empty when I am in tools/testing/selftests/nolibc/.
> > > > >
> > > > 
> > > > Interesting, here is the code I added to check the value:
> > > > 
> > > >     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > >     index 22f1e1d73fa8..1ae19e896e24 100644
> > > >     --- a/tools/testing/selftests/nolibc/Makefile
> > > >     +++ b/tools/testing/selftests/nolibc/Makefile
> > > >     @@ -12,6 +12,8 @@ include $(srctree)/scripts/subarch.include
> > > >      ARCH = $(SUBARCH)
> > > >      endif
> > > >     
> > > >     +$(error objtree=$(objtree), srctree=$(srctree))
> > > >     +
> > > > 
> > > > Whenever I do defconfig or run,
> > > > 
> > > >     $ make help
> > > >     Makefile:15: *** objtree=, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > > > 
> > > > It is only not empty when we pass O explicitly:
> > > > 
> > > >     $ mkdir out
> > > >     $ make help O=out
> > > >     Makefile:15: *** objtree=out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > > >     $ make help O=$PWD/out
> > > >     Makefile:15: *** objtree=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out, srctree=/labs/linux-lab/src/linux-stable.  Stop.
> > > 
> > > Welp, now it's the same for me.
> > > I guess I messed it up before, maybe I forgot to remove your changes
> > > while testing?
> > > 
> > > Anyways instead of having to manually do stuff with $(objtree) we could
> > > also use $(OUTPUT)$(IMAGE) to always get the correct image.
> > >
> > 
> > Do you mean here?
> > 
> >     # kernel image names by architecture
> >     IMAGE_i386    = arch/x86/boot/bzImage
> >     IMAGE_x86     = arch/x86/boot/bzImage
> >     IMAGE_arm64   = arch/arm64/boot/Image
> >     IMAGE_arm     = arch/arm/boot/zImage
> >     IMAGE_mips    = vmlinuz
> >     IMAGE_riscv   = arch/riscv/boot/Image
> >     IMAGE         = $(IMAGE_$(ARCH))
> >     IMAGE_NAME    = $(notdir $(IMAGE))
> > 
> > It does save another KERNEL_IMAGE macro in my future patch ;-)
> > 
> > But without O=, OUTPUT is also empty like objtree and when empty, it is
> > assigned as $(CURDIR), not $(srctree) as we expected for IMAGE and .config. To
> > be cleaner, objtree should also be used:
> > 
> >     - IMAGE         = $(IMAGE_$(ARCH))
> >     + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> > 
> > Is this what you want?
> 
> More like:
> 
> -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(OUTPUT)$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> 
> My assumption was that it's weird that we need to define such variables
> ourselves.
> 
> Using an empty $(OUTPUT) would have been fine if make is run from the
> root of the kernel tree. But that is not the case.
> 
> It still feels weird but I can't think of a nicer way, and it's not
> a big issue. So let's keep that part the same.
> 
> Or maybe Willy has a better idea.

I've just glanced over the discussion, but I'm wondering, why not
"$(objtree)/$(IMAGE)" instead ?

Willy
  
Willy Tarreau Aug. 4, 2023, 6:43 p.m. UTC | #8
On Sat, Aug 05, 2023 at 02:40:23AM +0800, Zhangjin Wu wrote:
> > On Fri, Aug 04, 2023 at 07:51:50PM +0200, Thomas Wei?schuh wrote:
> > > On 2023-08-05 00:29:10+0800, Zhangjin Wu wrote:
> [...]
> > > > Do you mean here?
> > > > 
> > > >     # kernel image names by architecture
> > > >     IMAGE_i386    = arch/x86/boot/bzImage
> > > >     IMAGE_x86     = arch/x86/boot/bzImage
> > > >     IMAGE_arm64   = arch/arm64/boot/Image
> > > >     IMAGE_arm     = arch/arm/boot/zImage
> > > >     IMAGE_mips    = vmlinuz
> > > >     IMAGE_riscv   = arch/riscv/boot/Image
> > > >     IMAGE         = $(IMAGE_$(ARCH))
> > > >     IMAGE_NAME    = $(notdir $(IMAGE))
> > > > 
> > > > It does save another KERNEL_IMAGE macro in my future patch ;-)
> > > > 
> > > > But without O=, OUTPUT is also empty like objtree and when empty, it is
> > > > assigned as $(CURDIR), not $(srctree) as we expected for IMAGE and .config. To
> > > > be cleaner, objtree should also be used:
> > > > 
> > > >     - IMAGE         = $(IMAGE_$(ARCH))
> > > >     + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> > > > 
> > > > Is this what you want?
> > > 
> > > More like:
> > > 
> > > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(OUTPUT)$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > 
> > > My assumption was that it's weird that we need to define such variables
> > > ourselves.
> > > 
> > > Using an empty $(OUTPUT) would have been fine if make is run from the
> > > root of the kernel tree. But that is not the case.
> > > 
> > > It still feels weird but I can't think of a nicer way, and it's not
> > > a big issue. So let's keep that part the same.
> > > 
> > > Or maybe Willy has a better idea.
> > 
> > I've just glanced over the discussion, but I'm wondering, why not
> > "$(objtree)/$(IMAGE)" instead ?
> >
> 
> We used "$(objtree)/$(IMAGE)" originally, I thought of Thomas asked me to
> further add "$(objtree)" in the first place of IMAGE,
> 
>      - IMAGE         = $(IMAGE_$(ARCH))
>      + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> 
> And then, we can use "IMAGE" directly in the qemu run & rerun targets:
> 
>     # run the tests after building the kernel
>     run: kernel
>     -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
>     +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
>     	$(Q)$(REPORT) "$(RUN_OUT)"
>     
>     # re-run the tests from an existing kernel
>     rerun:
>     -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
>     +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
>     	$(Q)$(REPORT) "$(RUN_OUT)"
> 
> Which one do you prefer? will renew this series soon.

It's not a matter of preference but which solution really works. In short
it should work fine and consistently when launched from topdir and from
the nolibc-test dir. I personally don't know the effect of O= when run
from a subdir (is it relative to the current dir or topdir?).

> And even further, I thought of puting everything to $(OUTPUT), but the
> change is very ugly and not good for v6.6, If you like, I will send that
> patch only as a discuss stuff.

Yeah I'd rather avoid ugly things for 6.6 now.

Thanks,
Willy
  
Zhangjin Wu Aug. 4, 2023, 7:17 p.m. UTC | #9
> On Sat, Aug 05, 2023 at 02:40:23AM +0800, Zhangjin Wu wrote:
> > > On Fri, Aug 04, 2023 at 07:51:50PM +0200, Thomas Wei?schuh wrote:
> > > > On 2023-08-05 00:29:10+0800, Zhangjin Wu wrote:
> > [...]
> > > > > Do you mean here?
> > > > > 
> > > > >     # kernel image names by architecture
> > > > >     IMAGE_i386    = arch/x86/boot/bzImage
> > > > >     IMAGE_x86     = arch/x86/boot/bzImage
> > > > >     IMAGE_arm64   = arch/arm64/boot/Image
> > > > >     IMAGE_arm     = arch/arm/boot/zImage
> > > > >     IMAGE_mips    = vmlinuz
> > > > >     IMAGE_riscv   = arch/riscv/boot/Image
> > > > >     IMAGE         = $(IMAGE_$(ARCH))
> > > > >     IMAGE_NAME    = $(notdir $(IMAGE))
> > > > > 
> > > > > It does save another KERNEL_IMAGE macro in my future patch ;-)
> > > > > 
> > > > > But without O=, OUTPUT is also empty like objtree and when empty, it is
> > > > > assigned as $(CURDIR), not $(srctree) as we expected for IMAGE and .config. To
> > > > > be cleaner, objtree should also be used:
> > > > > 
> > > > >     - IMAGE         = $(IMAGE_$(ARCH))
> > > > >     + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> > > > > 
> > > > > Is this what you want?
> > > > 
> > > > More like:
> > > > 
> > > > -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > > +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(OUTPUT)$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
> > > > 
> > > > My assumption was that it's weird that we need to define such variables
> > > > ourselves.
> > > > 
> > > > Using an empty $(OUTPUT) would have been fine if make is run from the
> > > > root of the kernel tree. But that is not the case.
> > > > 
> > > > It still feels weird but I can't think of a nicer way, and it's not
> > > > a big issue. So let's keep that part the same.
> > > > 
> > > > Or maybe Willy has a better idea.
> > > 
> > > I've just glanced over the discussion, but I'm wondering, why not
> > > "$(objtree)/$(IMAGE)" instead ?
> > >
> > 
> > We used "$(objtree)/$(IMAGE)" originally, I thought of Thomas asked me to
> > further add "$(objtree)" in the first place of IMAGE,
> > 
> >      - IMAGE         = $(IMAGE_$(ARCH))
> >      + IMAGE         = $(objtree)/$(IMAGE_$(ARCH))
> > 
> > And then, we can use "IMAGE" directly in the qemu run & rerun targets:
> > 
> >     # run the tests after building the kernel
> >     run: kernel
> >     -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
> >     +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
> >     	$(Q)$(REPORT) "$(RUN_OUT)"
> >     
> >     # re-run the tests from an existing kernel
> >     rerun:
> >     -	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
> >     +	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(RUN_OUT)"
> >     	$(Q)$(REPORT) "$(RUN_OUT)"
> > 
> > Which one do you prefer? will renew this series soon.
> 
> It's not a matter of preference but which solution really works. In short
> it should work fine and consistently when launched from topdir and from
> the nolibc-test dir. I personally don't know the effect of O= when run
> from a subdir (is it relative to the current dir or topdir?).
>

As suggested by Thomas, $(COMMAND_O) will be used to pass the absolute
$(O) for every sub $(MAKE), O= will work like this:

    // subdir
    $ pwd
    /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc
    $ mkdir -p out
    $ make run-user O=out -C /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/ -j4 | grep status
    Makefile:122: srctree=/labs/linux-lab/src/linux-stable
    Makefile:123: objtree: out
    Makefile:124: COMMAND_O: O=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out
    Makefile:125: OUTPUT=/labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/out/
    166 test(s): 158 passed,   8 skipped,   0 failed => status: warning

    // topdir
    $ pwd
    /labs/linux-lab/src/linux-stable
    $ mkdir -p out
    $ make run-user O=out -C /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/ -j4 | grep status
    Makefile:122: srctree=/labs/linux-lab/src/linux-stable
    Makefile:123: objtree: out
    Makefile:124: COMMAND_O: O=/labs/linux-lab/src/linux-stable/out
    Makefile:125: OUTPUT=/labs/linux-lab/src/linux-stable/out/
    166 test(s): 158 passed,   8 skipped,   0 failed => status: warning

    // outside
    $ pwd
    /labs/linux-lab/build
    $ mkdir -p out
    $ make run-user O=out -C /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/ -j4 | grep status
    Makefile:122: srctree=/labs/linux-lab/src/linux-stable
    Makefile:123: objtree: out
    Makefile:124: COMMAND_O: O=/labs/linux-lab/build/out
    Makefile:125: OUTPUT=/labs/linux-lab/build/out/
    166 test(s): 158 passed,   8 skipped,   0 failed => status: warning

The change:

    $ grep COMMAND_O -ur tools/testing/selftests/nolibc/Makefile 
    	$(Q)$(MAKE) -C ../../../include/nolibc $(COMMAND_O) ARCH=$(ARCH) OUTPUT=$(OUTPUT)sysroot/ headers_standalone
    	$(Q)$(MAKE) -C $(srctree) $(COMMAND_O) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
    	$(Q)$(MAKE) -C $(srctree) $(COMMAND_O) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

> > And even further, I thought of puting everything to $(OUTPUT), but the
> > change is very ugly and not good for v6.6, If you like, I will send that
> > patch only as a discuss stuff.
> 
> Yeah I'd rather avoid ugly things for 6.6 now.
>

Ok, since you also suggested let's develop in outside directory, it is better
to reserve the left parts as-is ;-)

> Thanks,
> Willy
  

Patch

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 51fef5e6a152..af590aee063a 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -9,6 +9,9 @@  ifeq ($(srctree),)
 srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR)))
 endif
 
+# add objtree for O= argument, required by IMAGE and .config
+objtree ?= $(srctree)
+
 ifeq ($(ARCH),)
 include $(srctree)/scripts/subarch.include
 ARCH = $(SUBARCH)
@@ -217,12 +220,12 @@  kernel: initramfs
 
 # run the tests after building the kernel
 run: kernel
-	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
+	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
 	$(Q)$(REPORT) $(CURDIR)/run.out
 
 # re-run the tests from an existing kernel
 rerun:
-	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
+	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(objtree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
 	$(Q)$(REPORT) $(CURDIR)/run.out
 
 # report with existing test log