Improve msrv CI #17006
Improve msrv CI #17006
Conversation
BenjaminBrienen
force-pushed
the
msrv-ci
branch
from
4b9d6fc to
62e1c14
Compare
|
Let me know if we should switch to using |
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
|
YEEEESSSSS it works |
|
It seems to be using the bot comment from main. Hmm... edit: this is correct behavior |
BenjaminBrienen
added
A-Build-System
| fi | ||
|
|
||
| # Additional crates | ||
| crates=("bevy_color" "bevy_ecs" "bevy_input_focus" "bevy_math" "bevy_mikktspace" "bevy_ptr" "bevy_reflect") |
There was a problem hiding this comment.
Is there a reason we aren't running this check on all crates (e.g., it'd increase CI time substantially, etc.) Strictly speaking, all the crates can be used independent of bevy, so if we could check all of them we probably should.
There was a problem hiding this comment.
I agree, but members @alice-i-cecile and @mockersf decided that only certain select crates should have an msrv, which I respect.
There was a problem hiding this comment.
Perfectly fine if there's prior discussion leading to this decision, just wanted to capture that question in the PR.
There was a problem hiding this comment.
For reference: #16333 (comment)
There was a problem hiding this comment.
We should add a comment on the reasoning here.
| if !self | ||
| if self | ||
| .component_info | ||
| .type_id() | ||
| .is_some_and(|id| id == TypeId::of::<T>()) | ||
| .is_none_or(|id| id != TypeId::of::<T>()) |
There was a problem hiding this comment.
Yeah I saw this come up a few times but was confused why it would fail CI locally for me and then pass on GitHub. Glad to see it resolved.
| fi | ||
|
|
||
| # Additional crates | ||
| crates=("bevy_color" "bevy_ecs" "bevy_input_focus" "bevy_math" "bevy_mikktspace" "bevy_ptr" "bevy_reflect") |
There was a problem hiding this comment.
bevy_input_focus shouldn't be in this list: it's not usable standalone.
|
I still think we should not mention the MSRV of subcrates. Bevy and its ecosystem in general is "latest stable rust". If we want to handle it, I would prefer it to be discussed and made explicit first. This is #16172, but it has received low interest so I doesn't seem to be a priority. Once we have decided on the policy, encoding it in CI will be the next step. The way it's done in this PR is very bad for maintainability, it should be a matrix job. |
|
I agree with it being a matrix job. I implemented it this way being a bit new to github actions, but I expected there to be much cleaner ways of doing it, and that sounds right to me. I completely understand your argument about not specifying them at all, but it just comes down to whether we view specifying rust-version to be valuable. I like it because it seems like a nice little correctness detail and it seems easy enough to maintain as long as we have the right CI. If that's all out of scope for bevy, no hard feelings! I have fun playing around with this stuff even if it ends up getting turned down. |
# Objective The rust-versions are out of date. Fixes #17008 ## Solution Update the values Cherry-picked from #17006 in case it is controversial ## Testing Validated locally and in #17006 --------- Co-authored-by: Alice Cecile <[email protected]>
It's not, it's just an helper that will display a better error message. It doesn't allow/forbid anything that isn't already. It usually replaces a message like "feature XXX is unstable" by "rust version Y is required". This is very nice for newcomers, but once you're used to rust and you know that Bevy has a guarantee to work on stable, you know you need to update your local rust when you see that message I think it's valuable on the root crate, less so on the others. It's debatable on the independent crates. So we have to balance "better error message for an advance case" with "CI time / contributor load" |
alice-i-cecile
added
the
X-Controversial
Objective
Fixes #16330
Fixes #17008
Solution
Have the CI check the msrv for a select list of crates which are intended to be used apart from bevy.
I added a tool to make checking MSRVs easier.
I also removed the
needs: buildfrom msrv because it often uses a different toolchain, so the cache probably isn't helpful. Unblocking this makes it complete quicker.Testing
bevy_ecs's rust-version is already incorrect, so see the showcase for a demonstration of the failure.
https://github.com/bevyengine/bevy/actions/runs/12529512145/job/34945194232
Showcase