Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

selftests: override KHDR_INCLUDES var #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matttbe
Copy link

@matttbe matttbe commented Aug 17, 2024

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
Copy link
Collaborator

chantra commented Sep 13, 2024

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.

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. In thi
s 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]
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
@matttbe
Copy link
Author

matttbe commented Sep 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants