diff mbox series

[2/3] selftests/lib.mk: Introduce check to validate required configs

Message ID 20241205114757.5916-3-simeddon@gmail.com (mailing list archive)
State New
Headers show
Series kselftest framework to introduce TEST_CONFIG_DEPS | expand

Commit Message

BiscuitBobby Dec. 5, 2024, 11:47 a.m. UTC 
Currently, kselftests does not have a generalised mechanism to skip compilation
and run tests when required kernel configuration flags are missing.

This patch introduces a check to validate the presence of required config flags
specified in the selftest makefile. In case scripts/config is not found,
this check is skipped.

Use TEST_CONFIG_DEPS to check for specific config options before compiling,
example usage:
```
TEST_CONFIG_DEPS := CONFIG_LIVEPATCH CONFIG_DYNAMIC_DEBUG
```
Here it checks whether CONFIG_LIVEPATCH and CONFIG_DYNAMIC_DEBUG are enabled.

Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Siddharth Menon <simeddon@gmail.com>
---
 tools/testing/selftests/lib.mk | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Mark Brown Dec. 5, 2024, 3:35 p.m. UTC | #1 
On Thu, Dec 05, 2024 at 05:17:56PM +0530, Siddharth Menon wrote:

> Currently, kselftests does not have a generalised mechanism to skip compilation
> and run tests when required kernel configuration flags are missing.

Should this be a build dependency or only a runtime dependency, or
should these be separate options for cases where the selftest builds a
module?  If people are building the selftests once and then using them
with a bunch of kernel builds it might be surprising if some of the
binaries vanish.

> -all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) \
> +KDIR ?= /lib/modules/$(shell uname -r)/build
> +

Shouldn't we try the current kernel tree, and for runtime checks
/proc/config.gz would be good to check when it's enabled?

> +define CHECK_CONFIG_DEPS
> +    $(if $(wildcard $(KDIR)/scripts/config),
> +        $(eval MISSING_FLAGS := $(filter-out 1,$(foreach cfg,$(TEST_CONFIG_DEPS),\
> +            $(shell cd $(KDIR) && scripts/config --state $(cfg) | grep -q '^\(y\|m\)$$' && echo 1 || echo $(cfg))))),
> +        $(info Skipping CHECK_GEN_REQ: $(KDIR)/scripts/config not found)
> +    )
> +    $(if $(MISSING_FLAGS),$(error Missing required config flags: $(MISSING_FLAGS)))
> +endef

This is going to use a separate set of config options to those listed in
the config file in the selftest directory which is perhaps a bit
surprising.  OTOH we do have a lot of the selftests directories where
not every test needs all the options so that's probably a good choice
unless we make things finer grained which might be more trouble than
it's worth.
BiscuitBobby Dec. 6, 2024, 7:20 p.m. UTC | #2 
On Thu, 5 Dec 2024 at 21:05, Mark Brown <broonie@kernel.org> wrote:
> Should this be a build dependency or only a runtime dependency, or
> should these be separate options for cases where the selftest builds a
> module?

Hello, thanks for looking through my patch. Some tests fail to compile and
throw errors in case their required config options are not enabled.
This optional check was created to prevent such issues.

> If people are building the selftests once and then using them
> with a bunch of kernel builds it might be surprising if some of the
> binaries vanish.

I'm not really familiar with packaging selftests, but this patch does not
remove existing binaries when running tests. If I misunderstood what
you are referring to, please let me know.

> Shouldn't we try the current kernel tree, and for runtime checks
> /proc/config.gz would be good to check when it's enabled?

When I looked into this, it seems (according to the config.gz man page)
that the contents of /proc/config.gz are the same as
/lib/modules/$(uname -r)/build/.config, but /proc/config.gz is only available if
the kernel was compiled with CONFIG_IKCONFIG_PROC enabled.

It does seem like /lib/modules/$(uname -r)/build/scripts is not always available
though, so will send in a new patch directly checking build/.config
after checking
it out on a few more systems.
Mark Brown Dec. 6, 2024, 7:50 p.m. UTC | #3 
On Sat, Dec 07, 2024 at 12:50:47AM +0530, BiscuitBobby wrote:
> On Thu, 5 Dec 2024 at 21:05, Mark Brown <broonie@kernel.org> wrote:

> > If people are building the selftests once and then using them
> > with a bunch of kernel builds it might be surprising if some of the
> > binaries vanish.

> I'm not really familiar with packaging selftests, but this patch does not
> remove existing binaries when running tests. If I misunderstood what
> you are referring to, please let me know.

It doesn't remove existing barriers but it's also not obvious that this
isn't just a generic dependency mechanism that should be used for any
old dependency rather than things that would actually break the build,
and it's one config list for both build and runtime checks.  If the
intention is that this should be infrequently used it probably needs to
be a bit clearer that that's the case.  As things stand I'd expect there
to be some confusion about the interaction between this and the existing
system with specifying config fragments.

> > Shouldn't we try the current kernel tree, and for runtime checks
> > /proc/config.gz would be good to check when it's enabled?

> When I looked into this, it seems (according to the config.gz man page)
> that the contents of /proc/config.gz are the same as
> /lib/modules/$(uname -r)/build/.config, but /proc/config.gz is only available if
> the kernel was compiled with CONFIG_IKCONFIG_PROC enabled.

> It does seem like /lib/modules/$(uname -r)/build/scripts is not always available
> though, so will send in a new patch directly checking build/.config
> after checking
> it out on a few more systems.

Indeed, when deploying a kernel for test people don't always deploy
modules onto the target filesystem, there may not be any modules at all
if CONFIG_MODULES is disabled, or people might just not install modules
that do get built if they're not needed on a given platform.
BiscuitBobby Dec. 6, 2024, 8:12 p.m. UTC | #4 
On Sat, 7 Dec 2024 at 01:20, Mark Brown <broonie@kernel.org> wrote:
>
> It doesn't remove existing barriers but it's also not obvious that this
> isn't just a generic dependency mechanism that should be used for any
> old dependency rather than things that would actually break the build,
> and it's one config list for both build and runtime checks.  If the
> intention is that this should be infrequently used it probably needs to
> be a bit clearer that that's the case.  As things stand I'd expect there
> to be some confusion about the interaction between this and the existing
> system with specifying config fragments.

I shall update the commit and documentation to make this more clear.
Thank you for the feedback.
Petr Mladek Dec. 10, 2024, 2:56 p.m. UTC | #5 
On Thu 2024-12-05 17:17:56, Siddharth Menon wrote:
> Currently, kselftests does not have a generalised mechanism to skip compilation
> and run tests when required kernel configuration flags are missing.
> 
> This patch introduces a check to validate the presence of required config flags
> specified in the selftest makefile. In case scripts/config is not found,
> this check is skipped.
> 
> Use TEST_CONFIG_DEPS to check for specific config options before compiling,
> example usage:
> ```
> TEST_CONFIG_DEPS := CONFIG_LIVEPATCH CONFIG_DYNAMIC_DEBUG

What is the reason to add another set of dependencies, please?

Both CONFIG_LIVEPATCH CONFIG_DYNAMIC_DEBUG are already mentioned in
tools/testing/selftests/livepatch/config

IMHO, the new check should read the dependencies
from the existing tools/testing/selftests/<test>/config file.

> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -97,7 +97,21 @@ 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) \
> +KDIR ?= /lib/modules/$(shell uname -r)/build
> +
> +define CHECK_CONFIG_DEPS
> +    $(if $(wildcard $(KDIR)/scripts/config),
> +        $(eval MISSING_FLAGS := $(filter-out 1,$(foreach cfg,$(TEST_CONFIG_DEPS),\
> +            $(shell cd $(KDIR) && scripts/config --state $(cfg) | grep -q '^\(y\|m\)$$' && echo 1 || echo $(cfg))))),
> +        $(info Skipping CHECK_GEN_REQ: $(KDIR)/scripts/config not found)
> +    )
> +    $(if $(MISSING_FLAGS),$(error Missing required config flags: $(MISSING_FLAGS)))
> +endef

It somehow does not work here. I get:

tools/testing/selftests/livepatch # make run_tests 
grep: .config: No such file or directory
grep: .config: No such file or directory
grep: .config: No such file or directory
grep: .config: No such file or directory
../lib.mk:112: *** Missing required config flags: CONFIG_LIVEPATCH CONFIG_DYNAMIC_DEBUG.  Stop.


I run the livepatch tests the following way.

1. On my workstation, I build the kernel RPMs using

     make rpm-pkg

2. In qemu test system, I mount the build directory from the
   workstation and install both kernel and kernel-devel packages:

    rpm -ivh rpmbuild/RPMS/x86_64/kernel-6.12.0_default+-35.x86_64.rpm
    rpm -ivh rpmbuild/RPMS/x86_64/kernel-devel-6.12.0_default+-35.x86_64.rpm

   and reboot

3. In rebooted qemu test system, I mount once again the build
   directory from the workstation and run the tests:

     cd tools/testing/selftests/livepatch
     make run_tests


The "grep" errors come from the "scripts/config" command. For example:

   tools/testing/selftests/livepatch # /lib/modules/6.12.0-default+/build/scripts/config -s CONFIG_LIVEPATCH
   grep: .config: No such file or directory
   grep: .config: No such file or directory
   undef

It helps to define patch to the config file installed by the devel
package:

   tools/testing/selftests/livepatch # /lib/modules/6.12.0-default+/build/scripts/config --file /lib/modules/6.12.0-default+/config -s CONFIG_LIVEPATCH
   y


But I am not sure if this works when people run the "make" in the
original build directory on the workstation.

Best Regards,
Petr
BiscuitBobby Dec. 10, 2024, 5:10 p.m. UTC | #6 
On Tue, 10 Dec 2024 at 20:26, Petr Mladek <pmladek@suse.com> wrote:
>
> What is the reason to add another set of dependencies, please?

I had done this because not every test required all the options specified in
tools/testing/selftests/<test>/config. I thought it would not be desirable to
prevent these tests from compiling/running.

> Both CONFIG_LIVEPATCH CONFIG_DYNAMIC_DEBUG are already mentioned in
> tools/testing/selftests/livepatch/config

This particular test only required CONFIG_LIVEPATCH to compile, but I
had included CONFIG_DYNAMIC_DEBUG, as Miroslav had expressed
wanting both of them checked.

> IMHO, the new check should read the dependencies
> from the existing tools/testing/selftests/<test>/config file.

I shall check tools/testing/selftests/<test>/config in my next patch as
suggested.

> I run the livepatch tests the following way.
>
> 1. On my workstation, I build the kernel RPMs using
>
>      make rpm-pkg
>
> 2. In qemu test system, I mount the build directory from the
>    workstation and install both kernel and kernel-devel packages:
>
>     rpm -ivh rpmbuild/RPMS/x86_64/kernel-6.12.0_default+-35.x86_64.rpm
>     rpm -ivh rpmbuild/RPMS/x86_64/kernel-devel-6.12.0_default+-35.x86_64.rpm
>
>    and reboot
>
> 3. In rebooted qemu test system, I mount once again the build
>    directory from the workstation and run the tests:
>
>      cd tools/testing/selftests/livepatch
>      make run_tests

Thanks, I will test building kernel RPMs when I update the check to be
more distro agnostic.

Sincerely,
Siddharth Menon
diff mbox series

Patch

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index d6edcfcb5be8..7ca713237bf7 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -97,7 +97,21 @@  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) \
+KDIR ?= /lib/modules/$(shell uname -r)/build
+
+define CHECK_CONFIG_DEPS
+    $(if $(wildcard $(KDIR)/scripts/config),
+        $(eval MISSING_FLAGS := $(filter-out 1,$(foreach cfg,$(TEST_CONFIG_DEPS),\
+            $(shell cd $(KDIR) && scripts/config --state $(cfg) | grep -q '^\(y\|m\)$$' && echo 1 || echo $(cfg))))),
+        $(info Skipping CHECK_GEN_REQ: $(KDIR)/scripts/config not found)
+    )
+    $(if $(MISSING_FLAGS),$(error Missing required config flags: $(MISSING_FLAGS)))
+endef
+
+check_config_deps:
+	$(call CHECK_CONFIG_DEPS)
+
+all: check_config_deps $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) \
 	$(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
 
 define RUN_TESTS
@@ -228,4 +242,4 @@  $(OUTPUT)/%:%.S
 	$(LINK.S) $^ $(LDLIBS) -o $@
 endif
 
-.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir
+.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir check_config_deps