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.

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

Use -D warnings in all relevant CI #17011

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Dec 28, 2024

Objective

Fixes #17009

See: https://doc.rust-lang.org/stable/clippy/continuous_integration/index.html

Solution

Add the env

Testing

CI should start to fail, then I'll fix it.

Showcase

image

@BenjaminBrienen BenjaminBrienen self-assigned this Dec 28, 2024
@BenjaminBrienen BenjaminBrienen added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 28, 2024
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Dec 29, 2024
@alice-i-cecile
Copy link
Member

I broadly agree with this: deny warnings is the standard practice for Rust + Bevy, and blocking CI is the best way to ensure that these are fixed and stay fixed.

@BenjaminBrienen
Copy link
Contributor Author

BenjaminBrienen commented Dec 29, 2024

An alternative direction that we could take is to deny warnings in CI except for unused imports, variables, and muts. This might help keep verbosity down while still catching important lints. If we do that though, I would expect all the cfg stuff to be removed in all existing places we already do do that. I don't like this though because I don't want irrelevant warnings when developing locally and there isn't enough granularity with allowing warnings in unchanged code.

@alice-i-cecile
Copy link
Member

I don't like this though because I don't want irrelevant warnings when developing locally and there isn't enough granularity with allowing warnings in unchanged code.

Agreed. Half of the reason why these lints exist are to provide a better local development experience.

@mockersf
Copy link
Member

mockersf commented Dec 30, 2024

OK to add -D warnings to CI, but I would prefer the warnings in task pool creation to be solved in a way that keep their builder for wasm and not wasm common

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 30, 2024
@BenjaminBrienen
Copy link
Contributor Author

I can do that

@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 31, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 31, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 31, 2024
Merged via the queue into bevyengine:main with commit 4460a4d Dec 31, 2024
29 of 30 checks passed
@BenjaminBrienen BenjaminBrienen removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 31, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 31, 2024
Small follow-up to #17011 

Please don't merge until the CI passes all checks
github-merge-queue bot pushed a commit that referenced this pull request Jan 3, 2025
# Objective

I missed a couple checks in #17011 

## Solution

Add env

## Testing

CI
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
Small follow-up to bevyengine#17011 

Please don't merge until the CI passes all checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build warnings with certain feature combinations
4 participants