diff mbox series

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

Message ID 20240912063119.1277322-1-anders.roxell@linaro.org
State New
Headers show
Series selftests: Makefile: add missing 'net/lib' to targets | expand

Commit Message

Anders Roxell Sept. 12, 2024, 6:31 a.m. UTC
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(+)

Comments

Jakub Kicinski Sept. 12, 2024, 3:23 p.m. UTC | #1
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.
Shuah Khan Sept. 12, 2024, 4:44 p.m. UTC | #2
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
Anders Roxell Sept. 15, 2024, 6:44 a.m. UTC | #3
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
Willem de Bruijn Sept. 15, 2024, 7:36 a.m. UTC | #4
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.
Jakub Kicinski Sept. 15, 2024, 2:46 p.m. UTC | #5
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.
Anders Roxell Sept. 16, 2024, 7:53 a.m. UTC | #6
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
diff mbox series

Patch

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