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

Update cfg_aliases in order to fix unexpected cfg condition warnings #6348

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Oct 1, 2024

Connections

Description
(title)

Testing
run cargo clippy with deleted rust-toolchain.toml file
-> Turns out we do have one condition that needs to be ignored, wgpu_validate_locks (cc: @jimblandy )

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Wumpf Wumpf requested a review from a team as a code owner October 1, 2024 08:00
@ErichDonGubler
Copy link
Member

I'm surprised this PR isn't running into the issues that motivated katharostech/cfg_aliases#10, which is why I'd kept similar changes held back in #6105. 🤔 Will try out these changes locally.

@ErichDonGubler ErichDonGubler added the area: infrastructure Testing, building, coordinating issues label Oct 1, 2024
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Mmm, yeah, CI is printing many warnings like this (link):

warning: [email protected]: cargo:rustc-check-cfg requires -Zcheck-cfg flag

...but it's not failing CI. wat?

🤔 If @gfx-rs/wgpu thinks that this is acceptable for downstream to endure, I suppose we could merge something like this. It's not likely to break downstream, but it will throw up warnings that users will be confronted (and have no leverage to resolve) if they're not on Rust 1.77 or higher. On principle, that sounds like a very downstream-hostile thing to do, and I'd strongly prefer we simply wait until MSRV is 1.77 or higher.

Gonna hit Request changes until we get consensus on the above.

Comment on lines +32 to +36
[lints.rust]
# The `wgpu_validate_locks` config is supposed to be set via `RUSTFLAGS='--cfg wgpu_validate_locks'`
# If set, `wgpu-core` uses the [`ranked`] module's locks. We hope to make this the default for debug builds soon.
unexpected_cfgs = { level = "allow", check-cfg = ['cfg(wgpu_validate_locks)'] }

Copy link
Member

Choose a reason for hiding this comment

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

thought: I've implemented something similar to this in this commit in #6105 as build.rs output instead:

println!("cargo::rustc-check-cfg=cfg(wgpu_validate_locks)");

The reason I did this was because I felt it made more sense to keep the cfg config (😆) in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷 no preference really, but I see you found one more than me.
So I can just cherry-pick your commit over

@Wumpf
Copy link
Member Author

Wumpf commented Oct 2, 2024

I've also observed the warning you cite in other projects which why I ignored it as well here, not thinking about it. Didn't get to the bottom of why this happens so far :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: infrastructure Testing, building, coordinating issues
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Unexpected cfg condition warnings for cfgs defined by build.rs
2 participants