Run clippy in the CI with warnings disallowed #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit ad85f13 ("Seems like edition in workspace isn't applied? Update all to 2021") found that `workspace.edition` isn't a valid key and added it to most crates' individual `[package]` table, but didn't remove the key from `[workspace]` nor added it to the `compile` and `runtime` crates. Note that `[workspace.package] edition = "2021"` exists since Rust 1.64, but still requires `[package] edition.workspace = true` to be repeated in every individual `Cargo.toml` for it to be inherited: https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
As announced earlier in Twinklebear#20 when introducing `BindgenOptions`, upstream `bindgen` didn't yet allow cloning nor copying their `Builder` struct, limiting it to generate a single binding per `Builder` object. This doesn't fit with ispc-rs's `Config` struct which explicitly allows multiple libraries to be compiled using (partially) the same base configuration (barring any side-effects from `fn compile(&mut self, lib)` updating various fields depending on `lib`). This issue has since been addressed upstream, and now allows us to move and clone the `bindgen::Builder` around instead of implementing a very limited subset of config fields and manually reconstructing the builder every time.
As discussed previously it'd be nice to keep `ispc-rs` "free" of simple "code smells" commonly pointed out by `clippy`. This is easy to guard by disallowing any clippy warnings from trickling into the codebase through PRs via an extra CI check. Downside is that Rust upgrades every 6 weeks and usually enables new or improves existing lints, resulting in unrelated CI errors caused by existing code being punted on unsuspecting contributors in newly opened PRs. But these are typically easy to resolve in a separate PR, or to be caught early when (also) running the CI job on a schedule.
|
Thanks @MarijnS95 , this looks good! I think it makes sense to be strict in CI to catch warnings and fix them early. I'll have to look into how to run the GH actions pipeline on a schedule, since this repo is more in maintenance mode than development so the commit frequency is not so high. I'm just re-running the CI pipelines now after merging your other PRs and will merge when they pass Though it would be good to work on ISPC GPU support if I can find the time 😄 |
|
I merged this one in manually to add in some commits fixing additional warnings/errors that |
|
@Twinklebear Thanks, looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #21, and #22 as the latest
bindgenfixes clippy lints in generated codeAs discussed previously it'd be nice to keep
ispc-rs"free" of simple "code smells" commonly pointed out byclippy. This is easy to guard by disallowing any clippy warnings from trickling into the codebase through PRs via an extra CI check.Downside is that Rust upgrades every 6 weeks and usually enables new or improves existing lints, resulting in unrelated CI errors caused by existing code being punted on unsuspecting contributors in newly opened PRs. But these are typically easy to resolve in a separate PR, or to be caught early when (also) running the CI job on a schedule.