Make compilation to fail on warnings#1104
Conversation
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
from
fe3610d to
1ffaa0c
Compare
|
This will be kept until compilation warnings are fixed in |
sarroutbi
changed the title
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
3 times, most recently
from
a3b1702 to
6da554c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
2 times, most recently
from
5a00dd1 to
0672ac6
Compare
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
from
0672ac6 to
18e6de8
Compare
sarroutbi
changed the title
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
from
18e6de8 to
6644542
Compare
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
2 times, most recently
from
db03739 to
dede790
Compare
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
2 times, most recently
from
c7f35d2 to
6ac9052
Compare
Signed-off-by: Sergio Arroutbi <sarroutb@redhat.com>
sarroutbi
force-pushed
the
202509030938-fail-compilation-on-warnings
branch
from
6ac9052 to
690f817
Compare
📝 WalkthroughWalkthroughA new GitHub Actions workflow is added to compile Rust code and run tests on the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/compile-no-warnings.yml:
- Around line 10-28: Remove this separate compile-no-warnings workflow and fold
its checks into the existing Rust workflow by adding a new job (or extra steps
in the existing tests job) that runs the same commands (the basic-compilation
job’s steps: RUSTFLAGS="-D warnings" cargo build, RUSTFLAGS="-D warnings" cargo
test, cargo clippy --all-features --all-targets -- -D clippy::all -D warnings,
and the clippy tests invocation) and reuse the same container configuration and
triggers as rust.yml; ensure the job name is unique (not duplicating the "Rust"
workflow display name), preserve the RUSTFLAGS/clippy flags and matrix/runner
settings from compile-no-warnings, and then delete compile-no-warnings.yml to
avoid duplicate checkouts and CI minutes.
- Line 2: The workflow's display name "Rust" collides with the existing
rust.yml; update the workflow name value (the YAML top-level name field
currently set to "Rust") to a unique, descriptive string such as "No-Warnings
Compilation" so it no longer conflicts with the existing rust workflow and
produces unambiguous status checks in the PR UI.
- Around line 23-24: The step currently runs `RUSTFLAGS="-D warnings" cargo
test`, which executes tests; change it to run compile-only by adding the
`--no-run` flag so the job only checks that test code compiles without warnings.
Update the step that invokes `cargo test` (the command string RUSTFLAGS="-D
warnings" cargo test) to use RUSTFLAGS="-D warnings" cargo test --no-run so it
does not run the tests.
- Around line 27-28: The CI step named "Check clippy test errors (no warnings
included)" is redundant because the command cargo clippy --all-features
--all-targets --tests -- -D clippy::all -D warnings duplicates the coverage of
the earlier clippy invocation; remove this step or change its flags so it
actually differs (for example, keep the step but remove --all-targets and use
--tests only, or remove --tests and run a more specific invocation without
--all-features), ensuring the unique step name and the command string are
updated accordingly so CI does not re-run identical linting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab53e4f6-eb3f-432c-8fd1-a2481aa52e7d
📒 Files selected for processing (1)
.github/workflows/compile-no-warnings.yml
| @@ -0,0 +1,28 @@ | |||
| --- | |||
| name: Rust | |||
There was a problem hiding this comment.
Workflow name collides with existing rust.yml.
The existing .github/workflows/rust.yml also uses name: Rust. Having two workflows with the same display name makes status checks and PR UI ambiguous. Consider a distinct name such as No-Warnings Compilation.
-name: Rust
+name: No-Warnings Compilation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Rust | |
| name: No-Warnings Compilation |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/compile-no-warnings.yml at line 2, The workflow's display
name "Rust" collides with the existing rust.yml; update the workflow name value
(the YAML top-level name field currently set to "Rust") to a unique, descriptive
string such as "No-Warnings Compilation" so it no longer conflicts with the
existing rust workflow and produces unambiguous status checks in the PR UI.
| env: | ||
| CARGO_TERM_COLOR: always | ||
|
|
||
| jobs: | ||
| basic-compilation: | ||
| name: Compilation with no warnings | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: quay.io/keylime/keylime-ci:latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Check compilation (no warnings) | ||
| run: RUSTFLAGS="-D warnings" cargo build | ||
| - name: Check test compilation (no warnings) | ||
| run: RUSTFLAGS="-D warnings" cargo test | ||
| - name: Check clippy errors (no warnings included) | ||
| run: cargo clippy --all-features --all-targets -- -D clippy::all -D warnings | ||
| - name: Check clippy test errors (no warnings included) | ||
| run: cargo clippy --all-features --all-targets --tests -- -D clippy::all -D warnings |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider folding these checks into the existing rust.yml workflow.
rust.yml already targets the same triggers and uses the same container for tests. Maintaining two workflows named "Rust" with overlapping checkouts/containers increases CI minutes and cognitive overhead. A single workflow with additional jobs (or additional steps in the existing tests job) would be simpler and keep RUSTFLAGS/clippy policy consistent across the repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/compile-no-warnings.yml around lines 10 - 28, Remove this
separate compile-no-warnings workflow and fold its checks into the existing Rust
workflow by adding a new job (or extra steps in the existing tests job) that
runs the same commands (the basic-compilation job’s steps: RUSTFLAGS="-D
warnings" cargo build, RUSTFLAGS="-D warnings" cargo test, cargo clippy
--all-features --all-targets -- -D clippy::all -D warnings, and the clippy tests
invocation) and reuse the same container configuration and triggers as rust.yml;
ensure the job name is unique (not duplicating the "Rust" workflow display
name), preserve the RUSTFLAGS/clippy flags and matrix/runner settings from
compile-no-warnings, and then delete compile-no-warnings.yml to avoid duplicate
checkouts and CI minutes.
| - name: Check test compilation (no warnings) | ||
| run: RUSTFLAGS="-D warnings" cargo test |
There was a problem hiding this comment.
cargo test executes tests; use --no-run for compile-only checks.
The job is titled "Compilation with no warnings" and the step is named "Check test compilation", but cargo test compiles and runs the full test suite. Since tests/run.sh in the existing rust.yml already executes tests, this is duplicated work and may also fail for non-warning reasons (flaky tests, environment). If the intent is only to verify test-code compiles without warnings, use --no-run.
- - name: Check test compilation (no warnings)
- run: RUSTFLAGS="-D warnings" cargo test
+ - name: Check test compilation (no warnings)
+ run: RUSTFLAGS="-D warnings" cargo test --no-run🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/compile-no-warnings.yml around lines 23 - 24, The step
currently runs `RUSTFLAGS="-D warnings" cargo test`, which executes tests;
change it to run compile-only by adding the `--no-run` flag so the job only
checks that test code compiles without warnings. Update the step that invokes
`cargo test` (the command string RUSTFLAGS="-D warnings" cargo test) to use
RUSTFLAGS="-D warnings" cargo test --no-run so it does not run the tests.
| - name: Check clippy test errors (no warnings included) | ||
| run: cargo clippy --all-features --all-targets --tests -- -D clippy::all -D warnings |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant clippy invocation.
--all-targets (line 26) already includes test targets, so this second step re-runs the same lints. Adding --tests on top of --all-targets does not broaden coverage—it's effectively duplicated work and CI time. Drop this step, or replace it with something that actually differs (e.g., without --all-features, or a specific target).
♻️ Proposed fix
- - name: Check clippy test errors (no warnings included)
- run: cargo clippy --all-features --all-targets --tests -- -D clippy::all -D warnings📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Check clippy test errors (no warnings included) | |
| run: cargo clippy --all-features --all-targets --tests -- -D clippy::all -D warnings |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/compile-no-warnings.yml around lines 27 - 28, The CI step
named "Check clippy test errors (no warnings included)" is redundant because the
command cargo clippy --all-features --all-targets --tests -- -D clippy::all -D
warnings duplicates the coverage of the earlier clippy invocation; remove this
step or change its flags so it actually differs (for example, keep the step but
remove --all-targets and use --tests only, or remove --tests and run a more
specific invocation without --all-features), ensuring the unique step name and
the command string are updated accordingly so CI does not re-run identical
linting.
3 participants
The new workflow runs on every push and pull request to the master branch and performs the following checks:
Both steps are executed with the
RUSTFLAGS="-D warnings"environment variable. This flag instructs the Rust compiler to treat all warnings as fatal errors, which will cause the build to fail if any warnings are present. This helps prevent latent bugs and ensures the code base remains clean.These are some reasons to enforce a "No Warnings" Policy:
Future-Proofing Code: A warning in the current version of a compiler might become a hard error in a future version. Code that compiles with warnings today may fail to compile tomorrow after a simple rustup update, causing unexpected build failures. Enforcing a "no warnings" policy ensures your code is robust against compiler updates.
Preventing Latent Bugs: Many warnings are not just stylistic suggestions; they often point to subtle logical flaws, potential null pointer dereferences, race conditions, or undefined behavior. A classic example is an "unused variable" warning, which can indicate that a necessary calculation was performed but its result was never used.
Improving Code Readability and Reducing Noise: When a build produces dozens of warnings, developers become desensitized to them. This "warning fatigue" can cause them to miss a new, critical warning that appears among the noise. A clean, warning-free build ensures that any new message from the compiler gets immediate attention.
Maintaining a Culture of Quality: A strict "no warnings" policy sets a high standard for the team. It encourages developers to write clean, precise, and intentional code, fostering a culture where quality is not negotiable.
Summary by CodeRabbit