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.

Sign up for GitHub

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

[Merged by Bors] - add rust-version for MSRV and CI job to check #6852

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - add rust-version for MSRV and CI job to check #6852

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc700e5
add msrv
mockersf Dec 4, 2022
f827076
add check in CI
mockersf Dec 4, 2022
46bde96
update rust version
mockersf Jan 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/bors.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ status = [
"build-without-default-features (bevy)",
"build-without-default-features (bevy_ecs)",
"build-without-default-features (bevy_reflect)",
"msrv",
]

use_squash_merge = true
Expand Down
26 changes: 26 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,3 +274,29 @@ jobs:
run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev
- name: Run cargo udeps
run: cargo udeps

msrv:
runs-on: ubuntu-latest
needs: build
steps:
- uses: actions/checkout@v3
- uses: actions/cache@v3
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
key: ${{ runner.os }}-cargo-msrv-${{ hashFiles('**/Cargo.toml') }}
- name: get MSRV
run: |
msrv=`cargo metadata --no-deps --format-version 1 | jq --raw-output '.packages[] | select(.name=="bevy") | .rust_version'`
echo "MSRV=$msrv" >> $GITHUB_ENV
- uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ env.MSRV }}
- name: Install alsa and udev
run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev
- name: Run cargo check
run: cargo check
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ keywords = ["game", "engine", "gamedev", "graphics", "bevy"]
license = "MIT OR Apache-2.0"
readme = "README.md"
repository = "https://github.com/bevyengine/bevy"
rust-version = "1.65.0"
Copy link
Member

@james7132 james7132 Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this be specified on all of the crates, not just the top level bevy? Ideally with workspace inheritance.

Copy link
Member Author

@mockersf mockersf Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. For the moment, the only crates that make sense to use individually would be bevy_ecs and bevy_reflect. There is no "need" to have them all on the same msrv, so that could mean multiplying the time in CI...

In my opinion the role of this field is mostly to provide a better error message for Bevy users.

If done with workspace inheritance, I would prefer to do most of the Cargo.toml fields at the same time, there are already at least one PR up for that #6089

Copy link
Member

Choose a reason for hiding this comment

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

I think it still makes sense from a plugin developer perspective, where we encourage depending on individual crates over the top level bevy to minimize compile times.

Copy link
Member Author

@mockersf mockersf Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we encourage depending on individual crates over the top level bevy

We don't?

That break pretty much all discoverability of plugins through dependency like https://lib.rs/crates/bevy/rev

Copy link
Member

Choose a reason for hiding this comment

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

Just checked the plugin guidelines and I guess we never did add those findings to it. Issues like #5528 are going to get worse as the engine and ecosystem grows. IMO, discoverability is a weak argument for serializing dependency compilation on a single choke point, especially when alternatives for discoverability exist.

This isn't the place for this conversation though.

For the moment, the only crates that make sense to use individually would be bevy_ecs and bevy_reflect. There is no "need" to have them all on the same msrv, so that could mean multiplying the time in CI...

If these are the crates that are most avidly consuming new Rust language features, the rest of the engine is underpinned by both right now. If we bump the MSRV of either of them, the rest of the engine transitively takes on that MSRV as well, and with bevy_reflect being a dependency of bevy_ecs, the two are likely to be kept in sync. Likewise, if we want a MSRV on bevy_ecs , it should be on the crate itself, not the top level crate. I'm well aware of multiple groups picking and choosing parts of Bevy for their own purposes without pulling the whole engine in, and only putting it on bevy does not solve this issue for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm well aware of multiple groups picking and choosing parts of Bevy for their own purposes without pulling the whole engine in, and only putting it on bevy does not solve this issue for them.

Those would look like advanced users, at that point I would hope they are pinning versions of everything or vendoring, and have a good scenario for updates anyway.

I think the most interesting type of users for this check is new users of Bevy / Rust.
Putting it on all crates will be handled by workspace inheritance with #6089. I'll update this PR to use it if it's merged first, but I don't think this PR is the place to add it.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Francois here: we're targeting beginners with this change, who are overwhelmingly just going to use bevy itself.

mockersf marked this conversation as resolved.
Show resolved Hide resolved

[workspace]
exclude = ["benches", "crates/bevy_ecs_compile_fail_tests"]
Expand Down