Check vmlinux.h header up-to-dateness

[d-e-s-o]

Check that vmlinux.h headers in the repository are up-to-date with
respect to the checked in config. Doing so should make it clear to pull
request submitters that they should update headers after update of the
config (or targeted kernel version or similar).

We don't want to completely automate away the update, to have a human in
the loop to double check sanity, e.g., for cases where config values get
renamed.

[d-e-s-o]

This depends on #14, the commits of which are included here.

[d-e-s-o]

Here is a test run that includes #13

[theihor]

I think a single PR with all relevant commits would be easier to follow. Maybe update summary here, and let's close the other PRs?

[d-e-s-o]

Closed #14 and updated this one.

[theihor]

"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.

[theihor]

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.

[d-e-s-o]

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.

[theihor]

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!

[d-e-s-o]

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 :-)

[theihor]

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.