2023-07-11 01:10:08

by John Hubbard

[permalink] [raw]
Subject: [PATCH] selftests/arm64: fix build failure during the "emit_tests" step

The build failure reported in [1] occurred because commit 9fc96c7c19df
("selftests: error out if kernel header files are not yet built") added
a new "kernel_header_files" dependency to "all", and that triggered
another, pre-existing problem. Specifically, the arm64 selftests
override the emit_tests target, and that override improperly declares
itself to depend upon the "all" target.

This is a problem because the "emit_tests" target in lib.mk was not
intended to be overridden. emit_tests is a very simple, sequential build
target that was originally invoked from the "install" target, which in
turn, depends upon "all".

That approach worked for years. But with 9fc96c7c19df in place,
emit_tests failed, because it does not set up all of the elaborate
things that "install" does. And that caused the new
"kernel_header_files" target (which depends upon $(KBUILD_OUTPUT) being
correct) to fail.

Some detail: The "all" target is .PHONY. Therefore, each target that
depends on "all" will cause it to be invoked again, and because
dependencies are managed quite loosely in the selftests Makefiles, many
things will run, even "all" is invoked several times in immediate
succession. So this is not a "real" failure, as far as build steps go:
everything gets built, but "all" reports a problem when invoked a second
time from a bad environment.

To fix this, simply remove the unnecessary "all" dependency from the
overridden emit_tests target. The dependency is still effectively
honored, because again, invocation is via "install", which also depends
upon "all".

An alternative approach would be to harden the emit_tests target so that
it can depend upon "all", but that's a lot more complicated and hard to
get right, and doesn't seem worth it, especially given that emit_tests
should probably not be overridden at all.

[1] https://lore.kernel.org/[email protected]

Fixes: 9fc96c7c19df ("selftests: error out if kernel header files are not yet built")
Reported-by: Mark Brown <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/arm64/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
index 9460cbe81bcc..ace8b67fb22d 100644
--- a/tools/testing/selftests/arm64/Makefile
+++ b/tools/testing/selftests/arm64/Makefile
@@ -42,7 +42,7 @@ run_tests: all
done

# Avoid any output on non arm64 on emit_tests
-emit_tests: all
+emit_tests:
@for DIR in $(ARM64_SUBTARGETS); do \
BUILD_TARGET=$(OUTPUT)/$$DIR; \
make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \

base-commit: d5fe758c21f4770763ae4c05580be239be18947d
--
2.41.0



2023-07-11 01:10:11

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH] selftests/arm64: fix build failure during the "emit_tests" step

On 7/10/23 17:56, John Hubbard wrote:
> The build failure reported in [1] occurred because commit 9fc96c7c19df
> ("selftests: error out if kernel header files are not yet built") added
> a new "kernel_header_files" dependency to "all", and that triggered
> another, pre-existing problem. Specifically, the arm64 selftests
> override the emit_tests target, and that override improperly declares
> itself to depend upon the "all" target.

I also have a corresponding fix for selftests/riscv, which has copied the
arm64 selftest approach. But I'll wait to see how reviews go here,
before sending that out.

thanks,
--
John Hubbard
NVIDIA

>
> This is a problem because the "emit_tests" target in lib.mk was not
> intended to be overridden. emit_tests is a very simple, sequential build
> target that was originally invoked from the "install" target, which in
> turn, depends upon "all".
>
> That approach worked for years. But with 9fc96c7c19df in place,
> emit_tests failed, because it does not set up all of the elaborate
> things that "install" does. And that caused the new
> "kernel_header_files" target (which depends upon $(KBUILD_OUTPUT) being
> correct) to fail.
>
> Some detail: The "all" target is .PHONY. Therefore, each target that
> depends on "all" will cause it to be invoked again, and because
> dependencies are managed quite loosely in the selftests Makefiles, many
> things will run, even "all" is invoked several times in immediate
> succession. So this is not a "real" failure, as far as build steps go:
> everything gets built, but "all" reports a problem when invoked a second
> time from a bad environment.
>
> To fix this, simply remove the unnecessary "all" dependency from the
> overridden emit_tests target. The dependency is still effectively
> honored, because again, invocation is via "install", which also depends
> upon "all".
>
> An alternative approach would be to harden the emit_tests target so that
> it can depend upon "all", but that's a lot more complicated and hard to
> get right, and doesn't seem worth it, especially given that emit_tests
> should probably not be overridden at all.
>
> [1] https://lore.kernel.org/[email protected]
>
> Fixes: 9fc96c7c19df ("selftests: error out if kernel header files are not yet built")
> Reported-by: Mark Brown <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> tools/testing/selftests/arm64/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> index 9460cbe81bcc..ace8b67fb22d 100644
> --- a/tools/testing/selftests/arm64/Makefile
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -42,7 +42,7 @@ run_tests: all
> done
>
> # Avoid any output on non arm64 on emit_tests
> -emit_tests: all
> +emit_tests:
> @for DIR in $(ARM64_SUBTARGETS); do \
> BUILD_TARGET=$(OUTPUT)/$$DIR; \
> make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>
> base-commit: d5fe758c21f4770763ae4c05580be239be18947d



2023-07-11 15:07:00

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] selftests/arm64: fix build failure during the "emit_tests" step

On Mon, Jul 10, 2023 at 05:56:29PM -0700, John Hubbard wrote:
> The build failure reported in [1] occurred because commit 9fc96c7c19df
> ("selftests: error out if kernel header files are not yet built") added
> a new "kernel_header_files" dependency to "all", and that triggered
> another, pre-existing problem. Specifically, the arm64 selftests
> override the emit_tests target, and that override improperly declares
> itself to depend upon the "all" target.

Tested-by: Mark Brown <[email protected]>

Thanks!


Attachments:
(No filename) (524.00 B)
signature.asc (499.00 B)
Download all attachments