Factor out incremental build prep into an action

[theihor]

Partially resolves #142

@chantra I've been testing this change in a simplified workflow on my fork.

Any suggestions how to verify kernel-patches/bpf CI would work with the change? I guess we could just push and see, but maybe there is a less disruptive way.

[chantra]

Thanks @theihor

I have not fully reviewed the whole PR yet, but from quick pass, it seems fine so far.

Any suggestions how to verify kernel-patches/bpf CI would work with the change? I guess we could just push and see, but maybe there is a less disruptive way.

Yes, you should create a PR against kernel-patches/vmtest, apply the changes would expect to do when migrating to using the new action, but until this is landed, you will need to use an action path pointing to your repo/branch, e.g

theihor/libbpf-ci/prepare-incremental-build@prepare-incremental-build-action instead of libbpf-ci/{action}@main.

[theihor]

@chantra

For the case of kernel-patches/bpf... the workspace just happens to be a linux repo. For other projects, we do download the sources in the step above and move it in place, so we end up being lucky and having the workspace being a linux repo too.

I believe, if we had the action working properly, we should be able to skip the "move linux source in place" step. I think this is out of scope for this task, but somethign to keep in mind for later to make the action self-contained.

In the simplified workflow where I was testing this action, I deliberately checked out kernel-patches/bpf into a non-workspace directory, and then passed a path to that directory as action input (see here).

So, assuming I haven't missed anything, the action doesn't care how the linux source tree directory was created: via checkout or via "download linux source" steps. As long as the correct path to the git repo is passed, the action will try finding the cached build output.

Finally, lets take the opportunity when working on actions to write a README.md in the action directory to document how to use it. Similarly to how actions in the marketplace do: https://github.com/actions/checkout/blob/main/README.md for instance.

Will do.

[chantra]

In the simplified workflow where I was testing this action, I deliberately checked out kernel-patches/bpf into a non-workspace directory, and then passed a path to that directory as action input (see here).

So, assuming I haven't missed anything, the action doesn't care how the linux source tree directory was created: via checkout or via "download linux source" steps. As long as the correct path to the git repo is passed, the action will try finding the cached build output.

yeah, I think we are missing something but it silently fails. In your case, no cache was available: https://github.com/theihor/libbpf-ci/actions/runs/10836721377/job/30071091347#step:4:63 What I am afraid is that at best we would download the cache but in an directory which is not accessible when we do the build. It is probably easier to test/validate with a actions/cache basic workflow.

[theihor]

yeah, I think we are missing something but it silently fails. In your case, no cache was available: https://github.com/theihor/libbpf-ci/actions/runs/10836721377/job/30071091347#step:4:63 What I am afraid is that at best we would download the cache but in an directory which is not accessible when we do the build. It is probably easier to test/validate with a actions/cache basic workflow.

In that workflow there was nothing to cache actually... I wrote another one, and there you can see the cache was

• created by https://github.com/theihor/libbpf-ci/actions/runs/10857913112/job/30135143759
• and then restored in subsequent run: https://github.com/theihor/libbpf-ci/actions/runs/10857937779/job/30135210069

Btw, do we want workflows like that (created specifically to test a custom action) in libbpf/ci? I am not sure.

[chantra]

Btw, do we want workflows like that (created specifically to test a custom action) in libbpf/ci? I am not sure.

Maybe not using the key used by the kernel. But given the hard time we have to validate it works as expected, I think it could be useful to have some integration workflows to avoid regression in the future.

[theihor]

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf.

I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66

Do you know if there is a reason for having two copies?

[chantra]

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf.

I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66

Do you know if there is a reason for having two copies?

No, I don't think so. The artifact is downloadable from GH (when going to the summary of the run, there is a link to the artifacts). The content of vmlinux-x86_64-gcc is https://gist.github.com/chantra/0acb0ec04a58db3adea2debd47e73fee

I think it could be moved in place. I know that we keep the "Makefile"s so later we can run make -s image_name to discover where the vmlinux file is. The rest, I am not too sure. Even in the selftests/bpf directory we may not need to keep everything that is compiled. At least not for the "artifact" that we re-use during the test run. We do need to keep it for when we update the cache though.

[theihor]

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf. I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66 Do you know if there is a reason for having two copies?

No, I don't think so. The artifact is downloadable from GH (when going to the summary of the run, there is a link to the artifacts). The content of vmlinux-x86_64-gcc is https://gist.github.com/chantra/0acb0ec04a58db3adea2debd47e73fee

I think it could be moved in place. I know that we keep the "Makefile"s so later we can run make -s image_name to discover where the vmlinux file is. The rest, I am not too sure. Even in the selftests/bpf directory we may not need to keep everything that is compiled. At least not for the "artifact" that we re-use during the test run. We do need to keep it for when we update the cache though.

I believe on kernel-patches/bpf CI only one of those two selftests/bpf copies is present. For example build job artifacts from this bpf-ci run contain only this inside:

$ tree -L 2
.
├── kbuild-output
│   ├── arch
│   ├── include
│   └── vmlinux
└── selftests
    └── bpf

And the flag controlling what's included in the tarball depends on the repository name.

So the question is why additional files are needed on kernel-patches/vmtest, but not on kernel-patches/bpf. I'll try to figure that out. If it's only for make image_name it seems silly, as we should be able to pass that name around as an input or env variable.

LGTM. Before this lands, do you mind testing on the kernel-patches/vmtest repo by using this action, just to make sure we did not missed anything.

I already did: https://github.com/kernel-patches/vmtest/pull/287

I tagged you there, but wasn't able to add you as a reviewer.

[chantra]

So the question is why additional files are needed on kernel-patches/vmtest, but not on kernel-patches/bpf. I'll try to figure that out. If it's only for make image_name it seems silly, as we should be able to pass that name around as an input or env variable.

make image_name only needs the Makefiles, and I don't think it needs anything related to selftests. So I think this is probably unrelated and just ended up there as an accident. As much as this PR is concerned, I think we should leave it like this, and follow-up in a separate PR.

I already did: kernel-patches/vmtest#287

I tagged you there, but wasn't able to add you as a reviewer.

Ha great. Thanks. If this works, then that's a high signal that everything should be fine :D

[chantra]

Feel free to merge this whenever you want, we may want to keep an eye on both kernel-patches/bpf and libbpf/libbpf after it landed just to be safe.

[theihor]

Feel free to merge this whenever you want, we may want to keep an eye on both kernel-patches/bpf and libbpf/libbpf after it landed just to be safe.

Ok. Thank you for the review.

I'd like BPF CI to become stable before merging this.