diff mbox series

[v3,11/11] selftests: error out if kernel header files are not yet built

Message ID 20230606071637.267103-12-jhubbard@nvidia.com
State New
Headers
Series A minor flurry of selftest/mm fixes |

Commit Message

John Hubbard June 6, 2023, 7:16 a.m. UTC 
  As per a discussion with Muhammad Usama Anjum [1], the following is how
one is supposed to build selftests:

    make headers && make -C tools/testing/selftests/mm

Change the selftest build system's lib.mk to fail out with a helpful
message if that prerequisite "make headers" has not been done yet.

[1] https://lore.kernel.org/all/bf910fa5-0c96-3707-cce4-5bcc656b6274@collabora.com/

Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/lib.mk | 36 +++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

Comments

Muhammad Usama Anjum June 6, 2023, 7:38 a.m. UTC | #1 
On 6/6/23 12:16 PM, John Hubbard wrote:
> As per a discussion with Muhammad Usama Anjum [1], the following is how
> one is supposed to build selftests:
> 
>     make headers && make -C tools/testing/selftests/mm
> 
> Change the selftest build system's lib.mk to fail out with a helpful
> message if that prerequisite "make headers" has not been done yet.
> 
> [1] https://lore.kernel.org/all/bf910fa5-0c96-3707-cce4-5bcc656b6274@collabora.com/
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/lib.mk | 36 +++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 05400462c779..b8ea03b9a015 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -44,10 +44,22 @@ endif
>  selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
>  top_srcdir = $(selfdir)/../../..
>  
> -ifeq ($(KHDR_INCLUDES),)
> -KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
> +ifneq ($(KBUILD_OUTPUT),)
> +  # Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
> +  # expand a shell special character '~'. We use a somewhat tedious way here.
> +  abs_objtree := $(shell cd $(top_srcdir) && mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
> +  $(if $(abs_objtree),, \
> +    $(error failed to create output directory "$(KBUILD_OUTPUT)"))
> +  # $(realpath ...) resolves symlinks
> +  abs_objtree := $(realpath $(abs_objtree))
> +  KHDR_DIR := ${abs_objtree}/usr/include
> +else
> +  abs_srctree := $(shell cd $(top_srcdir) && pwd)
> +  KHDR_DIR := ${abs_srctree}/usr/include
>  endif
>  
> +KHDR_INCLUDES := -isystem $(KHDR_DIR)
> +
>  # The following are built by lib.mk common compile rules.
>  # TEST_CUSTOM_PROGS should be used by tests that require
>  # custom build rule and prevent common build rule use.
> @@ -58,7 +70,25 @@ TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
>  TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
>  TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
>  
> -all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
> +all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
> +     $(TEST_GEN_FILES)
> +
> +kernel_header_files:
> +	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
> +	if [ $$? -ne 0 ]; then                                                 \
> +            RED='\033[1;31m';                                                  \
> +            NOCOLOR='\033[0m';                                                 \
> +            echo;                                                              \
> +            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
> +            echo "Please run this and try again:";                             \
> +            echo;                                                              \
> +            echo "    cd $(top_srcdir)";                                       \
> +            echo "    make headers";                                           \
> +            echo;                                                              \
> +	    exit 1; \
> +	fi
Thank you for adding this. This is outputting error for every selftest
directory. We should try to make it even better by just aborting the
Make-ing process the first time headers aren't detected. We can do this now
or later, fine by me.


make[1]: Entering directory
'/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/futex'

-e error: missing kernel header files.
Please run this and try again:

    cd /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/../../..
    make headers

make[1]: *** [../lib.mk:77: kernel_header_files] Error 1
make[1]: Leaving directory
'/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/futex'
make[1]: Entering directory
'/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/gpio'

-e error: missing kernel header files.
Please run this and try again:

    cd /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/../../..
    make headers

make[1]: *** [../lib.mk:77: kernel_header_files] Error 1
make[1]: Leaving directory
'/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/gpio'
m

Complete error log file is attached.


> +
> +.PHONY: kernel_header_files
>  
>  define RUN_TESTS
>  	BASE_DIR="$(selfdir)";			\
Muhammad Usama Anjum June 6, 2023, 7:57 a.m. UTC | #2 
On 6/6/23 12:16 PM, John Hubbard wrote:
> As per a discussion with Muhammad Usama Anjum [1], the following is how
> one is supposed to build selftests:
> 
>     make headers && make -C tools/testing/selftests/mm
> 
> Change the selftest build system's lib.mk to fail out with a helpful
> message if that prerequisite "make headers" has not been done yet.
> 
> [1] https://lore.kernel.org/all/bf910fa5-0c96-3707-cce4-5bcc656b6274@collabora.com/
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> ---
>  tools/testing/selftests/lib.mk | 36 +++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 05400462c779..b8ea03b9a015 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -44,10 +44,22 @@ endif
>  selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
>  top_srcdir = $(selfdir)/../../..
>  
> -ifeq ($(KHDR_INCLUDES),)
> -KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
> +ifneq ($(KBUILD_OUTPUT),)
> +  # Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
> +  # expand a shell special character '~'. We use a somewhat tedious way here.
> +  abs_objtree := $(shell cd $(top_srcdir) && mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
> +  $(if $(abs_objtree),, \
> +    $(error failed to create output directory "$(KBUILD_OUTPUT)"))
> +  # $(realpath ...) resolves symlinks
> +  abs_objtree := $(realpath $(abs_objtree))
> +  KHDR_DIR := ${abs_objtree}/usr/include
> +else
> +  abs_srctree := $(shell cd $(top_srcdir) && pwd)
> +  KHDR_DIR := ${abs_srctree}/usr/include
>  endif
>  
> +KHDR_INCLUDES := -isystem $(KHDR_DIR)
> +
>  # The following are built by lib.mk common compile rules.
>  # TEST_CUSTOM_PROGS should be used by tests that require
>  # custom build rule and prevent common build rule use.
> @@ -58,7 +70,25 @@ TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
>  TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
>  TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
>  
> -all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
> +all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
> +     $(TEST_GEN_FILES)
> +
> +kernel_header_files:
> +	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
> +	if [ $$? -ne 0 ]; then                                                 \
> +            RED='\033[1;31m';                                                  \
> +            NOCOLOR='\033[0m';                                                 \
> +            echo;                                                              \
> +            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
> +            echo "Please run this and try again:";                             \
> +            echo;                                                              \
> +            echo "    cd $(top_srcdir)";                                       \
> +            echo "    make headers";                                           \
> +            echo;                                                              \
> +	    exit 1; \
> +	fi
> +
> +.PHONY: kernel_header_files
>  
>  define RUN_TESTS
>  	BASE_DIR="$(selfdir)";			\
John Hubbard June 6, 2023, 8:10 p.m. UTC | #3 
On 6/6/23 00:38, Muhammad Usama Anjum wrote:
...
>> +kernel_header_files:
>> +	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
>> +	if [ $$? -ne 0 ]; then                                                 \
>> +            RED='\033[1;31m';                                                  \
>> +            NOCOLOR='\033[0m';                                                 \
>> +            echo;                                                              \
>> +            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
>> +            echo "Please run this and try again:";                             \
>> +            echo;                                                              \
>> +            echo "    cd $(top_srcdir)";                                       \
>> +            echo "    make headers";                                           \
>> +            echo;                                                              \
>> +	    exit 1; \
>> +	fi
> Thank you for adding this. This is outputting error for every selftest
> directory. We should try to make it even better by just aborting the
> Make-ing process the first time headers aren't detected. We can do this now
> or later, fine by me.
> 
OK, I see. Yes, this can be improved by adding the same mechanism to the 
selftests/Makefile, that is in selftests/mm/Makefile.

I'd like to keep both, because as I mentioned earlier, mm folks like to
run just that one Makefile, sometimes, and selftests/mm/Makefile is not
invoking the top level Makefile. Rather, it includes lib.mk--which the
top level Makefile does *not* include.

Arguably, using includes instead of recursive Make, would improve this
framework: reduce duplication such as the above. But that's a larger
project and just food for thought at this point.

Anyway, this works nicely on my system, and I'll attach it as a patch
also in case you want to try it out. What do you think of this:

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 90a62cf75008..bdca160063d8 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -144,10 +144,12 @@ ifneq ($(KBUILD_OUTPUT),)
   abs_objtree := $(realpath $(abs_objtree))
   BUILD := $(abs_objtree)/kselftest
   KHDR_INCLUDES := -isystem ${abs_objtree}/usr/include
+  KHDR_DIR := ${abs_objtree}/usr/include
 else
   BUILD := $(CURDIR)
   abs_srctree := $(shell cd $(top_srcdir) && pwd)
   KHDR_INCLUDES := -isystem ${abs_srctree}/usr/include
+  KHDR_DIR := ${abs_srctree}/usr/include
   DEFAULT_INSTALL_HDR_PATH := 1
 endif
 
@@ -161,7 +163,7 @@ export KHDR_INCLUDES
 # all isn't the first target in the file.
 .DEFAULT_GOAL := all
 
-all:
+all: kernel_header_files
 	@ret=1;							\
 	for TARGET in $(TARGETS); do				\
 		BUILD_TARGET=$$BUILD/$$TARGET;			\
@@ -172,6 +174,23 @@ all:
 		ret=$$((ret * $$?));				\
 	done; exit $$ret;
 
+kernel_header_files:
+	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                          \
+	if [ $$? -ne 0 ]; then                                                     \
+            RED='\033[1;31m';                                                  \
+            NOCOLOR='\033[0m';                                                 \
+            echo;                                                              \
+            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
+            echo "Please run this and try again:";                             \
+            echo;                                                              \
+            echo "    cd $(top_srcdir)";                                       \
+            echo "    make headers";                                           \
+            echo;                                                              \
+	    exit 1;                                                                \
+	fi
+
+.PHONY: kernel_header_files
+
 run_tests: all
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\



thanks,
Muhammad Usama Anjum June 7, 2023, 5:37 a.m. UTC | #4 
On 6/7/23 1:10 AM, John Hubbard wrote:
> On 6/6/23 00:38, Muhammad Usama Anjum wrote:
> ...
>>> +kernel_header_files:
>>> +	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
>>> +	if [ $$? -ne 0 ]; then                                                 \
>>> +            RED='\033[1;31m';                                                  \
>>> +            NOCOLOR='\033[0m';                                                 \
>>> +            echo;                                                              \
>>> +            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
>>> +            echo "Please run this and try again:";                             \
>>> +            echo;                                                              \
>>> +            echo "    cd $(top_srcdir)";                                       \
>>> +            echo "    make headers";                                           \
>>> +            echo;                                                              \
>>> +	    exit 1; \
>>> +	fi
>> Thank you for adding this. This is outputting error for every selftest
>> directory. We should try to make it even better by just aborting the
>> Make-ing process the first time headers aren't detected. We can do this now
>> or later, fine by me.
>>
> OK, I see. Yes, this can be improved by adding the same mechanism to the 
> selftests/Makefile, that is in selftests/mm/Makefile.
> 
> I'd like to keep both, because as I mentioned earlier, mm folks like to
> run just that one Makefile, sometimes, and selftests/mm/Makefile is not
> invoking the top level Makefile. Rather, it includes lib.mk--which the
> top level Makefile does *not* include.
> 
> Arguably, using includes instead of recursive Make, would improve this
> framework: reduce duplication such as the above. But that's a larger
> project and just food for thought at this point.
> 
> Anyway, this works nicely on my system, and I'll attach it as a patch
> also in case you want to try it out. What do you think of this:
Nice patch. Thanks. Lets add this patch as well. Please add the tag for
this new patch:

Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 90a62cf75008..bdca160063d8 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -144,10 +144,12 @@ ifneq ($(KBUILD_OUTPUT),)
>    abs_objtree := $(realpath $(abs_objtree))
>    BUILD := $(abs_objtree)/kselftest
>    KHDR_INCLUDES := -isystem ${abs_objtree}/usr/include
> +  KHDR_DIR := ${abs_objtree}/usr/include
>  else
>    BUILD := $(CURDIR)
>    abs_srctree := $(shell cd $(top_srcdir) && pwd)
>    KHDR_INCLUDES := -isystem ${abs_srctree}/usr/include
> +  KHDR_DIR := ${abs_srctree}/usr/include
>    DEFAULT_INSTALL_HDR_PATH := 1
>  endif
>  
> @@ -161,7 +163,7 @@ export KHDR_INCLUDES
>  # all isn't the first target in the file.
>  .DEFAULT_GOAL := all
>  
> -all:
> +all: kernel_header_files
>  	@ret=1;							\
>  	for TARGET in $(TARGETS); do				\
>  		BUILD_TARGET=$$BUILD/$$TARGET;			\
> @@ -172,6 +174,23 @@ all:
>  		ret=$$((ret * $$?));				\
>  	done; exit $$ret;
>  
> +kernel_header_files:
> +	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                          \
> +	if [ $$? -ne 0 ]; then                                                     \
> +            RED='\033[1;31m';                                                  \
> +            NOCOLOR='\033[0m';                                                 \
> +            echo;                                                              \
> +            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
> +            echo "Please run this and try again:";                             \
> +            echo;                                                              \
> +            echo "    cd $(top_srcdir)";                                       \
> +            echo "    make headers";                                           \
> +            echo;                                                              \
> +	    exit 1;                                                                \
> +	fi
> +
> +.PHONY: kernel_header_files
> +
>  run_tests: all
>  	@for TARGET in $(TARGETS); do \
>  		BUILD_TARGET=$$BUILD/$$TARGET;	\
> 
> 
> 
> thanks,
Peter Zijlstra Nov. 3, 2023, 12:16 p.m. UTC | #5 
On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
> As per a discussion with Muhammad Usama Anjum [1], the following is how
> one is supposed to build selftests:
> 
>     make headers && make -C tools/testing/selftests/mm
> 
> Change the selftest build system's lib.mk to fail out with a helpful
> message if that prerequisite "make headers" has not been done yet.
> 

NAK NAK NAK

This now means I can no longer run selftests, I thank you very much! :-/

root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
***
*** The source tree is not clean, please run 'make mrproper'
*** in /usr/src/linux-2.6


I've always done:

  cd tools/testing/selftests/x86; make

and that has always worked

Now I can't bloody well build *any* selftest or risk not being able to
do builds.
David Hildenbrand Nov. 3, 2023, 12:22 p.m. UTC | #6 
On 03.11.23 13:16, Peter Zijlstra wrote:
> On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
>> As per a discussion with Muhammad Usama Anjum [1], the following is how
>> one is supposed to build selftests:
>>
>>      make headers && make -C tools/testing/selftests/mm
>>
>> Change the selftest build system's lib.mk to fail out with a helpful
>> message if that prerequisite "make headers" has not been done yet.
>>
> 
> NAK NAK NAK
> 
> This now means I can no longer run selftests, I thank you very much! :-/
> 
> root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
> make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
> ***
> *** The source tree is not clean, please run 'make mrproper'
> *** in /usr/src/linux-2.6
> 
> 
> I've always done:
> 
>    cd tools/testing/selftests/x86; make
> 
> and that has always worked
> 
> Now I can't bloody well build *any* selftest or risk not being able to
> do builds.

This change landed in 6.5, no? And 6.6 was just released. Just curious 
why you notice that now.
Peter Zijlstra Nov. 3, 2023, 12:46 p.m. UTC | #7 
On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
> On 03.11.23 13:16, Peter Zijlstra wrote:
> > On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
> > > As per a discussion with Muhammad Usama Anjum [1], the following is how
> > > one is supposed to build selftests:
> > > 
> > >      make headers && make -C tools/testing/selftests/mm
> > > 
> > > Change the selftest build system's lib.mk to fail out with a helpful
> > > message if that prerequisite "make headers" has not been done yet.
> > > 
> > 
> > NAK NAK NAK
> > 
> > This now means I can no longer run selftests, I thank you very much! :-/
> > 
> > root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
> > make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
> > ***
> > *** The source tree is not clean, please run 'make mrproper'
> > *** in /usr/src/linux-2.6
> > 
> > 
> > I've always done:
> > 
> >    cd tools/testing/selftests/x86; make
> > 
> > and that has always worked
> > 
> > Now I can't bloody well build *any* selftest or risk not being able to
> > do builds.
> 
> This change landed in 6.5, no? And 6.6 was just released. Just curious why
> you notice that now.

Dunno, last time I edited the selftests and needed to recompile was a
few weeks ago.
David Hildenbrand Nov. 3, 2023, 12:59 p.m. UTC | #8 
On 03.11.23 13:46, Peter Zijlstra wrote:
> On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
>> On 03.11.23 13:16, Peter Zijlstra wrote:
>>> On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
>>>> As per a discussion with Muhammad Usama Anjum [1], the following is how
>>>> one is supposed to build selftests:
>>>>
>>>>       make headers && make -C tools/testing/selftests/mm
>>>>
>>>> Change the selftest build system's lib.mk to fail out with a helpful
>>>> message if that prerequisite "make headers" has not been done yet.
>>>>
>>>
>>> NAK NAK NAK
>>>
>>> This now means I can no longer run selftests, I thank you very much! :-/
>>>
>>> root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
>>> make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
>>> ***
>>> *** The source tree is not clean, please run 'make mrproper'
>>> *** in /usr/src/linux-2.6
>>>
>>>
>>> I've always done:
>>>
>>>     cd tools/testing/selftests/x86; make
>>>
>>> and that has always worked
>>>
>>> Now I can't bloody well build *any* selftest or risk not being able to
>>> do builds.
>>
>> This change landed in 6.5, no? And 6.6 was just released. Just curious why
>> you notice that now.
> 
> Dunno, last time I edited the selftests and needed to recompile was a
> few weeks ago.

Okay. the question is if your workflow can be easily adjusted, or if we 
can improve that header handling as a whole.

The problem I had with this recently: just because we did a "make 
headers" once in a git tree doesn't mean that it is still up-to-date.

So once some selftest changes showed up that require newer headers, 
building the selftests again fails without a hint that another round of 
"make headers" would be required.
David Hildenbrand Nov. 3, 2023, 1 p.m. UTC | #9 
On 03.11.23 13:59, David Hildenbrand wrote:
> On 03.11.23 13:46, Peter Zijlstra wrote:
>> On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
>>> On 03.11.23 13:16, Peter Zijlstra wrote:
>>>> On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
>>>>> As per a discussion with Muhammad Usama Anjum [1], the following is how
>>>>> one is supposed to build selftests:
>>>>>
>>>>>        make headers && make -C tools/testing/selftests/mm
>>>>>
>>>>> Change the selftest build system's lib.mk to fail out with a helpful
>>>>> message if that prerequisite "make headers" has not been done yet.
>>>>>
>>>>
>>>> NAK NAK NAK
>>>>
>>>> This now means I can no longer run selftests, I thank you very much! :-/
>>>>
>>>> root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
>>>> make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
>>>> ***
>>>> *** The source tree is not clean, please run 'make mrproper'
>>>> *** in /usr/src/linux-2.6
>>>>
>>>>
>>>> I've always done:
>>>>
>>>>      cd tools/testing/selftests/x86; make
>>>>
>>>> and that has always worked
>>>>
>>>> Now I can't bloody well build *any* selftest or risk not being able to
>>>> do builds.
>>>
>>> This change landed in 6.5, no? And 6.6 was just released. Just curious why
>>> you notice that now.
>>
>> Dunno, last time I edited the selftests and needed to recompile was a
>> few weeks ago.
> 
> Okay. the question is if your workflow can be easily adjusted, or if we
> can improve that header handling as a whole.
> 
> The problem I had with this recently: just because we did a "make
> headers" once in a git tree doesn't mean that it is still up-to-date.
> 
> So once some selftest changes showed up that require newer headers,
> building the selftests again fails without a hint that another round of
> "make headers" would be required.

To clarify: maybe some kind of a warning would be better, ideally that 
the headers might be outdated and that another "make headers" would be 
required in case there are any build errors.
Peter Zijlstra Nov. 3, 2023, 1:08 p.m. UTC | #10 
On Fri, Nov 03, 2023 at 01:59:28PM +0100, David Hildenbrand wrote:

> Okay. the question is if your workflow can be easily adjusted, or if we can
> improve that header handling as a whole.

So on IRC the following was suggested:

  make O=defconfig-build headers ; make O=defconfig-build -C tools/testing/selftests/x86

But that makes absolutely no sense to me; because the headers and
selftests are not .config dependent. Furthermore I don't want them in a
kernel build dir.

> The problem I had with this recently: just because we did a "make headers"
> once in a git tree doesn't mean that it is still up-to-date.
> 
> So once some selftest changes showed up that require newer headers, building
> the selftests again fails without a hint that another round of "make
> headers" would be required.

Yeah, so I've been adding #ifndef guards all over the place for decades
and that just works. You need it in normal userspace too.

This super reliance on the very latestesetst headers is just a total
pain.
Miroslav Benes Dec. 8, 2023, 12:44 p.m. UTC | #11 
Hi John, Muhammad,

On Tue, 6 Jun 2023, John Hubbard wrote:

> As per a discussion with Muhammad Usama Anjum [1], the following is how
> one is supposed to build selftests:
> 
>     make headers && make -C tools/testing/selftests/mm
> 
> Change the selftest build system's lib.mk to fail out with a helpful
> message if that prerequisite "make headers" has not been done yet.
> 
> [1] https://lore.kernel.org/all/bf910fa5-0c96-3707-cce4-5bcc656b6274@collabora.com/

could you, please, elaborate more on that one is supposed to build 
selftests with 'make headers'? Yes, Documentation/dev-tools/kselftest.rst 
mentions that because you might need headers but...

The common way how we test the kernel is to build the kernel, install it 
somewhere and run selftests on top. The sequence basically being "make 
rpm-pkg; rpm -ivh; cd tools/testing/selftest/livepatch/ in source tree; 
sudo make run_tests" (or a similar variation of the procedure). The point 
is that we want to test the running kernel with its respective environment 
installed in /lib/modules/`uname -r`/ (if needed). This way we can run 
newer selftests from the current mainline tree on older kernels among 
others.

The commit breaks the use case which worked for a long long time.

It also breaks what Marcos proposed for livepatch selftests in 
https://lore.kernel.org/all/20231031-send-lp-kselftests-v3-0-2b1655c2605f@suse.com/

I guess we can always work around it by letting subsystem selftests to 
override KHDR_DIR but I am not comfortable with the behaviour that your 
commit introduced in the first place to be honest.

Thank you,
Miroslav
Peter Zijlstra Dec. 8, 2023, 3:14 p.m. UTC | #12 
On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
> On 03.11.23 13:16, Peter Zijlstra wrote:
> > On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
> > > As per a discussion with Muhammad Usama Anjum [1], the following is how
> > > one is supposed to build selftests:
> > > 
> > >      make headers && make -C tools/testing/selftests/mm
> > > 
> > > Change the selftest build system's lib.mk to fail out with a helpful
> > > message if that prerequisite "make headers" has not been done yet.
> > > 
> > 
> > NAK NAK NAK
> > 
> > This now means I can no longer run selftests, I thank you very much! :-/
> > 
> > root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
> > make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
> > ***
> > *** The source tree is not clean, please run 'make mrproper'
> > *** in /usr/src/linux-2.6
> > 
> > 
> > I've always done:
> > 
> >    cd tools/testing/selftests/x86; make
> > 
> > and that has always worked
> > 
> > Now I can't bloody well build *any* selftest or risk not being able to
> > do builds.
> 
> This change landed in 6.5, no? And 6.6 was just released. Just curious why
> you notice that now.

And I hit it again (different box etc..)

Can we please get this garbage fixed already?
David Hildenbrand Dec. 8, 2023, 3:21 p.m. UTC | #13 
On 08.12.23 16:14, Peter Zijlstra wrote:
> On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
>> On 03.11.23 13:16, Peter Zijlstra wrote:
>>> On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
>>>> As per a discussion with Muhammad Usama Anjum [1], the following is how
>>>> one is supposed to build selftests:
>>>>
>>>>       make headers && make -C tools/testing/selftests/mm
>>>>
>>>> Change the selftest build system's lib.mk to fail out with a helpful
>>>> message if that prerequisite "make headers" has not been done yet.
>>>>
>>>
>>> NAK NAK NAK
>>>
>>> This now means I can no longer run selftests, I thank you very much! :-/
>>>
>>> root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
>>> make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
>>> ***
>>> *** The source tree is not clean, please run 'make mrproper'
>>> *** in /usr/src/linux-2.6
>>>
>>>
>>> I've always done:
>>>
>>>     cd tools/testing/selftests/x86; make
>>>
>>> and that has always worked
>>>
>>> Now I can't bloody well build *any* selftest or risk not being able to
>>> do builds.
>>
>> This change landed in 6.5, no? And 6.6 was just released. Just curious why
>> you notice that now.
> 
> And I hit it again (different box etc..)
> 
> Can we please get this garbage fixed already?

I'd suggest to either revert or turn into a warning.

@John?
John Hubbard Dec. 8, 2023, 8:29 p.m. UTC | #14 
On 12/8/23 07:21, David Hildenbrand wrote:
> On 08.12.23 16:14, Peter Zijlstra wrote:
>> On Fri, Nov 03, 2023 at 01:22:54PM +0100, David Hildenbrand wrote:
>>> On 03.11.23 13:16, Peter Zijlstra wrote:
>>>> On Tue, Jun 06, 2023 at 12:16:37AM -0700, John Hubbard wrote:
>>>>> As per a discussion with Muhammad Usama Anjum [1], the following is 
>>>>> how
>>>>> one is supposed to build selftests:
>>>>>
>>>>>       make headers && make -C tools/testing/selftests/mm
>>>>>
>>>>> Change the selftest build system's lib.mk to fail out with a helpful
>>>>> message if that prerequisite "make headers" has not been done yet.
>>>>>
>>>>
>>>> NAK NAK NAK
>>>>
>>>> This now means I can no longer run selftests, I thank you very much! 
>>>> :-/
>>>>
>>>> root@spr:/usr/src/linux-2.6# make O=defconfig-build/ -j64
>>>> make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
>>>> ***
>>>> *** The source tree is not clean, please run 'make mrproper'
>>>> *** in /usr/src/linux-2.6
>>>>
>>>>
>>>> I've always done:
>>>>
>>>>     cd tools/testing/selftests/x86; make
>>>>
>>>> and that has always worked
>>>>
>>>> Now I can't bloody well build *any* selftest or risk not being able to
>>>> do builds.
>>>
>>> This change landed in 6.5, no? And 6.6 was just released. Just 
>>> curious why
>>> you notice that now.
>>
>> And I hit it again (different box etc..)
>>
>> Can we please get this garbage fixed already?
> 
> I'd suggest to either revert or turn into a warning.

That would put us back into a half-broken sort of situation, though...
see below.

> 
> @John?
> 

I don't have a strong opinion about how this should be done, and in
fact I believed at the time that I was bringing the system into
compliance with what everyone wanted here. :)

There seem to be two conflicting visions:

a) The way it was (much) earlier: use ifdefs and defines to get by
without the latest kernel headers, or

b) Requiring recent kernel headers to build the various selftests.

Shuah, Peter, others: can we choose a direction please? Either
way will work, and I personally don't care which one we choose.


thanks,
Peter Zijlstra Dec. 8, 2023, 10:10 p.m. UTC | #15 
On Fri, Dec 08, 2023 at 12:29:37PM -0800, John Hubbard wrote:

> I don't have a strong opinion about how this should be done, and in
> fact I believed at the time that I was bringing the system into
> compliance with what everyone wanted here. :)
> 
> There seem to be two conflicting visions:
> 
> a) The way it was (much) earlier: use ifdefs and defines to get by
> without the latest kernel headers, or
> 
> b) Requiring recent kernel headers to build the various selftests.
> 
> Shuah, Peter, others: can we choose a direction please? Either
> way will work, and I personally don't care which one we choose.

So as David already argued, the current thing does not in fact help with
b. You just have to install once and the error goes away, then carry
that tree for a year and you're running old crap again.

My biggest beef with the whole thing is that I simply do not want to use
'make headers', it doesn't work for me.

I have a ton of output directories and I don't care to build tools into
the output dirs, in fact some of them flat out refuse to work that way
(bpf comes to mind).
John Hubbard Dec. 9, 2023, 1:39 a.m. UTC | #16 
On 12/8/23 14:10, Peter Zijlstra wrote:
> So as David already argued, the current thing does not in fact help with
> b. You just have to install once and the error goes away, then carry
> that tree for a year and you're running old crap again.
> 
> My biggest beef with the whole thing is that I simply do not want to use
> 'make headers', it doesn't work for me.
> 
> I have a ton of output directories and I don't care to build tools into
> the output dirs, in fact some of them flat out refuse to work that way
> (bpf comes to mind).

Going with that, then, I believe it is best to simply revert commit
9fc96c7c19df ("selftests: error out if kernel header files are not
yet built"). And then follow up with a series of (many) changes to
wean the various selftests off of the kernel headers.

I'll post the revert shortly.

thanks,
diff mbox series

Patch

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 05400462c779..b8ea03b9a015 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -44,10 +44,22 @@  endif
 selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
 top_srcdir = $(selfdir)/../../..
 
-ifeq ($(KHDR_INCLUDES),)
-KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
+ifneq ($(KBUILD_OUTPUT),)
+  # Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
+  # expand a shell special character '~'. We use a somewhat tedious way here.
+  abs_objtree := $(shell cd $(top_srcdir) && mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
+  $(if $(abs_objtree),, \
+    $(error failed to create output directory "$(KBUILD_OUTPUT)"))
+  # $(realpath ...) resolves symlinks
+  abs_objtree := $(realpath $(abs_objtree))
+  KHDR_DIR := ${abs_objtree}/usr/include
+else
+  abs_srctree := $(shell cd $(top_srcdir) && pwd)
+  KHDR_DIR := ${abs_srctree}/usr/include
 endif
 
+KHDR_INCLUDES := -isystem $(KHDR_DIR)
+
 # The following are built by lib.mk common compile rules.
 # TEST_CUSTOM_PROGS should be used by tests that require
 # custom build rule and prevent common build rule use.
@@ -58,7 +70,25 @@  TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
 TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
 TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
 
-all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
+all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
+     $(TEST_GEN_FILES)
+
+kernel_header_files:
+	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
+	if [ $$? -ne 0 ]; then                                                 \
+            RED='\033[1;31m';                                                  \
+            NOCOLOR='\033[0m';                                                 \
+            echo;                                                              \
+            echo -e "$${RED}error$${NOCOLOR}: missing kernel header files.";   \
+            echo "Please run this and try again:";                             \
+            echo;                                                              \
+            echo "    cd $(top_srcdir)";                                       \
+            echo "    make headers";                                           \
+            echo;                                                              \
+	    exit 1; \
+	fi
+
+.PHONY: kernel_header_files
 
 define RUN_TESTS
 	BASE_DIR="$(selfdir)";			\