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.

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

Jump to bottom

ci: Add Rust for Linux #2851

Merged
merged 1 commit into from
Sep 19, 2024
Merged

ci: Add Rust for Linux #2851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .github/workflows/bindgen.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ jobs:
BINDGEN_FEATURE_RUNTIME: ${{matrix.feature_runtime}}
BINDGEN_FEATURE_EXTRA_ASSERTS: ${{matrix.feature_extra_asserts}}
BINDGEN_NO_DEFAULT_FEATURES: ${{matrix.no_default_features}}
BINDGEN_RUST_FOR_LINUX_TEST: ${{matrix.os == 'ubuntu-latest' && matrix.llvm_version == '16.0' && matrix.feature_extra_asserts == 0 && 1 || 0}}
Copy link
Contributor Author

@ojeda ojeda Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ojeda ojeda Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

run: ./ci/test.sh

check-cfg:
Expand Down
76 changes: 76 additions & 0 deletions ci/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,79 @@ assert_no_diff

# Run the integration tests
(cd bindgen-integration && cargo test $CARGO_ARGS)

if [ "$BINDGEN_RUST_FOR_LINUX_TEST" == "1" ]; then
# Run the Rust for Linux test
#
# This is intended to be an end-to-end test that runs what Linux kernel
# developers/users would do. It is a quick, build-only test of the Rust code
# in the Linux kernel.

# Put LLVM binaries in the path for `LLVM=1`. The LLVM `bin` directory should
# go first since there are others in the Ubuntu image.
export PATH="${LLVM_DIRECTORY}/bin:${PATH}"

# Kernel build dependency: `bindgen-cli`, which is under test.
#
# Using `cargo build` (and adding the two common profiles to the `$PATH`) so
# that we can use `$CARGO_ARGS` as is, since `cargo install` does not support
# `--release`. `--target-dir` is used to isolate from other possible tests.
# A cleaner alternative is using `--out-dir`, but it is unstable.
(cd bindgen-cli && cargo build --target-dir ${HOME}/.bindgen $CARGO_ARGS)
export PATH="${HOME}/.bindgen/release:${HOME}/.bindgen/debug:${PATH}"

# Kernel build dependency: `libelf-dev`.
sudo apt-get update
sudo apt-get install libelf-dev

# Kernel build dependency: the Rust standard library sources.
#
# `rustup` is used here to install the `rust-src` component (instead of using
# `actions-rs/toolchain`'s `components` option in the workflow step) since we
# only need it for this test, and anyway the action installs `rustup`.
rustup component add rust-src

# Ideally this should be updated from time to time (e.g. every kernel `-rc1`),
# and each update should only contain this change.
#
# Both commit hashes and tags are supported.
LINUX_VERSION=c13320499ba0efd93174ef6462ae8a7a2933f6e7
Comment on lines +156 to +157
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


# Download Linux at a specific commit
mkdir -p linux
git -C linux init
git -C linux remote add origin https://github.com/torvalds/linux.git
git -C linux fetch --depth 1 origin ${LINUX_VERSION}
git -C linux checkout FETCH_HEAD

# Configure Rust for Linux
cat <<EOF > linux/kernel/configs/rfl-for-bindgen-ci.config
# CONFIG_WERROR is not set
Comment on lines +166 to +168
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


CONFIG_RUST=y

CONFIG_SAMPLES=y
CONFIG_SAMPLES_RUST=y

CONFIG_SAMPLE_RUST_MINIMAL=m
CONFIG_SAMPLE_RUST_PRINT=y

CONFIG_RUST_PHYLIB_ABSTRACTIONS=y
CONFIG_AX88796B_PHY=y
CONFIG_AX88796B_RUST_PHY=y

CONFIG_KUNIT=y
CONFIG_RUST_KERNEL_DOCTESTS=y
EOF

make -C linux LLVM=1 -j$(($(nproc) + 1)) \
rustavailable \
defconfig \
rfl-for-bindgen-ci.config

# Build Rust for Linux
make -C linux LLVM=1 -j$(($(nproc) + 1)) \
samples/rust/rust_minimal.o \
samples/rust/rust_print.o \
drivers/net/phy/ax88796b_rust.o
fi
Loading