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

Don't use cargo +TOOLCHAIN shortcut and move --cfg bevy_lint logic #264

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Feb 8, 2025

The bevy_lint executable currently runs:

RUSTC_WORKSPACE_WRAPPER=path/to/bevy_lint_driver cargo +TOOLCHAIN check --cfg bevy_lint

This broke for someone on Discord, however, because the +TOOLCHAIN syntax requires Rustup's cargo proxy, which may not be in their PATH. This PR fixes this by instead calling rustup run directly:

RUSTC_WORKSPACE_WRAPPER=path/to/bevy_lint_driver rustup run TOOLCHAIN cargo check --cfg bevy_lint

Both versions do the exact same thing, and both will fail if you do not have Rustup installed, but the latter version should work in a few more scenarios. (And it makes the error message clearer, since it mentions Rustup.)

Furthermore, this PR moves the --cfg bevy_lint logic from the bevy_lint executable to bevy_lint_driver. While the linter should still behave the same, this means programs will still be able to detect the linter even if the use called bevy_lint_driver directly instead of bevy_lint.1

Footnotes

  1. For example, our UI tests call bevy_lint_driver directly. This means we can now technically use #[cfg(bevy_lint)] in our UI tests, even if it doesn't make sense to do so.

BD103 added 2 commits February 8, 2025 11:20
…CHAIN`

Some users don't have Rustup's proxy `cargo` on their path, so we cannot depend on `cargo +TOOLCHAIN` always working. Since we do require `rustup`, though, the explicit form should work more often.
Now if someone calls the driver directly, the `cfg` will still be applied.
@BD103 BD103 added A-Linter Related to the linter and custom lints C-Usability An improvement that makes the API more pleasant D-Trivial Nice and easy! A great choice to get started with Bevy CLI S-Needs-Review The PR needs to be reviewed before it can be merged labels Feb 8, 2025
@BD103 BD103 merged commit 8d58625 into main Feb 10, 2025
8 checks passed
@BD103 BD103 deleted the explicit-rustup branch February 10, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Usability An improvement that makes the API more pleasant D-Trivial Nice and easy! A great choice to get started with Bevy CLI S-Needs-Review The PR needs to be reviewed before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants