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

fix: only emit rustc-check-cfg on Rust 1.77.0 or newer #10

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

Conversation

ErichDonGubler
Copy link

No description provided.

@zicklag
Copy link
Member

zicklag commented Aug 13, 2024

I was on the fence about whether we should check the Rust version and asked about that in #7 (comment), but @ogoffart and @Urgau were of the opinion that it probably wasn't necessary to do the check.

If we do the check, I think we can do it like in this comment #7 (comment), like the semver crate, instead of adding another dependency, since it's just a tiny function necessary to parse the version.

If this is something that WGPU needs, then I think we should just do the check, but I'd like to give the opportunity for comments before merging.

@ErichDonGubler
Copy link
Author

WGPU could just wait until its MSRV is high enough to avoid warnings, and then upgrade. There aren't other features from this crate that we consume, so 🤷🏻‍♂️ I wouldn't be devastated if this PR were simply closed.

@ogoffart
Copy link
Contributor

Pro this change:

  • hides warning for people trying to compile the directly dependent crate with an old version of rust

Cons:

  • adds a dependency, that calls rustc to know the version, making the build a tiny bit slower.

One could argue that the build time is probably negligible.

But on the other hand, these warnings are only shown for some old version of rustc, they are not shown for dependencies (i.e, if someone use wgpu from crates.io, they won't see the warnings because of the caps-lint) and cargo warnings can't be turned into error on the CI.

@zicklag
Copy link
Member

zicklag commented Sep 5, 2024

For now I'm leaning towards just leaving this be in favor of keeping this crate as 100% minimal and "guilt-free" as possible, but I don't have strong opinions on this change, so if anybody feels that this should definitely be done, feel free to comment and we'll get it fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants