ACK/Cmnt: [SRU][U/F/E/B][PATCH 1/1] UBUNTU: [Packaging] add support to compile/run selftests

[Kleber Souza]

On 23.04.20 09:17, Stefan Bader wrote:
> On 22.04.20 18:40, Kleber Sacilotto de Souza wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1874286
>>
>> Add some simple debian/rules targets that will keep a list of the
>> selftests that Ubuntu cares about, compile and run them. This can be
>> plugged into our test build infrastructure in order to compile the
>> testcases before we close and package a release.
>>
>> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
>> ---
> 

Hi Stefan,

Thanks for the review.

> In general ok with it. Just wondering whether
> a) the new targets should be .PHONY

Good catch.

> b) runselftests should depend on compileselftests
> 

The reason I wanted to keep compile and run independent is:
1. the target run_tests will also compile the testcase before running if
it hasn't been compiled.
2. when providing a list of TARGETS the return code will always be 0
even if tests fail to compile.

So in order to catch compilation errors with the return code but still
have the option to run all the testcases, even knowing that some of them
might be broken, I decided to add the two targets that are run differently.

But I agree that this is not clear by looking at the description of the
targets.  I'll add some more info when applying.


Thanks,
Kleber

> This ack is for the xenial variant, too.
> 
> -Stefan
> 
>>  debian/rules.d/0-common-vars.mk |  3 +++
>>  debian/rules.d/1-maintainer.mk  | 13 +++++++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/debian/rules.d/0-common-vars.mk b/debian/rules.d/0-common-vars.mk
>> index bcab5749f8aa..bcfbc497d78c 100644
>> --- a/debian/rules.d/0-common-vars.mk
>> +++ b/debian/rules.d/0-common-vars.mk
>> @@ -255,3 +255,6 @@ LN = ln -sf
>>  # flavour as arguments.
>>  custom_override = \
>>   $(shell if [ -n "$($(1)_$(2))" ]; then echo "$($(1)_$(2))"; else echo "$($(1))"; fi)
>> +
>> +# selftests that Ubuntu cares about
>> +ubuntu_selftests = breakpoints bpf cpu-hotplug efivarfs memfd memory-hotplug mount net ptrace seccomp timers powerpc user ftrace
>> diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk
>> index 828e47bc1330..339fe9c8953e 100644
>> --- a/debian/rules.d/1-maintainer.mk
>> +++ b/debian/rules.d/1-maintainer.mk
>> @@ -26,6 +26,10 @@ help:
>>  	@echo
>>  	@echo "  diffupstream    : Diff stock kernel code against upstream (git)"
>>  	@echo
>> +	@echo "  compileselftests : Only compile the selftests listed on ubuntu_selftests variable"
>> +	@echo
>> +	@echo "  runselftests    : Run the selftests listed on ubuntu_selftests variable"
>> +	@echo
>>  	@echo "  help            : If you are kernel hacking, you need the professional"
>>  	@echo "                    version of this"
>>  	@echo
>> @@ -68,6 +72,7 @@ printenv:
>>  	@echo "skipdbg           = $(skipdbg)"
>>  	@echo "ubuntu_log_opts   = $(ubuntu_log_opts)"
>>  	@echo "CONCURRENCY_LEVEL = $(CONCURRENCY_LEVEL)"
>> +	@echo "ubuntu_selftests  = $(ubuntu_selftests)"
>>  	@echo "bin package name  = $(bin_pkg_name)"
>>  	@echo "hdr package name  = $(hdrs_pkg_name)"
>>  	@echo "doc package name  = $(doc_pkg_name)"
>> @@ -159,3 +164,11 @@ startnewrelease:
>>  	cat $(DEBIAN)/changelog >> $(DEBIAN)/changelog.new; \
>>  	mv $(DEBIAN)/changelog.new $(DEBIAN)/changelog
>>  
>> +compileselftests:
>> +	# a loop is needed here to fail on errors
>> +	for test in $(ubuntu_selftests); do \
>> +		$(kmake) -C tools/testing/selftests TARGETS="$$test"; \
>> +	done;
>> +
>> +runselftests:
>> +	$(kmake) -C tools/testing/selftests TARGETS="$(ubuntu_selftests)" run_tests
>>
> 
>