2022-08-03 21:19:24

by Guillaume Tucker

[permalink] [raw]
Subject: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

Revert part of the earlier changes to fix the kselftest build when
using a sub-directory from the top of the tree as this broke the
landlock test build as a side-effect when building with "make -C
tools/testing/selftests/landlock".

Reported-by: Mickaël Salaün <[email protected]>
Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
Signed-off-by: Guillaume Tucker <[email protected]>
---
tools/testing/selftests/landlock/Makefile | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
index a6959df28eb0..02868ac3bc71 100644
--- a/tools/testing/selftests/landlock/Makefile
+++ b/tools/testing/selftests/landlock/Makefile
@@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
TEST_GEN_PROGS_EXTENDED := true

OVERRIDE_TARGETS := 1
+top_srcdir := ../../../..
include ../lib.mk

+khdr_dir = $(top_srcdir)/usr/include
+
$(OUTPUT)/true: true.c
$(LINK.c) $< $(LDLIBS) -o $@ -static

-$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
- $(LINK.c) $< $(LDLIBS) -o $@ -lcap
+$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
+ $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
--
2.30.2



2022-08-03 23:14:44

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

On 8/3/22 2:13 PM, Guillaume Tucker wrote:
> Revert part of the earlier changes to fix the kselftest build when
> using a sub-directory from the top of the tree as this broke the
> landlock test build as a side-effect when building with "make -C
> tools/testing/selftests/landlock".
>
> Reported-by: Mickaël Salaün <[email protected]>
> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> Signed-off-by: Guillaume Tucker <[email protected]>
> ---
> tools/testing/selftests/landlock/Makefile | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>

Thank you. Now applied to linux-kselftest next for the next pull request.

thanks,
-- Shuah

2022-08-04 10:51:13

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h


On 03/08/2022 22:13, Guillaume Tucker wrote:
> Revert part of the earlier changes to fix the kselftest build when
> using a sub-directory from the top of the tree as this broke the
> landlock test build as a side-effect when building with "make -C
> tools/testing/selftests/landlock".
>
> Reported-by: Mickaël Salaün <[email protected]>
> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> Signed-off-by: Guillaume Tucker <[email protected]>
> ---
> tools/testing/selftests/landlock/Makefile | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
> index a6959df28eb0..02868ac3bc71 100644
> --- a/tools/testing/selftests/landlock/Makefile
> +++ b/tools/testing/selftests/landlock/Makefile
> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
> TEST_GEN_PROGS_EXTENDED := true
>
> OVERRIDE_TARGETS := 1
> +top_srcdir := ../../../..

Not sure it changes much, but most other selftests Makefiles use
"top_srcdir = ../../../.." (without ":="). Why this change?


> include ../lib.mk
>
> +khdr_dir = $(top_srcdir)/usr/include
> +
> $(OUTPUT)/true: true.c
> $(LINK.c) $< $(LDLIBS) -o $@ -static
>
> -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
> - $(LINK.c) $< $(LDLIBS) -o $@ -lcap
> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
> + $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)

2022-08-04 19:45:11

by Guillaume Tucker

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

On 04/08/2022 12:36, Mickaël Salaün wrote:
>
> On 03/08/2022 22:13, Guillaume Tucker wrote:
>> Revert part of the earlier changes to fix the kselftest build when
>> using a sub-directory from the top of the tree as this broke the
>> landlock test build as a side-effect when building with "make -C
>> tools/testing/selftests/landlock".
>>
>> Reported-by: Mickaël Salaün <[email protected]>
>> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
>> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
>> Signed-off-by: Guillaume Tucker <[email protected]>
>> ---
>>   tools/testing/selftests/landlock/Makefile | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>> index a6959df28eb0..02868ac3bc71 100644
>> --- a/tools/testing/selftests/landlock/Makefile
>> +++ b/tools/testing/selftests/landlock/Makefile
>> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
>>   TEST_GEN_PROGS_EXTENDED := true
>>     OVERRIDE_TARGETS := 1
>> +top_srcdir := ../../../..
>
> Not sure it changes much, but most other selftests Makefiles use "top_srcdir = ../../../.." (without ":="). Why this change?

I didn't simply apply your diff but edited the file by hand to
test various combinations and see what side effects it might
have. So when I added top_srcdir I typed it by hand and used :=
as a reflex since it's the standard way of assigning variables.
Using = instead only makes a difference when the r-value has
something dynamic as it will be re-evaluated every time it's
used. So for constant values, I guess it's more of a question of
coding style and conventions. Maybe all the top_srcdir variables
should be changed to := but that's unnecessary churn... Either
way, it's benign.

Shuah, feel free to change this back to = in this particular case
if it's more consistent with other Makefiles. Consistency is
often better than arbitrary rules. Or conversely, change to :=
for the khdr_dir definition... Entirely up to you I think.

Thanks,
Guillaume

>>   include ../lib.mk
>>   +khdr_dir = $(top_srcdir)/usr/include
>> +
>>   $(OUTPUT)/true: true.c
>>       $(LINK.c) $< $(LDLIBS) -o $@ -static
>>   -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
>> -    $(LINK.c) $< $(LDLIBS) -o $@ -lcap
>> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
>> +    $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)


2022-08-05 17:30:30

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h


On 04/08/2022 21:38, Guillaume Tucker wrote:
> On 04/08/2022 12:36, Mickaël Salaün wrote:
>>
>> On 03/08/2022 22:13, Guillaume Tucker wrote:
>>> Revert part of the earlier changes to fix the kselftest build when
>>> using a sub-directory from the top of the tree as this broke the
>>> landlock test build as a side-effect when building with "make -C
>>> tools/testing/selftests/landlock".
>>>
>>> Reported-by: Mickaël Salaün <[email protected]>
>>> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
>>> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
>>> Signed-off-by: Guillaume Tucker <[email protected]>
>>> ---
>>>   tools/testing/selftests/landlock/Makefile | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>>> index a6959df28eb0..02868ac3bc71 100644
>>> --- a/tools/testing/selftests/landlock/Makefile
>>> +++ b/tools/testing/selftests/landlock/Makefile
>>> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
>>>   TEST_GEN_PROGS_EXTENDED := true
>>>     OVERRIDE_TARGETS := 1
>>> +top_srcdir := ../../../..
>>
>> Not sure it changes much, but most other selftests Makefiles use "top_srcdir = ../../../.." (without ":="). Why this change?
>
> I didn't simply apply your diff but edited the file by hand to
> test various combinations and see what side effects it might
> have. So when I added top_srcdir I typed it by hand and used :=
> as a reflex since it's the standard way of assigning variables.
> Using = instead only makes a difference when the r-value has
> something dynamic as it will be re-evaluated every time it's
> used. So for constant values, I guess it's more of a question of
> coding style and conventions. Maybe all the top_srcdir variables
> should be changed to := but that's unnecessary churn... Either
> way, it's benign.
>
> Shuah, feel free to change this back to = in this particular case
> if it's more consistent with other Makefiles. Consistency is
> often better than arbitrary rules. Or conversely, change to :=
> for the khdr_dir definition... Entirely up to you I think.

Looks good to me, thanks! Shuah, feel free to add
Signed-off-by: Mickaël Salaün <[email protected]>

>
> Thanks,
> Guillaume
>
>>>   include ../lib.mk
>>>   +khdr_dir = $(top_srcdir)/usr/include
>>> +
>>>   $(OUTPUT)/true: true.c
>>>       $(LINK.c) $< $(LDLIBS) -o $@ -static
>>>   -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
>>> -    $(LINK.c) $< $(LDLIBS) -o $@ -lcap
>>> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
>>> +    $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
>

2022-08-12 15:57:47

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

Shuah, do you plan to send a pull request before merge window closes?

On 05/08/2022 19:16, Mickaël Salaün wrote:
>
> On 04/08/2022 21:38, Guillaume Tucker wrote:
>> On 04/08/2022 12:36, Mickaël Salaün wrote:
>>>
>>> On 03/08/2022 22:13, Guillaume Tucker wrote:
>>>> Revert part of the earlier changes to fix the kselftest build when
>>>> using a sub-directory from the top of the tree as this broke the
>>>> landlock test build as a side-effect when building with "make -C
>>>> tools/testing/selftests/landlock".
>>>>
>>>> Reported-by: Mickaël Salaün <[email protected]>
>>>> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
>>>> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
>>>> Signed-off-by: Guillaume Tucker <[email protected]>
>>>> ---
>>>>   tools/testing/selftests/landlock/Makefile | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>>>> index a6959df28eb0..02868ac3bc71 100644
>>>> --- a/tools/testing/selftests/landlock/Makefile
>>>> +++ b/tools/testing/selftests/landlock/Makefile
>>>> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
>>>>   TEST_GEN_PROGS_EXTENDED := true
>>>>     OVERRIDE_TARGETS := 1
>>>> +top_srcdir := ../../../..
>>>
>>> Not sure it changes much, but most other selftests Makefiles use "top_srcdir = ../../../.." (without ":="). Why this change?
>>
>> I didn't simply apply your diff but edited the file by hand to
>> test various combinations and see what side effects it might
>> have. So when I added top_srcdir I typed it by hand and used :=
>> as a reflex since it's the standard way of assigning variables.
>> Using = instead only makes a difference when the r-value has
>> something dynamic as it will be re-evaluated every time it's
>> used. So for constant values, I guess it's more of a question of
>> coding style and conventions. Maybe all the top_srcdir variables
>> should be changed to := but that's unnecessary churn... Either
>> way, it's benign.
>>
>> Shuah, feel free to change this back to = in this particular case
>> if it's more consistent with other Makefiles. Consistency is
>> often better than arbitrary rules. Or conversely, change to :=
>> for the khdr_dir definition... Entirely up to you I think.
>
> Looks good to me, thanks! Shuah, feel free to add
> Signed-off-by: Mickaël Salaün <[email protected]>
>
>>
>> Thanks,
>> Guillaume
>>
>>>>   include ../lib.mk
>>>>   +khdr_dir = $(top_srcdir)/usr/include
>>>> +
>>>>   $(OUTPUT)/true: true.c
>>>>       $(LINK.c) $< $(LDLIBS) -o $@ -static
>>>>   -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
>>>> -    $(LINK.c) $< $(LDLIBS) -o $@ -lcap
>>>> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
>>>> +    $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
>>

2022-08-13 10:12:46

by Anders Roxell

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

On Wed, 3 Aug 2022 at 22:14, Guillaume Tucker
<[email protected]> wrote:
>
> Revert part of the earlier changes to fix the kselftest build when
> using a sub-directory from the top of the tree as this broke the
> landlock test build as a side-effect when building with "make -C
> tools/testing/selftests/landlock".
>
> Reported-by: Mickaël Salaün <[email protected]>
> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> Signed-off-by: Guillaume Tucker <[email protected]>

Building with this patch doesn't work, it gives this output:
make[3]: Entering directory
'/home/anders/src/kernel/next/tools/testing/selftests/landlock'
make[3]: Leaving directory
'/home/anders/src/kernel/next/tools/testing/selftests/landlock'
make[3]: *** No rule to make target
'/home/anders/.cache/tuxmake/builds/78/build/kselftest/landlock/base_test',
needed by 'all'. Stop.

I'm building like this:
tuxmake --runtime podman --target-arch x86_64 --toolchain gcc-12
--kconfig defconfig kselftest

which translates into this make command:
make --silent --keep-going --jobs=32
O=/home/anders/.cache/tuxmake/builds/78/build
INSTALL_PATH=/home/anders/.cache/tuxmake/builds/78/build/kselftest_install
ARCH=x86_64 CROSS_COMPILE=x86_64-linux-gnu- kselftest-install

building without this patch works, see below:

make[3]: Entering directory
'/home/anders/src/kernel/next/tools/testing/selftests/landlock'
x86_64-linux-gnu-gcc -Wall -O2 -isystem
/home/anders/.cache/tuxmake/builds/77/build/usr/include base_test.c
-o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/base_test
-lcap
x86_64-linux-gnu-gcc -Wall -O2 -isystem
/home/anders/.cache/tuxmake/builds/77/build/usr/include fs_test.c
-o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/fs_test
-lcap
x86_64-linux-gnu-gcc -Wall -O2 -isystem
/home/anders/.cache/tuxmake/builds/77/build/usr/include
ptrace_test.c -o
/home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/ptrace_test
-lcap
x86_64-linux-gnu-gcc -Wall -O2 -isystem
/home/anders/.cache/tuxmake/builds/77/build/usr/include true.c -o
/home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/true
-static
make[3]: Leaving directory
'/home/anders/src/kernel/next/tools/testing/selftests/landlock'

Cheers,
Anders

> ---
> tools/testing/selftests/landlock/Makefile | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
> index a6959df28eb0..02868ac3bc71 100644
> --- a/tools/testing/selftests/landlock/Makefile
> +++ b/tools/testing/selftests/landlock/Makefile
> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
> TEST_GEN_PROGS_EXTENDED := true
>
> OVERRIDE_TARGETS := 1
> +top_srcdir := ../../../..
> include ../lib.mk
>
> +khdr_dir = $(top_srcdir)/usr/include
> +
> $(OUTPUT)/true: true.c
> $(LINK.c) $< $(LDLIBS) -o $@ -static
>
> -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
> - $(LINK.c) $< $(LDLIBS) -o $@ -lcap
> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
> + $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
> --
> 2.30.2
>

2022-08-13 12:56:25

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h


On 13/08/2022 12:01, Anders Roxell wrote:
> On Wed, 3 Aug 2022 at 22:14, Guillaume Tucker
> <[email protected]> wrote:
>>
>> Revert part of the earlier changes to fix the kselftest build when
>> using a sub-directory from the top of the tree as this broke the
>> landlock test build as a side-effect when building with "make -C
>> tools/testing/selftests/landlock".
>>
>> Reported-by: Mickaël Salaün <[email protected]>
>> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
>> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
>> Signed-off-by: Guillaume Tucker <[email protected]>
>
> Building with this patch doesn't work, it gives this output:
> make[3]: Entering directory
> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> make[3]: Leaving directory
> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> make[3]: *** No rule to make target
> '/home/anders/.cache/tuxmake/builds/78/build/kselftest/landlock/base_test',
> needed by 'all'. Stop.
>
> I'm building like this:
> tuxmake --runtime podman --target-arch x86_64 --toolchain gcc-12
> --kconfig defconfig kselftest
>
> which translates into this make command:
> make --silent --keep-going --jobs=32
> O=/home/anders/.cache/tuxmake/builds/78/build
> INSTALL_PATH=/home/anders/.cache/tuxmake/builds/78/build/kselftest_install
> ARCH=x86_64 CROSS_COMPILE=x86_64-linux-gnu- kselftest-install

This works well for me. Which commit is checkout?


>
> building without this patch works, see below:
>
> make[3]: Entering directory
> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> x86_64-linux-gnu-gcc -Wall -O2 -isystem
> /home/anders/.cache/tuxmake/builds/77/build/usr/include base_test.c
> -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/base_test
> -lcap
> x86_64-linux-gnu-gcc -Wall -O2 -isystem
> /home/anders/.cache/tuxmake/builds/77/build/usr/include fs_test.c
> -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/fs_test
> -lcap
> x86_64-linux-gnu-gcc -Wall -O2 -isystem
> /home/anders/.cache/tuxmake/builds/77/build/usr/include
> ptrace_test.c -o
> /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/ptrace_test
> -lcap
> x86_64-linux-gnu-gcc -Wall -O2 -isystem
> /home/anders/.cache/tuxmake/builds/77/build/usr/include true.c -o
> /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/true
> -static
> make[3]: Leaving directory
> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
Does this work if you revert this patch, commit a917dd94b832
("selftests/landlock: drop deprecated headers dependency") and commit
f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")?

This patch mainly revert commit a917dd94b832, so I don't see the issue.


>
> Cheers,
> Anders
>
>> ---
>> tools/testing/selftests/landlock/Makefile | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>> index a6959df28eb0..02868ac3bc71 100644
>> --- a/tools/testing/selftests/landlock/Makefile
>> +++ b/tools/testing/selftests/landlock/Makefile
>> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
>> TEST_GEN_PROGS_EXTENDED := true
>>
>> OVERRIDE_TARGETS := 1
>> +top_srcdir := ../../../..
>> include ../lib.mk
>>
>> +khdr_dir = $(top_srcdir)/usr/include
>> +
>> $(OUTPUT)/true: true.c
>> $(LINK.c) $< $(LDLIBS) -o $@ -static
>>
>> -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
>> - $(LINK.c) $< $(LDLIBS) -o $@ -lcap
>> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
>> + $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
>> --
>> 2.30.2
>>

2022-08-15 17:47:40

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

On 8/12/22 9:29 AM, Mickaël Salaün wrote:
> Shuah, do you plan to send a pull request before merge window closes?
>

I was hoping to do so and missed the window. Looks like more discussion
happening on this change. I will get this in for the next rc

thanks,
-- Shuah

2022-08-16 17:24:34

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h



On 15/08/2022 19:17, Shuah Khan wrote:
> On 8/12/22 9:29 AM, Mickaël Salaün wrote:
>> Shuah, do you plan to send a pull request before merge window closes?
>>
>
> I was hoping to do so and missed the window. Looks like more discussion
> happening on this change. I will get this in for the next rc

Yes please, this is a regression fix. I cannot reproduce the issue
reported by Anders Roxell.

2022-08-22 14:43:37

by Anders Roxell

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h

On Sat, 13 Aug 2022 at 14:31, Mickaël Salaün <[email protected]> wrote:
>
>
> On 13/08/2022 12:01, Anders Roxell wrote:
> > On Wed, 3 Aug 2022 at 22:14, Guillaume Tucker
> > <[email protected]> wrote:
> >>
> >> Revert part of the earlier changes to fix the kselftest build when
> >> using a sub-directory from the top of the tree as this broke the
> >> landlock test build as a side-effect when building with "make -C
> >> tools/testing/selftests/landlock".
> >>
> >> Reported-by: Mickaël Salaün <[email protected]>
> >> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
> >> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> >> Signed-off-by: Guillaume Tucker <[email protected]>
> >
> > Building with this patch doesn't work, it gives this output:
> > make[3]: Entering directory
> > '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> > make[3]: Leaving directory
> > '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> > make[3]: *** No rule to make target
> > '/home/anders/.cache/tuxmake/builds/78/build/kselftest/landlock/base_test',
> > needed by 'all'. Stop.
> >
> > I'm building like this:
> > tuxmake --runtime podman --target-arch x86_64 --toolchain gcc-12
> > --kconfig defconfig kselftest
> >
> > which translates into this make command:
> > make --silent --keep-going --jobs=32
> > O=/home/anders/.cache/tuxmake/builds/78/build
> > INSTALL_PATH=/home/anders/.cache/tuxmake/builds/78/build/kselftest_install
> > ARCH=x86_64 CROSS_COMPILE=x86_64-linux-gnu- kselftest-install
>
> This works well for me.

Interesting

> Which commit is checkout?

I used the latest next tag, I tried to on todays tag as well
next-20220822 and I see
the same issue.
building without 'O=...' I can build the landlock tests...

>
>
> >
> > building without this patch works, see below:
> >
> > make[3]: Entering directory
> > '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> > x86_64-linux-gnu-gcc -Wall -O2 -isystem
> > /home/anders/.cache/tuxmake/builds/77/build/usr/include base_test.c
> > -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/base_test
> > -lcap
> > x86_64-linux-gnu-gcc -Wall -O2 -isystem
> > /home/anders/.cache/tuxmake/builds/77/build/usr/include fs_test.c
> > -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/fs_test
> > -lcap
> > x86_64-linux-gnu-gcc -Wall -O2 -isystem
> > /home/anders/.cache/tuxmake/builds/77/build/usr/include
> > ptrace_test.c -o
> > /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/ptrace_test
> > -lcap
> > x86_64-linux-gnu-gcc -Wall -O2 -isystem
> > /home/anders/.cache/tuxmake/builds/77/build/usr/include true.c -o
> > /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/true
> > -static
> > make[3]: Leaving directory
> > '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
> Does this work if you revert this patch, commit a917dd94b832
> ("selftests/landlock: drop deprecated headers dependency") and commit
> f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")?
>
> This patch mainly revert commit a917dd94b832, so I don't see the issue.
>
>
> >
> > Cheers,
> > Anders
> >
> >> ---
> >> tools/testing/selftests/landlock/Makefile | 7 +++++--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
> >> index a6959df28eb0..02868ac3bc71 100644
> >> --- a/tools/testing/selftests/landlock/Makefile
> >> +++ b/tools/testing/selftests/landlock/Makefile
> >> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
> >> TEST_GEN_PROGS_EXTENDED := true
> >>
> >> OVERRIDE_TARGETS := 1
> >> +top_srcdir := ../../../..
> >> include ../lib.mk
> >>
> >> +khdr_dir = $(top_srcdir)/usr/include
> >> +
> >> $(OUTPUT)/true: true.c
> >> $(LINK.c) $< $(LDLIBS) -o $@ -static
> >>
> >> -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
> >> - $(LINK.c) $< $(LDLIBS) -o $@ -lcap
> >> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
> >> + $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
> >> --
> >> 2.30.2
> >>

2022-08-25 09:37:56

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH] selftests/landlock: fix broken include of linux/landlock.h


On 22/08/2022 16:00, Anders Roxell wrote:
> On Sat, 13 Aug 2022 at 14:31, Mickaël Salaün <[email protected]> wrote:
>>
>>
>> On 13/08/2022 12:01, Anders Roxell wrote:
>>> On Wed, 3 Aug 2022 at 22:14, Guillaume Tucker
>>> <[email protected]> wrote:
>>>>
>>>> Revert part of the earlier changes to fix the kselftest build when
>>>> using a sub-directory from the top of the tree as this broke the
>>>> landlock test build as a side-effect when building with "make -C
>>>> tools/testing/selftests/landlock".
>>>>
>>>> Reported-by: Mickaël Salaün <[email protected]>
>>>> Fixes: a917dd94b832 ("selftests/landlock: drop deprecated headers dependency")
>>>> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
>>>> Signed-off-by: Guillaume Tucker <[email protected]>
>>>
>>> Building with this patch doesn't work, it gives this output:
>>> make[3]: Entering directory
>>> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
>>> make[3]: Leaving directory
>>> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
>>> make[3]: *** No rule to make target
>>> '/home/anders/.cache/tuxmake/builds/78/build/kselftest/landlock/base_test',
>>> needed by 'all'. Stop.
>>>
>>> I'm building like this:
>>> tuxmake --runtime podman --target-arch x86_64 --toolchain gcc-12
>>> --kconfig defconfig kselftest
>>>
>>> which translates into this make command:
>>> make --silent --keep-going --jobs=32
>>> O=/home/anders/.cache/tuxmake/builds/78/build
>>> INSTALL_PATH=/home/anders/.cache/tuxmake/builds/78/build/kselftest_install
>>> ARCH=x86_64 CROSS_COMPILE=x86_64-linux-gnu- kselftest-install
>>
>> This works well for me.
>
> Interesting

I used this command (inspired by yours):

make --silent --keep-going --jobs=32 "O=${HOME}/build"
"INSTALL_PATH=${HOME}/build/kselftest_install" ARCH=x86_64
CROSS_COMPILE=x86_64-linux-gnu- kselftest-install

Can you run this command without using tuxmake?


>
>> Which commit is checkout?
>
> I used the latest next tag, I tried to on todays tag as well
> next-20220822 and I see
> the same issue.
> building without 'O=...' I can build the landlock tests...

Can you test it with Linux v5.19 and v6.0-rc2 and see if there is a
difference?

Is your workspace clean?
What is the version of your make?

Can you replace this line from the Makefile with static names?
"src_test := $(wildcard *_test.c)"



>
>>
>>
>>>
>>> building without this patch works, see below:
>>>
>>> make[3]: Entering directory
>>> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
>>> x86_64-linux-gnu-gcc -Wall -O2 -isystem
>>> /home/anders/.cache/tuxmake/builds/77/build/usr/include base_test.c
>>> -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/base_test
>>> -lcap
>>> x86_64-linux-gnu-gcc -Wall -O2 -isystem
>>> /home/anders/.cache/tuxmake/builds/77/build/usr/include fs_test.c
>>> -o /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/fs_test
>>> -lcap
>>> x86_64-linux-gnu-gcc -Wall -O2 -isystem
>>> /home/anders/.cache/tuxmake/builds/77/build/usr/include
>>> ptrace_test.c -o
>>> /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/ptrace_test
>>> -lcap
>>> x86_64-linux-gnu-gcc -Wall -O2 -isystem
>>> /home/anders/.cache/tuxmake/builds/77/build/usr/include true.c -o
>>> /home/anders/.cache/tuxmake/builds/77/build/kselftest/landlock/true
>>> -static
>>> make[3]: Leaving directory
>>> '/home/anders/src/kernel/next/tools/testing/selftests/landlock'
>> Does this work if you revert this patch, commit a917dd94b832
>> ("selftests/landlock: drop deprecated headers dependency") and commit
>> f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")?
>>
>> This patch mainly revert commit a917dd94b832, so I don't see the issue.
>>
>>
>>>
>>> Cheers,
>>> Anders
>>>
>>>> ---
>>>> tools/testing/selftests/landlock/Makefile | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>>>> index a6959df28eb0..02868ac3bc71 100644
>>>> --- a/tools/testing/selftests/landlock/Makefile
>>>> +++ b/tools/testing/selftests/landlock/Makefile
>>>> @@ -9,10 +9,13 @@ TEST_GEN_PROGS := $(src_test:.c=)
>>>> TEST_GEN_PROGS_EXTENDED := true
>>>>
>>>> OVERRIDE_TARGETS := 1
>>>> +top_srcdir := ../../../..
>>>> include ../lib.mk
>>>>
>>>> +khdr_dir = $(top_srcdir)/usr/include
>>>> +
>>>> $(OUTPUT)/true: true.c
>>>> $(LINK.c) $< $(LDLIBS) -o $@ -static
>>>>
>>>> -$(OUTPUT)/%_test: %_test.c ../kselftest_harness.h common.h
>>>> - $(LINK.c) $< $(LDLIBS) -o $@ -lcap
>>>> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/linux/landlock.h ../kselftest_harness.h common.h
>>>> + $(LINK.c) $< $(LDLIBS) -o $@ -lcap -I$(khdr_dir)
>>>> --
>>>> 2.30.2
>>>>