libbpf / ci

BPF CI
Other
10 stars 24 forks source link

selftests: override `KHDR_INCLUDES` var #131

Open matttbe opened 1 month ago

matttbe commented 1 month ago

In a recent series [1], I suggested to use KHDR_INCLUDES variable to avoid having to duplicate UAPI header files.

The BPF CI builds the kernel in a separated directory -- KBUILD_OUTPUT variable is set and exported -- and the BPF selftests are executed directly, from the selftests/bpf directory, not from its parent with the TARGETS parameter. In this case, it is required to override KHDR_INCLUDES to look at the build directory, and not the kernel source, in usr/include.

Note that tools/testing/selftests/Makefile supports KBUILD_OUTPUT, but this Makefile is not used by the BPF CI: it directly uses the one from the bpf directory: tools/testing/selftests/bpf/Makefile. That's fine, KHDR_INCLUDES can be overridden, that should then fix the build issue seen in [1].

Also, this KHDR_INCLUDES variable is not used by the BPF selftests before my series [1]. It is then fine to merge this modification before applying my modifications.

Link: https://lore.kernel.org/bpf/20240816-ups-bpf-next-selftests-use-khdr-v1-0-1e19f3d5b17a@kernel.org/ [1]


Note that I validated this modification only manually, I didn't try to deploy the CI infrastructure on my side. When applying the same commands as the ones from this script, I can reproduce the issue reported by the CI. Not after having set KHDR_INCLUDES.

chantra commented 4 days ago

Hi @matttbe , sorry for the last update here. Reading bpf: use KHDR_INCLUDES for the UAPI headers, it seems this is not needed right? Or at least not just now.

If you still think this is needed, would you mind rebasing so it just integration tests (which were not enabled at the time you created this PR). If this is not needed, we can close this for now.

matttbe commented 3 days ago

Hi @chantra, thank you for your reply.

Reading bpf: use KHDR_INCLUDES for the UAPI headers, it seems this is not needed right? Or at least not just now.

These two patches have indeed been rejected, but I think this modification is still needed: Alexei didn't want to modify the existing selftests, but, from what I understood, he was open to add KHDR_INCLUDES if there was a need. But for that, the CI needs to support it.

A bit of context: for the moment, if a recent UAPI header files is needed for a new BPF selftest, the header file needs to be duplicated in tools/includes. Except that the network maintainers don't want more duplicated files there, the ones generated by make headers should be used instead. To do that, we need a way to know where the UAPI kernel headers have been generated. The KSelfTests provides KHDR_INCLUDES variable, but here, and because how the selftests are built, it needs to be overridden to point to the build dir which is in a separated directory, not the default one. Adding the new variable will not change anything for the existing test because it is currently not used by the BPF Selftest Makefile. But still, I think it makes sense to set it to the right value, and it will help later when it will be needed.

Note that I sent the kernel patches I mentioned in the PR description because some new tests depended on a new UAPI header file. Because I could not duplicate this header file, and because the BPF CI didn't set KHDR_INCLUDES, I had to find an alternative in the newer versions. So I don't need it for the moment, but to avoid others to find alternatives, it would be good if the BPF CI set KHDR_INCLUDES to the correct value. I'm sure someone else will need access other UAPI files in the near future.