| Message ID | 20230606071637.267103-12-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A minor flurry of selftest/mm fixes | expand |
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)"; \
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)"; \
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,
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,
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.
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.
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.
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.
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.
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.
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
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?
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?
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,
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).
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 --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)"; \
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(-)