-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add rust-toolchain file #22040
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
Open
IceSentry
wants to merge
15
commits into
bevyengine:main
Choose a base branch
from
IceSentry:rust-toolchain
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add rust-toolchain file #22040
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
5fabda0
Add rust-toolchain file
IceSentry f7fd938
remove whitespace
IceSentry a5c50cd
add rustfmt
IceSentry 997dc50
add miri
IceSentry 2f5eb56
specify profile
IceSentry f1ef3aa
wip test ci trick
IceSentry 504152b
fix
IceSentry fdb4ce2
forgot a v
IceSentry 094697f
try to specify the version instead of stable
IceSentry 37c6af1
try no toolchain specified
IceSentry 8d880e8
use rustup override
IceSentry 8380706
use override everywhere
IceSentry d53c5c0
add a few more overrides
IceSentry 2076ebe
add more missing overrides
IceSentry 80d35bb
Add dependabot support
IceSentry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [toolchain] | ||
| channel = "1.91.1" | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the value in being exact, but before adding semi-redundant "things", can you verify that the minimum Rust version in
Cargo.tomldoes not meet your needs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize we even had a rust version specified.
Are you saying I should set the toolchain value to match that 1.88 or are you saying that the toolchain file is redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying it is partially redundant. It is a piece of metadata that we already bump periodically, that is an attestation that Bevy requires features from that version.
The big tradeoff is that we aren't actually running our CI for that version (we use latest stable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is ... is that enough of a solve for your usecase, given that the proposal adds more complexity to our process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit that being aware of the rust version does change things a little bit, but in my case the reason I want it is for the ability to git bisect and having cargo automatically detect the toolchain it should be using makes it simpler. With that said, yeah, I guess the rust-version field solves the "guessing the compatible rust version" part. Either it didn't exist until recently or that field is essentially ignored by cargo though, because me and many people I've talked to on discord have had issues with invalid rust versions when running git bisect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't run all the CI on it, but Bevy is built in CI with the rust version from rust-version in this job: https://github.com/bevyengine/bevy/blob/main/.github/workflows/ci.yml#L456
It was (only) added 3 years ago
I think the issue is maybe with lints (builtin or clippy), with every rust version having new lints that Bevy fails. Or it did happen a few times that new versions of Rust don't compile Bevy, this is fixed usually well enough in advance by the Rust team with PR to Bevy, but if you go back in history with a modern version of rust it will fail to compile.