Skip to content

Make compilation to fail on warnings #1104

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

Open
sarroutbi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make compilation to fail on warnings #1104

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
28 changes: 28 additions & 0 deletions .github/workflows/compile-no-warnings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
name: Rust
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


"on":
push:
branches: [master]
pull_request:
branches: [master]

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
Comment on lines +23 to +24
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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 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
Comment on lines +10 to +28
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

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

Comment on lines +27 to +28
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

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

Suggested change
- 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.