Skip to content

Check vmlinux.h header up-to-dateness #16

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.

Sign up for GitHub

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

Merged
danielocfb merged 3 commits into from
Jan 9, 2026
Merged

Check vmlinux.h header up-to-dateness #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af55bc9
Clone pahole from GitHub
d-e-s-o Jan 6, 2026
faaf636
Generate vmlinux.h headers in parallel
d-e-s-o Jan 6, 2026
8075b45
Check vmlinux.h header up-to-dateness
d-e-s-o Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,31 @@ jobs:
EOF
cargo update
cargo check --tests
generate-headers:
name: Generate vmlinux.h headers
uses: ./.github/workflows/vmlinux.h.yml
check-headers:
name: Check headers are up to date
runs-on: ubuntu-latest
needs: generate-headers
steps:
- uses: actions/checkout@v4

- name: Download generated headers
uses: actions/download-artifact@v4
with:
name: vmlinux.h
path: generated-headers

- name: Copy headers to include/ directory
shell: bash
run: |
# Copy each vmlinux.h file to the corresponding include/ subdirectory
for dir in generated-headers/*/; do
arch=$(basename "$dir")
cp "$dir"vmlinux.h "include/$arch/"
done

- name: Check for differences
run: git diff --exit-code ||
(echo "!!!! CHECKED IN vmlinux.h HEADER IS OUTDATED !!!!" && false)
86 changes: 62 additions & 24 deletions .github/workflows/vmlinux.h.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,22 @@ on:
branches:
- main
workflow_dispatch:
workflow_call:

jobs:
gen-headers:
name: Generate vmlinux.h
name: Generate vmlinux.h (${{ matrix.arch }})
runs-on: ubuntu-latest
strategy:
matrix:
arch:
- x86_64
- aarch64
- arm
- loongarch64
- ppc64le
- riscv64
- s390x

steps:

Expand All @@ -24,42 +35,69 @@ jobs:
fetch-depth: 1
path: linux/

- name: Check out pahole source
uses: actions/checkout@v6
with:
repository: 'acmel/dwarves'
ref: 'next'
fetch-depth: 1
path: pahole/

- name: Install pahole
Copy link
Contributor

@theihor theihor Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Install fresh pahole" seems to be a common requirement across libbpf/bpf CI, so we might want to factor out install-pahole action from setup-build-env in libbpf/ci and use it.

It's more work though, so at your discretion of course.

shell: bash
run: |
sudo apt -y install cmake git libdw-dev libelf-dev
cd pahole
mkdir -p build
cmake -B=build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/usr
make -C build -j$(nproc)
sudo make -C build install
Comment on lines +49 to +54
Copy link
Contributor

@theihor theihor Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having separate shell scripts is generally easier to test, compared to the code inlined in yaml. Any reason you decided to inline?

I can see how using actions/checkout might be preferable, but then we could pass a repo path to the script.

Copy link
Contributor Author

@d-e-s-o d-e-s-o Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's horrible having to switch between shell scripts and yaml. It's a different files, different language, different context. And all that with two languages that are among the most error prone to write.

In this case it's six lines for which I think it makes no sense to have to context switch for. If it was for me the remaining scripts would be inlined as well.

A shell script may -- in theory -- make it easier to test. In practice it's an accumulation of half a dozen scripts that are rarely properly isolated (e.g., using GitHub specific variables), that depend on a specific distro and package manager that barely anybody uses (fb uses Fedora/Cent OS with different package manager). So at that point you end up mocking variables, modifying scripts, translating package names, and whatnot just to test locally.

If this, say, were a fully reproducible Nix-style thing, for which we could run one command and would be guaranteed a reliable repro, that would change my perspective.

In my opinion, and I understand that is somewhat of a hot take, the incentives should be set to test in the form of end-to-end workflow in CI and upstream that. That's the environment that is at least somewhat stable and, more importantly, that we regularly test/run on.

Copy link
Contributor

@theihor theihor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is a can-of-worm-y discussion, but since you've shared your opinion I'll share mine :)

I think it's horrible having to switch between shell scripts and yaml. It's a different files, different language, different context. And all that with two languages that are among the most error prone to write.

I mostly agree. However at some point a decision was made to use GitHub Actions platform, and the language we must use to interface with it is YAML. All things considered, the benefits from using the Actions so far outweighted the incovenience of YAML (not to mention outages, bugs etc).

Now YAML is not a programming language. However one can inline programs there, which is convenient for small workflows. But I argue that it slows down development and debugging.

In this case it's six lines for which I think it makes no sense to have to context switch for. If it was for me the remaining scripts would be inlined as well.

By "context switch" do you mean opening a different file? If so IMO it's better because now you know you're just in a bash script and not in "bash string in yaml". With proper discipline in writing scripts (admittedly, not an easy thing to maintain), the dependencies can be very clear.

From the execution perspective there is no difference. IIRC actions software puts inline code into temporary files anyway.

A shell script may -- in theory -- make it easier to test. In practice it's an accumulation of half a dozen scripts that are rarely properly isolated (e.g., using GitHub specific variables), that depend on a specific distro and package manager that barely anybody uses (fb uses Fedora/Cent OS with different package manager). So at that point you end up mocking variables, modifying scripts, translating package names, and whatnot just to test locally.

Here I disagree. It is in fact, not in theory, easier to test a separate shell script, even if it requires 10 env variables to set, or a container to run.

If all the scripts are inlined, the only way to test a tiny change in a script is to run the entire workflow and wait. This dramatically slows down development and debugging, even if the workflow runs for less than a minute.

Yes, because of the environment dependencies you don't have a choice but spend time on setting them up locally. But the alternative is N minutes cycle for each dumb mistake. Not an issue one time, but it adds up quickly when debugging.

The problems with isolation and distro differences do not disappear because the script got inlined. It still runs apt-get install. When you're testing on Fedora and your workflow runs on Ubuntu, translating the packages is the wrong way to do it, because then you're not testing the code you're going to run. Personally I use containers for this, it's not difficult.

Btw "package manager that barely anybody uses" is a gross exaggeration. Maybe you don't use it, but debian-based distros are very popular and GitHub hosted runners (which we also use) are Ubuntu-based. Independent of whether we like it or not.

If this, say, were a fully reproducible Nix-style thing, for which we could run one command and would be guaranteed a reliable repro, that would change my perspective.

Full reproducibility is very difficult to achieve in practice, even with tools like Nix. It sounds great, but I am yet to see a complicated CI system that actually meets this standard. Not to mention migration costs (say if we decided to move everything to nix, which we can't for BPF CI btw because s390x).

There is an alternative some projects use: you write a "CI program" in your language of choice, and then the yaml only triggers the entry point. It's a reasonable approach, but again: migration cost.

In my opinion, and I understand that is somewhat of a hot take, the incentives should be set to test in the form of end-to-end workflow in CI and upstream that. That's the environment that is at least somewhat stable and, more importantly, that we regularly test/run on.

Keeping bash scripts in separate files does not change the requirement to test end-to-end workflow. It's unavoidable anyways. Separate, small, single-responsibility scripts enable an option to test/debug/develop a small part of a workflow locally. Inlining the scripts makes this harder with no benefit in return, IMO.

Now let's turn back to vmlinux.h

If my arguments aren't convincing, I don't want this difference in opinion to be a blocker.

If you're taking ownership of this repo, which you seem to be interested in, then please go ahead and merge as is.

Thanks!

Copy link
Contributor Author

@d-e-s-o d-e-s-o Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it's six lines for which I think it makes no sense to have to context switch for. If it was for me the remaining scripts would be inlined as well.

By "context switch" do you mean opening a different file?

Yep.

Btw "package manager that barely anybody uses" is a gross exaggeration. Maybe you don't use it, but debian-based distros are very popular and GitHub hosted runners (which we also use) are Ubuntu-based. Independent of whether we like it or not.

What I mean is that this is a repository that, at the end of the day, is Meta maintained. Meta doesn't use apt based distros on dev environments to the degree I understand.

Now let's turn back to vmlinux.h

If my arguments aren't convincing, I don't want this difference in opinion to be a blocker.

Sounds good, thanks for sharing your thoughts. Given that it's a small number of lines that are trivial to copy and paste, mostly affecting a package likely already present anyway, let's just move on.

If you're taking ownership of this repo, which you seem to be interested in, then please go ahead and merge as is.

I am making a contribution to a repository that I created, asking for your review because I touched files that you added in my absense and because you seemed to show interest in having a stake in it as well. From my perspective it's a community owned thing, but the community seems to be two Meta people at the moment :-)

Copy link
Contributor

@theihor theihor Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're taking ownership of this repo, which you seem to be interested in, then please go ahead and merge as is.

I am making a contribution to a repository that I created, asking for your review because I touched files that you added in my absense and because you seemed to show interest in having a stake in it as well. From my perspective it's a community owned thing, but the community seems to be two Meta people at the moment :-)

Yes, sorry, I somehow missed the fact that you created this repo :)

What I meant is that if you have strong opinions on how the code should be written it's only fair to expect that you own it.

I think this was a useful discussion, and now that we are aware of each other's opinion the review process should be easier. Feel free to ping me for reviews, if you think it'd be useful.


- name: Install dependencies
shell: bash
run: |
./scripts/install-dependencies.sh
./scripts/install-pahole.sh
./scripts/install-bpftool.sh

- name: x86_64/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh x86_64

- name: aarch64/vmlinux.h
- name: Generate ${{ matrix.arch }}/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh aarch64
run: ./scripts/gen-vmlinux-header.sh ${{ matrix.arch }}

- name: arm/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh arm

- name: loongarch64/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh loongarch64
- name: Upload headers
uses: actions/upload-artifact@v4
with:
name: vmlinux.h-${{ matrix.arch }}
if-no-files-found: error
path: ./vmlinux.h/${{ matrix.arch }}

- name: ppc64le/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh ppc64le
combine-headers:
name: Combine all vmlinux.h headers
runs-on: ubuntu-latest
needs: gen-headers

- name: riscv64/vmlinux.h
shell: bash
run: ./scripts/gen-vmlinux-header.sh riscv64
steps:
- name: Download all artifacts
uses: actions/download-artifact@v4
with:
pattern: vmlinux.h-*

- name: s390x/vmlinux.h
- name: Reorganize headers
shell: bash
run: ./scripts/gen-vmlinux-header.sh s390x
run: |
mkdir -p vmlinux.h
for dir in vmlinux.h-*/; do
arch="${dir#vmlinux.h-}"
arch="${arch%/}"
# Map architecture names to match include/ directory structure
target_arch="$arch"
case "$arch" in
ppc64le) target_arch="powerpc" ;;
esac
mkdir -p "vmlinux.h/$target_arch"
mv "$dir"* "vmlinux.h/$target_arch/"
done

- name: Upload headers
- name: Upload combined headers
uses: actions/upload-artifact@v4
with:
name: vmlinux.h
Expand Down
22 changes: 0 additions & 22 deletions scripts/install-pahole.sh
View file
Open in desktop

This file was deleted.