Re: [PATCH bpf-next v4] selftests/bpf: use auto-dependencies for test objects

["Björn Töpel" <bjorn@kernel.org>]

Ihor Solodrai <ihor.solodrai@pm.me> writes:

> On Saturday, September 14th, 2024 at 3:54 AM, Björn Töpel <bjorn@kernel.org> wrote:
>
> [...]
>
>> > 
>> > Could you please check on your side if this helps? Maybe there are
>> > other issues.
>> 
>> 
>> I don't even get that far in the "install" target. When I revert the
>> patch, I get to the issue you describe above (trying to install
>> non-existing file).
>> 
>> Here's an excerpt from a failed "install":
>> 
>> | BINARY test_progs
>> | BINARY test_progs-no_alu32
>> | BINARY test_progs-cpuv4
>> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> 
>> At this point the "all" target is complete; All good, and the "install"
>> is started.
>> 
>> | mkdir -p /home/bjorn/output/foo/kselftest/kselftest
>> | install -m 744 kselftest/module.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/runner.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/prefix.pl /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/ktap_helpers.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/ksft.py /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 run_kselftest.sh /home/bjorn/output/foo/kselftest/
>> | rm -f /home/bjorn/output/foo/kselftest/kselftest-list.txt
>> 
>> This is from the top-level "tools/testing/selftests/Makefile", and we
>> enter the BPF directory for "install".
>> 
>> | make[1]: Entering directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> |
>> | Auto-detecting system features:
>> | ... llvm: [ OFF ]
>> |
>> | LINK-BPF [test_progs] test_static_linked.bpf.o
>> | LINK-BPF [test_progs] linked_funcs.bpf.o
>> | LINK-BPF [test_progs] linked_vars.bpf.o
>> | LINK-BPF [test_progs] linked_maps.bpf.o
>> | LINK-BPF [test_progs] test_subskeleton.bpf.o
>> | LINK-BPF [test_progs] test_subskeleton_lib.bpf.o
>> | ...
>> | EXT-COPY [test_maps]
>> | make[1]: *** No rule to make target 'atomics.lskel.h', needed by '/home/bjorn/output/foo/kselftest/bpf/atomics.test.o'. Stop.
>> | make[1]: *** Waiting for unfinished jobs....
>> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> | make: *** [Makefile:259: install] Error 2
>> | make: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests'
>> 
>> ...and for some reason the auto-dependencies decides to re-build, and
>> fails.
>> 
>> So, unfortunately it's something else related to your patch.
>
> Björn,
>
> I think I figured out the cause of the issue, but some details and a
> proper solution are unclear yet.
>
> In generated %.test.d makefiles some dependencies (in particular
> %.skel.h) are referred to by filename as opposed to full path. For
> example:
>
>     $ cat /output/foo/kselftest/bpf/cpuv4/atomics.test.d
>     /output/foo/kselftest/bpf/cpuv4/atomics.test.o: \
>      /opt/linux/tools/testing/selftests/bpf/prog_tests/atomics.c \
>      [...]
>      /opt/linux/tools/testing/selftests/bpf/trace_helpers.h \
>      /opt/linux/tools/testing/selftests/bpf/testing_helpers.h atomics.lskel.h \  # <-- THIS
>      /output/foo/kselftest/bpf/tools/include/bpf/skel_internal.h \
>      /output/foo/kselftest/bpf/tools/include/bpf/bpf.h
>
> This is of course a problem, because make needs to know how to build a
> target with this exact name. And in the patch it was (partially)
> solved by this piece:
>
> +# When the compiler generates a %.d file, only skel basenames (not
> +# full paths) are specified as prerequisites for corresponding %.o
> +# file. This target makes %.skel.h basename dependent on full paths,
> +# linking generated %.d dependency with actual %.skel.h files.
> +$(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
> +	@true
>
> This links %.skel.h to /output/foo/kselftest/bpf/no_alu32/%.skel.h and alike.
>
> Your build is breaking because there is no such rule for
> %.lskel.h and %.subskel.h, which are trivial to add:
>
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -628,6 +628,12 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TR
>  $(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
>         @true
>  
> +$(notdir %.lskel.h): $(TRUNNER_OUTPUT)/%.lskel.h
> +       @true
> +
> +$(notdir %.subskel.h): $(TRUNNER_OUTPUT)/%.subskel.h
> +       @true
> +
>  endif
>
> With this change a command below completed for me in the environment
> you shared:
>
>      $ make -j \$((\$(nproc)-1)) O=/output/foo TARGETS=bpf SKIP_TARGETS="" KSFT_INSTALL_PATH=/output/foo/kselftest -C tools/testing/selftests install

Thank you! FWIW, this solves my build issue, and by extension making it
possible for the RISC-V CI to test BPF again! ;-)

Tested-by: Björn Töpel <bjorn@kernel.org>

Would be awesome if you can spin a patch proper for the above. Even if
it's uncomplete (by what you mention below), it solves my immediate
issue.

> What is a mystery to me is why this was not an issue for simple `make
> -C tool/testing/selftests/bpf`. And also why in the environment I
> tried yesterday I didn't get the failure you're seeing.
>
> Do you happen to have a Dockerfile of ghcr.io/linux-riscv/pw-builder ?
> If possible, I'd like to look at it and compare with my local
> environment.

Sure, it's at: https://github.com/linux-riscv/docker

Note that it's not something special. Simply Ubuntu 24.04 (noble), with
Clang/LLVM nightly from apt.llvm.org. I can reproduce this on my laptop,
and non-Docker builder that are running 24.04,a and Debian Sid.

> The other issue that I'll have to think about is the unnecessary
> re-builds that you've noticed. I suspect this happens due to the same
> reason: a generated dependency on X.skel.h can't find a file in
> current directory (because it was put to /output/foo), and so rebuild
> is triggered. I'll have to come up with a workaround.
>
>
> Björn, please try a change suggested above and let me know if it helps.
>
> I will investigate these problems more, and there will definitely be a
> follow up patch.
>
> Thank you for reporting.

Thank you for the swift response, and workaround! Much appreciated!

Now, enjoy the rest of the Sunday!


Cheers,
Björn