This reverts commit 9fc96c7c19df ("selftests: error out if kernel header
files are not yet built").
It turns out that requiring the kernel headers to be built as a
prerequisite to building selftests, does not work in many cases. For
example, Peter Zijlstra writes:
"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)." [1]
Therefore, stop erroring out on the selftests build. Additional patches
will be required in order to change over to not requiring the kernel
headers.
[1] https://lore.kernel.org/[email protected]
Cc: Anders Roxell <[email protected]>
Cc: Muhammad Usama Anjum <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Peter Xu <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Shuah Khan <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/Makefile | 21 +----------------
tools/testing/selftests/lib.mk | 40 +++-----------------------------
2 files changed, 4 insertions(+), 57 deletions(-)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 3b2061d1c1a5..8247a7c69c36 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -155,12 +155,10 @@ 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
@@ -174,7 +172,7 @@ export KHDR_INCLUDES
# all isn't the first target in the file.
.DEFAULT_GOAL := all
-all: kernel_header_files
+all:
@ret=1; \
for TARGET in $(TARGETS); do \
BUILD_TARGET=$$BUILD/$$TARGET; \
@@ -185,23 +183,6 @@ all: kernel_header_files
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; \
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 118e0964bda9..aa646e0661f3 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -44,26 +44,10 @@ endif
selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
top_srcdir = $(selfdir)/../../..
-ifeq ("$(origin O)", "command line")
- KBUILD_OUTPUT := $(O)
+ifeq ($(KHDR_INCLUDES),)
+KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
endif
-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.
@@ -74,25 +58,7 @@ 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: 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
+all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
define RUN_TESTS
BASE_DIR="$(selfdir)"; \
--
2.43.0
On 12/9/23 7:01 AM, John Hubbard wrote:
> This reverts commit 9fc96c7c19df ("selftests: error out if kernel header
> files are not yet built").
I don't think whole of this commit needs to be reverted. Lets leave the
warning message as it is and just remove the condition to abort the
compilation.
>
> It turns out that requiring the kernel headers to be built as a
> prerequisite to building selftests, does not work in many cases. For
> example, Peter Zijlstra writes:
>
> "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)." [1]
>
> Therefore, stop erroring out on the selftests build. Additional patches
> will be required in order to change over to not requiring the kernel
> headers.
>
> [1] https://lore.kernel.org/[email protected]
>
> Cc: Anders Roxell <[email protected]>
> Cc: Muhammad Usama Anjum <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Peter Xu <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> tools/testing/selftests/Makefile | 21 +----------------
> tools/testing/selftests/lib.mk | 40 +++-----------------------------
> 2 files changed, 4 insertions(+), 57 deletions(-)
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 3b2061d1c1a5..8247a7c69c36 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -155,12 +155,10 @@ 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
>
> @@ -174,7 +172,7 @@ export KHDR_INCLUDES
> # all isn't the first target in the file.
> .DEFAULT_GOAL := all
>
> -all: kernel_header_files
> +all:
> @ret=1; \
> for TARGET in $(TARGETS); do \
> BUILD_TARGET=$$BUILD/$$TARGET; \
> @@ -185,23 +183,6 @@ all: kernel_header_files
> 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; \
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 118e0964bda9..aa646e0661f3 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -44,26 +44,10 @@ endif
> selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
> top_srcdir = $(selfdir)/../../..
>
> -ifeq ("$(origin O)", "command line")
> - KBUILD_OUTPUT := $(O)
> +ifeq ($(KHDR_INCLUDES),)
> +KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
> endif
>
> -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.
> @@ -74,25 +58,7 @@ 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: 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
> +all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
>
> define RUN_TESTS
> BASE_DIR="$(selfdir)"; \
--
BR,
Muhammad Usama Anjum
On Mon, 2023-12-11 at 16:00 +0500, Muhammad Usama Anjum wrote:
> On 12/9/23 7:01 AM, John Hubbard wrote:
> > This reverts commit 9fc96c7c19df ("selftests: error out if kernel
> > header
> > files are not yet built").
> I don't think whole of this commit needs to be reverted. Lets leave
> the
> warning message as it is and just remove the condition to abort the
> compilation.
Reverting or just printing the warning would work for our testcases, as
long as it doesn't error out.
>
> >
> > It turns out that requiring the kernel headers to be built as a
> > prerequisite to building selftests, does not work in many cases.
> > For
> > example, Peter Zijlstra writes:
> >
> > "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)." [1]
> >
> > Therefore, stop erroring out on the selftests build. Additional
> > patches
> > will be required in order to change over to not requiring the
> > kernel
> > headers.
> >
> > [1]
> > https://lore.kernel.org/[email protected]
> >
> > Cc: Anders Roxell <[email protected]>
> > Cc: Muhammad Usama Anjum <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Peter Xu <[email protected]>
> > Cc: Jonathan Corbet <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>
> > Cc: Shuah Khan <[email protected]>
> > Signed-off-by: John Hubbard <[email protected]>
> > ---
> > tools/testing/selftests/Makefile | 21 +----------------
> > tools/testing/selftests/lib.mk | 40 +++-------------------------
> > ----
> > 2 files changed, 4 insertions(+), 57 deletions(-)
> >
> > diff --git a/tools/testing/selftests/Makefile
> > b/tools/testing/selftests/Makefile
> > index 3b2061d1c1a5..8247a7c69c36 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -155,12 +155,10 @@ 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
> >
> > @@ -174,7 +172,7 @@ export KHDR_INCLUDES
> > # all isn't the first target in the file.
> > .DEFAULT_GOAL := all
> >
> > -all: kernel_header_files
> > +all:
> > @ret=1;
> > \
> > for TARGET in $(TARGETS); do \
> > BUILD_TARGET=$$BUILD/$$TARGET; \
> > @@ -185,23 +183,6 @@ all: kernel_header_files
> > 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; \
> > diff --git a/tools/testing/selftests/lib.mk
> > b/tools/testing/selftests/lib.mk
> > index 118e0964bda9..aa646e0661f3 100644
> > --- a/tools/testing/selftests/lib.mk
> > +++ b/tools/testing/selftests/lib.mk
> > @@ -44,26 +44,10 @@ endif
> > selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
> > top_srcdir = $(selfdir)/../../..
> >
> > -ifeq ("$(origin O)", "command line")
> > - KBUILD_OUTPUT := $(O)
> > +ifeq ($(KHDR_INCLUDES),)
> > +KHDR_INCLUDES := -isystem $(top_srcdir)/usr/include
> > endif
> >
> > -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.
> > @@ -74,25 +58,7 @@ 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: 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
> > +all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED)
> > $(TEST_GEN_FILES)
> >
> > define RUN_TESTS
> > BASE_DIR="$(selfdir)"; \
>
On 12/11/23 03:00, Muhammad Usama Anjum wrote:
> On 12/9/23 7:01 AM, John Hubbard wrote:
>> This reverts commit 9fc96c7c19df ("selftests: error out if kernel header
>> files are not yet built").
> I don't think whole of this commit needs to be reverted. Lets leave the
> warning message as it is and just remove the condition to abort the
> compilation.
>
Hi Muhammad!
If we do decide that "make headers" or something like it is required,
then yes, this patch should be changed from a revert, to a "warn instead
of failing out" patch.
First, though, I'd like us to choose a design direction. The patch as
written is intended to put us on a design that does not require "make
headers" before building the selftests, because that approach would work
for all the cases I've seen so far.
If we want something else, then David Hildenbrand has listed several
ideas, and I've added a 4th one to the list, in [1].
[1] https://lore.kernel.org/[email protected]
thanks,
--
John Hubbard
NVIDIA
On 12/12/23 12:00 AM, John Hubbard wrote:
> On 12/11/23 03:00, Muhammad Usama Anjum wrote:
>> On 12/9/23 7:01 AM, John Hubbard wrote:
>>> This reverts commit 9fc96c7c19df ("selftests: error out if kernel header
>>> files are not yet built").
>> I don't think whole of this commit needs to be reverted. Lets leave the
>> warning message as it is and just remove the condition to abort the
>> compilation.
>>
>
> Hi Muhammad!
>
> If we do decide that "make headers" or something like it is required,
> then yes, this patch should be changed from a revert, to a "warn instead
> of failing out" patch.
I support this is as most of the times when the latest headers aren't
installed in the system. Hence the build of all those kselftests would fail
which require the recently added macros. There is no workaround to build
those tests until `make headers` is done or the latest headers are
installed. The former is easier.
If we just turn this into warning, most people reporting issues with `make
headers` would go away. They will be able to build all those kselftest
which don't require latest headers. For example mincore kselftest gets
build without KHDR_INCLUDES. In case people want to build failing tests,
they should add #ifdefs to the tests and submit patches which is idea 4.
>
> First, though, I'd like us to choose a design direction. The patch as
> written is intended to put us on a design that does not require "make
> headers" before building the selftests, because that approach would work
> for all the cases I've seen so far.
>
> If we want something else, then David Hildenbrand has listed several
> ideas, and I've added a 4th one to the list, in [1].
>
>
> [1] https://lore.kernel.org/[email protected]
>
>
> thanks,
--
BR,
Muhammad Usama Anjum