Re: [PATCH v2 2/2] selftests/lib.mk: Introduce check to validate required configs

[Petr Mladek <pmladek@suse.com>]

On Sat 2024-12-21 01:05:36, 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 config files. In case scripts/config or the current
> kernel config is not found, this check is skipped.
> 
> In order to view the missing config options required to compile the test,
> set the environment variable LOCALMODCONFIG_DEBUG=1.

As I wrote in the review for the 1st patch, I would prefer to print
the missing config options by default. The LOCALMODCONFIG_DEBUG
variable is pretty non-standard and hard to memorize thing.

> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -97,7 +97,14 @@ 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) \
> +TEST_DIR := $(shell pwd)
> +
> +check_config_deps:
> +	@$(selfdir)/mktest.pl $(TEST_DIR)/config || \
> +	{ echo "Skipping test: $(notdir $(TEST_DIR))"; exit 1; }

I would write a more meaningful message, e.g.

	{ echo "Skipping test because of missing kernel features: $(notdir $(TEST_DIR))"; exit 1; }

> +
> +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 +235,4 @@ $(OUTPUT)/%:%.S
>  	$(LINK.S) $^ $(LDLIBS) -o $@
>  endif

Otherwise, it seems to work well for the livepatching selftests.

I guess that it might prevent running some selftests because of
too strict or outdated information in some
tools/testing/selftests/<project>/config files. So that it might
cause regressions.

But I think that this is the right way to go. I am just not sure
whether we should wait for complains from linux-next. Or if we
should be more proactive in fixing the various <project>/config
files.

Best Regards,
Petr