Skip to content

CI: improvements#271

Draft
thackara wants to merge 3 commits into
rapido-linux:masterfrom
thackara:ci_improvements
Draft

CI: improvements#271
thackara wants to merge 3 commits into
rapido-linux:masterfrom
thackara:ci_improvements

Conversation

@thackara
Copy link
Copy Markdown
Contributor

@thackara thackara commented May 25, 2026

Raising this PR to get inputs.

  • Add lint job with shellcheck, rustfmt, and clippy to CI
  • Thinking of if cut scripts changed|added, then run rapido cut on that script, yes or no?

@thackara thackara force-pushed the ci_improvements branch 2 times, most recently from 87f6314 to 8dfe17f Compare May 25, 2026 07:37
Copy link
Copy Markdown
Collaborator

@ddiss ddiss left a comment

Choose a reason for hiding this comment

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

Thanks Sangeetha. The changes look fine, with a couple of minor suggestions below

Comment thread .github/workflows/ci.yml Outdated
steps:
- uses: actions/checkout@v4
- name: Install shellcheck
run: sudo apt-get update -y && sudo apt-get install -y shellcheck
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Assuming these steps are all run sequentially, I'd prefer to install all dependencies via a single apt-get invocation at the start.

Comment thread .github/workflows/ci.yml
- name: Install shellcheck
run: sudo apt-get update -y && sudo apt-get install -y shellcheck
- name: Run shellcheck
run: shellcheck --severity=error autorun/*.sh cut/*.sh selftest/*.sh rapido
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've had some frustrating encounters with shellcheck in the past. Let's try it out, but I might be inclined to drop it if it gets in the way too much :)

Comment thread .github/workflows/ci.yml Outdated
- name: Rust format check
run: git ls-files "*.rs" | grep -v "^src/third_party" | xargs rustfmt --check
- name: Rust clippy
run: cargo-1.85 clippy --offline -- -D warnings -A clippy::byte_char_slices
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with clippy. Let's try it and perhaps drop it in future if there's too much noise.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This clippy call is currently failing with:

error[E0514]: found crate `cpio` compiled by an incompatible version of rustc
   --> src/bin/rapido-vm.rs:121:30
    |
121 |     let mut archive_walker = cpio::archive_walk(reader)?;
    |                              ^^^^
    |
    = note: the following crate versions were found:
            crate `cpio` compiled by <unknown rustc version>: /home/runner/work/rapido/rapido/target/debug/deps/libcpio-fbbf5d2c2d9b4b42.rmeta
    = help: please recompile that crate using this compiler (rustc 1.95.0 (59807616e 2026-04-14)) (consider running `cargo clean` first)

error[E0514]: found crate `kv_conf` compiled by an incompatible version of rustc
   --> src/bin/rapido-init.rs:374:22
    |
374 |     let conf = match kv_conf::kv_conf_process(&mut reader) {
    |                      ^^^^^^^
    |
    = note: the following crate versions were found:
            crate `kv_conf` compiled by <unknown rustc version>: /home/runner/work/rapido/rapido/target/debug/deps/libkv_conf-7b5f861fa61d0198.rmeta
    = help: please recompile that crate using this compiler (rustc 1.95.0 (59807616e 2026-04-14)) (consider running `cargo clean` first)

Any ideas why? Will the lint and test CI jobs run in parallel, if so, can this cause any races?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can they race?

No.

these jobs run in parallel by default, but on different virtual machines!

2026-05-25T07:37:58.4263155Z Worker ID: {524b6141-0d0b-45d2-a84c-d2a791010309}
2026-05-25T07:37:58.4264098Z Azure Region: eastus2

2026-05-25T07:37:58.5172853Z Worker ID: {0becdb66-54e9-48ec-961c-d41e4149dd95}
2026-05-25T07:37:58.5173536Z Azure Region: centralus

Comment thread autorun/lib/fstests.sh
for i in $(ls /sys/block); do
_ser="$(cat /sys/block/${i}/serial 2>/dev/null)" ||
_ser="$(cat /sys/block/${i}/device/serial 2>/dev/null)" ||
for i in /sys/block/*; do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I prefer these glob based iterators, but I think we need to explicitly set
shopt -s nullglob to avoid Bash's silly empty glob behaviour, no?
We should do this once in vm_autorun.env to ensure that it's set for all autorun scripts.

Comment thread autorun/fcoe_local.sh Outdated
break
fi
sleep 1
done
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's some white-space damage above. Please use tabs.

Comment thread autorun/fstests_ntfs3.sh Outdated
# create mkfs symlink for xfstests, which assumes mkfs.$FSTYPE
mkfsbin="$(type -P mkfs.ntfs)"
ln -s "${mkfsbin}" "${mkfsbin}3" || break
ln -s "${mkfsbin}" "${mkfsbin}3" || exit 1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use _fatal instead of exit

Comment thread selftest/selftest.sh
for t in $(ls "selftest/test/"${filter}); do
if ! [ -x "${t}" ]; then
echo "$t skipped"
for t in selftest/test/${filter}*; do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

t="${t##*/}" here, to avoid the subsequent changes?

assert_eq!(
mod_a.rel_path,
"kernel/mod_a.ko",
mod_a.rel_path, "kernel/mod_a.ko",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

strange, I thought rust fmt put everything on one line, or split lines at every ,. Oh well

thackara added 2 commits June 2, 2026 10:05
address failures caught by `shellcheck`

|
| In autorun/fcoe_local.sh line 121:
| 	while [ ! -d /sys/class/fc_remote_ports/rport-* ]; do
|                      ^-- SC2144 (error): -d doesn't work with globs. Use a for loop.
|
|
| In autorun/fcoe_local.sh line 133:
| 				for b in "${target}/${hctl}/block/*"; do
|                                          ^-------------------------^ SC2066 (error): Since you double quoted this, it will not word split, and the loop will only run once.
|
|
| In autorun/fstests_ntfs3.sh line 11:
| ln -s "${mkfsbin}" "${mkfsbin}3" || break
|                                     ^---^ SC2105 (error): break is only valid in loops.
|
|
| In autorun/ocfs2.sh line 39:
| for i in $(ls /sys/block); do
|          ^--------------^ SC2045 (error): Iterating over ls output is fragile. Use globs.
|
|
| In autorun/openiscsi.sh line 39:
| for i in $(ls /etc/iscsi/nodes/*/*/default); do
|          ^-- SC2045 (error): Iterating over ls output is fragile. Use globs.
|
|
| In cut/fstests_xfs.sh line 17:
| [[ ${man_deps[@]} == ${man_deps[@]%.gz} ]] || man_deps+=(zcat gzip)
|    ^------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|                      ^----------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|
|
| In cut/fstests_xfs.sh line 18:
| [[ ${man_deps[@]} == ${man_deps[@]%.bz2} ]] || man_deps+=(bzcat)
|    ^------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|                      ^-----------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|
|
| In cut/fstests_xfs.sh line 19:
| [[ ${man_deps[@]} == ${man_deps[@]%.xz} ]] || man_deps+=(xzcat)
|    ^------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|                      ^----------------^ SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
|
|
| In selftest/selftest.sh line 60:
| 	for t in $(ls "selftest/test/"${filter}); do
|                  ^-----------------------------^ SC2045 (error): Iterating over ls output is fragile. Use globs.
|
| For more information:
|   https://www.shellcheck.net/wiki/SC2045 -- Iterating over ls output is fragi...
|   https://www.shellcheck.net/wiki/SC2066 -- Since you double quoted this, it ...
|   https://www.shellcheck.net/wiki/SC2105 -- break is only valid in loops.

Changes include:
- Replace `ls` output with safe bash globs SC2045
- Fixed path resolution when migrating away from `ls` basenames.
- Fixed `break` (SC2105).
- Replaced implicit array concatenation in `[[ ]]` tests with explicit
  expansion syntax (SC2199).
- Replaced unsafe dir glob inside `[ -d ]` SC2144.
- Fixed SC2166.

Signed-off-by: Sangeetha Thackarajan <sangeetha.thackarajan@suse.com>
this is purely formatting change with no functional alterations (done in
preparation for new CI/lint job).
cmd: `git ls-files "*.rs" | grep -v "^src/third_party" | xargs rustfmt`

Signed-off-by: Sangeetha Thackarajan <sangeetha.thackarajan@suse.com>
@thackara thackara force-pushed the ci_improvements branch 2 times, most recently from 25add3a to 8bf8b71 Compare June 2, 2026 05:42
Signed-off-by: Sangeetha Thackarajan <sangeetha.thackarajan@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants