selftests: Makefile: add missing 'net/lib' to targets

[PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Anders Roxell 2 months, 2 weeks ago

Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 tools/testing/selftests/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 3b7df5477317..fc3681270afe 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -64,6 +64,7 @@ TARGETS += net
 TARGETS += net/af_unix
 TARGETS += net/forwarding
 TARGETS += net/hsr
+TARGETS += net/lib
 TARGETS += net/mptcp
 TARGETS += net/netfilter
 TARGETS += net/openvswitch
-- 
2.45.2

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Jakub Kicinski 2 months, 2 weeks ago

On Thu, 12 Sep 2024 08:31:18 +0200 Anders Roxell wrote:
> Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
>  tools/testing/selftests/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 3b7df5477317..fc3681270afe 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -64,6 +64,7 @@ TARGETS += net
>  TARGETS += net/af_unix
>  TARGETS += net/forwarding
>  TARGETS += net/hsr
> +TARGETS += net/lib
>  TARGETS += net/mptcp
>  TARGETS += net/netfilter
>  TARGETS += net/openvswitch

Please make sure you always include a commit message. Among other
things writing one would force you to understand the code, and
in this case understand that this target is intentionally left out.
Look around the Makefile for references to net/lib, you'll figure 
it out.

The patch is incorrect.

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Anders Roxell 2 months, 2 weeks ago

On Thu, 12 Sept 2024 at 17:23, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 Sep 2024 08:31:18 +0200 Anders Roxell wrote:
> > Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > ---
> >  tools/testing/selftests/Makefile | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > index 3b7df5477317..fc3681270afe 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -64,6 +64,7 @@ TARGETS += net
> >  TARGETS += net/af_unix
> >  TARGETS += net/forwarding
> >  TARGETS += net/hsr
> > +TARGETS += net/lib
> >  TARGETS += net/mptcp
> >  TARGETS += net/netfilter
> >  TARGETS += net/openvswitch
>
> Please make sure you always include a commit message. Among other
> things writing one would force you to understand the code, and
> in this case understand that this target is intentionally left out.
> Look around the Makefile for references to net/lib, you'll figure
> it out.
>
> The patch is incorrect.

You’re right, the patch is incorrect, I could have explained better.
I’m seeing an issue with an out-of-tree cross compilation build of
kselftest and can’t figure out what’s wrong.

make --keep-going --jobs=32 O=/tmp/build
INSTALL_PATH=/tmp/build/kselftest_install \
     ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- \
     CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- kselftest-install

[...]
make[4]: Entering directory
'/home/anders/src/kernel/linux/tools/testing/selftests/net/lib'
  CC       csum
/usr/lib/gcc-cross/aarch64-linux-gnu/13/../../../../aarch64-linux-gnu/bin/ld:
cannot open output file /tmp/build/kselftest/net/lib/csum: No such
file or directory
collect2: error: ld returned 1 exit status
[...]

Any thoughts on what might be causing this?

Cheers,
Anders

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Willem de Bruijn 2 months, 2 weeks ago

On Sun, Sep 15, 2024 at 8:45 AM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> On Thu, 12 Sept 2024 at 17:23, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 12 Sep 2024 08:31:18 +0200 Anders Roxell wrote:
> > > Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
> > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > > ---
> > >  tools/testing/selftests/Makefile | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > > index 3b7df5477317..fc3681270afe 100644
> > > --- a/tools/testing/selftests/Makefile
> > > +++ b/tools/testing/selftests/Makefile
> > > @@ -64,6 +64,7 @@ TARGETS += net
> > >  TARGETS += net/af_unix
> > >  TARGETS += net/forwarding
> > >  TARGETS += net/hsr
> > > +TARGETS += net/lib
> > >  TARGETS += net/mptcp
> > >  TARGETS += net/netfilter
> > >  TARGETS += net/openvswitch
> >
> > Please make sure you always include a commit message. Among other
> > things writing one would force you to understand the code, and
> > in this case understand that this target is intentionally left out.
> > Look around the Makefile for references to net/lib, you'll figure
> > it out.
> >
> > The patch is incorrect.
>
> You’re right, the patch is incorrect, I could have explained better.
> I’m seeing an issue with an out-of-tree cross compilation build of
> kselftest and can’t figure out what’s wrong.
>
> make --keep-going --jobs=32 O=/tmp/build
> INSTALL_PATH=/tmp/build/kselftest_install \
>      ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- \
>      CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- kselftest-install
>
> [...]
> make[4]: Entering directory
> '/home/anders/src/kernel/linux/tools/testing/selftests/net/lib'
>   CC       csum
> /usr/lib/gcc-cross/aarch64-linux-gnu/13/../../../../aarch64-linux-gnu/bin/ld:
> cannot open output file /tmp/build/kselftest/net/lib/csum: No such
> file or directory
> collect2: error: ld returned 1 exit status
> [...]
>
> Any thoughts on what might be causing this?

I wonder if this is due to the O= argument.

Last week I noticed that some TARGETs explicitly have support for
this, like x86. Added in 2016 in commit a8ba798bc8ec6 ("selftests:
enable O and KBUILD_OUTPUT"). But by now this support is hardly
universal. amd-pstate does not have this infra, for instance.

Though if the only breakage is in net/lib, then that does not explain it fully.

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Jakub Kicinski 2 months, 2 weeks ago

On Sun, 15 Sep 2024 09:36:10 +0200 Willem de Bruijn wrote:
> > You’re right, the patch is incorrect, I could have explained better.
> > I’m seeing an issue with an out-of-tree cross compilation build of
> > kselftest and can’t figure out what’s wrong.
> >
> > make --keep-going --jobs=32 O=/tmp/build
> > INSTALL_PATH=/tmp/build/kselftest_install \
> >      ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- \
> >      CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- kselftest-install
> >
> > [...]
> > make[4]: Entering directory
> > '/home/anders/src/kernel/linux/tools/testing/selftests/net/lib'
> >   CC       csum
> > /usr/lib/gcc-cross/aarch64-linux-gnu/13/../../../../aarch64-linux-gnu/bin/ld:
> > cannot open output file /tmp/build/kselftest/net/lib/csum: No such
> > file or directory
> > collect2: error: ld returned 1 exit status
> > [...]
> >
> > Any thoughts on what might be causing this?  
> 
> I wonder if this is due to the O= argument.
> 
> Last week I noticed that some TARGETs explicitly have support for
> this, like x86. Added in 2016 in commit a8ba798bc8ec6 ("selftests:
> enable O and KBUILD_OUTPUT"). But by now this support is hardly
> universal. amd-pstate does not have this infra, for instance.
> 
> Though if the only breakage is in net/lib, then that does not explain it fully.

Some funny business with this install target, I haven't investigated
fully but the dependency on all doesn't seem to do its job, and the
install target has a copy/paste of all with this line missing:

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 3b7df5477317..3aee8e7b9993 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -261,6 +261,7 @@ ifdef INSTALL_PATH
 	@ret=1;	\
 	for TARGET in $(TARGETS) $(INSTALL_DEP_TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
+		mkdir -p $$BUILD_TARGET;	\
 		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET install \
 				INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \
 				SRC_PATH=$(shell readlink -e $$(pwd)) \


Andres, please feel free to test / write commit message and submit this
one liner, but even with that the build for some targets fails for me.
"make [..] install" seems wobbly.

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Anders Roxell 2 months, 2 weeks ago

On Sun, 15 Sept 2024 at 16:46, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 15 Sep 2024 09:36:10 +0200 Willem de Bruijn wrote:
> > > You’re right, the patch is incorrect, I could have explained better.
> > > I’m seeing an issue with an out-of-tree cross compilation build of
> > > kselftest and can’t figure out what’s wrong.
> > >
> > > make --keep-going --jobs=32 O=/tmp/build
> > > INSTALL_PATH=/tmp/build/kselftest_install \
> > >      ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- \
> > >      CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- kselftest-install
> > >
> > > [...]
> > > make[4]: Entering directory
> > > '/home/anders/src/kernel/linux/tools/testing/selftests/net/lib'
> > >   CC       csum
> > > /usr/lib/gcc-cross/aarch64-linux-gnu/13/../../../../aarch64-linux-gnu/bin/ld:
> > > cannot open output file /tmp/build/kselftest/net/lib/csum: No such
> > > file or directory
> > > collect2: error: ld returned 1 exit status
> > > [...]
> > >
> > > Any thoughts on what might be causing this?
> >
> > I wonder if this is due to the O= argument.
> >
> > Last week I noticed that some TARGETs explicitly have support for
> > this, like x86. Added in 2016 in commit a8ba798bc8ec6 ("selftests:
> > enable O and KBUILD_OUTPUT"). But by now this support is hardly
> > universal. amd-pstate does not have this infra, for instance.
> >
> > Though if the only breakage is in net/lib, then that does not explain it fully.
>
> Some funny business with this install target, I haven't investigated
> fully but the dependency on all doesn't seem to do its job, and the
> install target has a copy/paste of all with this line missing:
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 3b7df5477317..3aee8e7b9993 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -261,6 +261,7 @@ ifdef INSTALL_PATH
>         @ret=1; \
>         for TARGET in $(TARGETS) $(INSTALL_DEP_TARGETS); do \
>                 BUILD_TARGET=$$BUILD/$$TARGET;  \
> +               mkdir -p $$BUILD_TARGET;        \
>                 $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET install \
>                                 INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \
>                                 SRC_PATH=$(shell readlink -e $$(pwd)) \
>
>
> Andres, please feel free to test / write commit message and submit this
> one liner, but even with that the build for some targets fails for me.

Thank you Jakub, that solved this issue, I'll send a patch shortly.

> "make [..] install" seems wobbly.

Yes it is.

Cheers,
Anders

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Shuah Khan 2 months, 2 weeks ago

On 9/12/24 09:23, Jakub Kicinski wrote:
> On Thu, 12 Sep 2024 08:31:18 +0200 Anders Roxell wrote:
>> Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>> ---
>>   tools/testing/selftests/Makefile | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index 3b7df5477317..fc3681270afe 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -64,6 +64,7 @@ TARGETS += net
>>   TARGETS += net/af_unix
>>   TARGETS += net/forwarding
>>   TARGETS += net/hsr
>> +TARGETS += net/lib
>>   TARGETS += net/mptcp
>>   TARGETS += net/netfilter
>>   TARGETS += net/openvswitch
> 
> Please make sure you always include a commit message. Among other
> things writing one would force you to understand the code, and
> in this case understand that this target is intentionally left out.
> Look around the Makefile for references to net/lib, you'll figure
> it out.
> 

+1 - thank you for outlining the benefits of writing a change log
which includes the details.

This patch is missing the changelog completely - change log is
an important part of sending a patch.

> The patch is incorrect.

thanks,
-- Shuah

Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets

Posted by Willem de Bruijn 2 months, 2 weeks ago

On Thu, Sep 12, 2024 at 2:31 AM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

This target is automatically built for targets that depend on it.

See the commit that introduced it, b86761ff6374.

+++ b/tools/testing/selftests/Makefile
@@ -116,6 +116,13 @@ TARGETS += zram
 TARGETS_HOTPLUG = cpu-hotplug
 TARGETS_HOTPLUG += memory-hotplug

+# Networking tests want the net/lib target, include it automatically
+ifneq ($(filter net,$(TARGETS)),)
+ifeq ($(filter net/lib,$(TARGETS)),)
+       INSTALL_DEP_TARGETS := net/lib
+endif
+endif

If you believe that it needs to be included directly, please expand
the commit message with the reasoning.