ci: Add Rust for Linux

[ojeda]

Rust for Linux, so far, has pinned the Rust compiler and bindgen versions. The kernel is looking into expanding that support to several versions, i.e. establishing a minimum supported version, so that the kernel can start to be more easily built. In particular, it should be possible to build the kernel using the tools provided directly by Linux distributions. In order to help achieve that goal, the Rust project has added the kernel to its Rust pre-merge CI.

This commit does the same for bindgen. In particular, it adds a quick, build-only test of the Rust code in the kernel as an extra step in the test workflow.

This is intended to be an end-to-end test that runs what kernel developers/users would do. In particular, it is useful to catch certain issues that go beyond the C header comparisons. For instance, it would have been able to catch an issue like the --version option unexpectedly requiring a header in 0.69.0 (fixed in 0.69.1) [1].

It would also have detected another issue present in 0.66.0 and 0.66.1: a panic handling certain C headers with string literals containing an interior NUL [2]. While the kernel is not really a stable test, and such an issue would still require that a proper test is added, it is nevertheless a good test case of non-trivial C headers that may trigger edge cases like that.

Of course, bindgen may need to disable the test for different reasons, i.e. there is no expectation to block any urgent/important PR, and the kernel can also call bindgen differently depending on the version, i.e. we are happy to adjust on our side too. Even if it gets disabled often, we would still be in a better situation than not having the test at all.

The Linux version (hash or tag) should ideally be updated from time to time (e.g. every kernel -rc1), and each update should only contain that change.

Link: #2678 [1]
Link: #2567 [2]

[ojeda]

Self-review with some comments for reference.

[ojeda]

If someone knows a better way to handle this that includes --target and debug/release, please let me know!

[ojeda]

Also, I checked in one of my runs that we are actually using the just-built bindgen, but it may be nice to double-check somehow that we are actually using that one and not the other that is already installed in .cargo. I didn't see e.g. the Git hash printed by bindgen in --version --verbose, but that could be a nice way (like in rustc).

[ojeda]

Perhaps a single additional job should do this. In any case, by doing it in test.sh, it is easy to add it to existing ones or not as needed (and it was nice to test that it actually worked in all the variations of the matrix enabled here).

Also, I disable it under feature_extra_asserts since it seems to take too long / does not finish.

[pvdrz]

Hi Miguel, do you have any estimates on the time it takes to run this new workflow?

[ojeda]

If you mean adding it to the existing jobs, then taking a look at the CI run here and comparing it with the latest, it adds about a minute or two (i.e. to each job in the matrix that enables BINDGEN_RUST_FOR_LINUX_TEST).

If you mean moving it to a new job, then it may run in parallel with the existing jobs (depending on how GitHub handles it). The total time of the new job is similar to existing ones (i.e. 2-3 minutes), as long as one disables BINDGEN_MAIN_TESTS and the other tests above (cargo check, cargo test), since they would be run in the other part of the matrix anyway. So it could be that the total time of the CI does not grow much, depending on GitHub, although we lose doing this end-to-end test with e.g. debug etc.

[ojeda]

(Edited the message above to add some more details.)

Which one is the best approach I guess depends on how much you care about the extra minute(s) in existing tests and whether you think running this new part under e.g. debug is worth it etc. I am happy to send the "separate job" one where I added a new BINDGEN_* variable to skip the other bits if needed.

[pvdrz]

I think we can live with an extra couple minutes here and there, what do you think @emilio? I don't have a strong preference about having this on a completely new job or not. The only question I have is if we want to include this job as a required green check for every PR or if we would be ok with it failing from time to time and fix bindgen before each release.

[ojeda]

This could be reduced in scope a bit more, but I just did what we are currently doing in the Rust CI, which seemed reasonable (and they should notice any issue there first).

[ojeda]

Similarly, I used the same hash here as in the Rust CI, though we could use a tag already if needed.

[ojeda]

In e.g. https://github.com/rust-lang/rust-bindgen/actions/runs/9568431855/job/26378640703?pr=2851 you can see at the bottom:

  RUSTC L rust/core.o
  BINDGEN rust/bindings/bindings_generated.rs
  BINDGEN rust/bindings/bindings_helpers_generated.rs
  BINDGEN rust/uapi/uapi_generated.rs
  EXPORTS rust/exports_core_generated.h
  RUSTC P rust/libmacros.so
  RUSTC L rust/compiler_builtins.o
  RUSTC L rust/alloc.o
  RUSTC L rust/bindings.o
  RUSTC L rust/build_error.o
  RUSTC L rust/uapi.o
  EXPORTS rust/exports_alloc_generated.h
  EXPORTS rust/exports_bindings_generated.h
  RUSTC L rust/kernel.o
  EXPORTS rust/exports_kernel_generated.h
  RUSTDOC TK rust/kernel/lib.rs
  RUSTC [M] samples/rust/rust_minimal.o
  RUSTC     samples/rust/rust_print.o
  RUSTC     drivers/net/phy/ax88796b_rust.o

[emilio]

It seems you wouldn't need RUSTC_BOOTSTRAP etc if you use CARGO_TARGET_DIR=$HOME/.bindgen or so?

https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads

[emilio]

Apparently the --target-dir flag is also not unstable.

[emilio]

Ah, ok so I see what you're doing here, you just want to make sure you get the target-dir/debug/ or release right...

Maybe given we're only interested on running this on linux (so no explicit --target), and we know whether we're doing a release build (via BINDGEN_RELEASE_BUILD), maybe just cargo build $CARGO_ARGS, and then either conditionally add target/debug or target/release to the PATH, or maybe just add both to the PATH?

[ojeda]

Yeah, I did this to be able to use whatever you had in $CARGO_ARGS (and cargo install does not accept --release), and it seemed like the clean way, but sadly unstable.

The --target should not matter for this CI as you say, but I guess you could be adding different profiles in the future. If that is not a concern, then simply adding both debug/release to $PATH sounds the simplest.

(It could still "break" or be surprising if other things get refactored and both debug and release get built in the same test for something else above. We could use --target-dir to at least isolate that).

[ojeda]

v2: instead of the unstable --out-dir, put both the common profiles in $PATH and use --target-dir to isolate a bit.

(Using --target-dir implies we rebuild more than needed, since some crates may be already built by the previous tests)

[pvdrz]

@ojeda would be reasonable to run this job on a cron, let's say, once every week?

[ojeda]

Up to you of course -- did something change?

If the extra couple minutes would be too much even in a new job, then a periodic post-merge CI is better than nothing. The disadvantage would be that one would find the issues after the fact, which is more painful overall (e.g. may require figuring out which commit actually broke it and whether a revert is needed and whether that breaks something else, the PR author may not be around to help, etc.).