-
Notifications
You must be signed in to change notification settings - Fork 377
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
Change CI to build with Rust 1.32 and document that as MSRV #433
Conversation
bors try |
tryBuild failed: |
Looks like we need at least 1.34 for riscv64gc, so I will update the CI to use 1.34 for that target. Not sure what we need to fix the asmjs or x86_64-pc-windows-msvc though, will investigate. |
…scripten, and x86_64-pc-windows-msvc
Does an MSRV make sense for a binary-only crate? |
b93c4e4
to
ee91870
Compare
Okay, looks like riscv64 really needs a very modern Rust. Now we have everything working on 1.32.0 except for:
I think it's worth having; I think there are two good reasons:
The current WG policy means it shouldn't be restrictive to have the MSRV, since if a new feature is required we can easily increase the MSRV; it therefore serves as documentation and a check that we're only bumping it deliberately. Do you think there are reasons to not have MSRV for a binary-only crate compared to a library? I appreciate it adds a maintenance burden to occasionally increase MSRV when a new feature is used, but I'd like to know if there are other downsides I'm missing. |
I don't think the MSRV should apply to specific targets, it should only pertain to the compilation of the
I think if that remains the case, it's fine to add an MSRV for documentation purposes. |
That makes sense, though it might still be useful for users to know what the MSRV is for the target(s) they're interested in. In any event we need to pick a version of Rust for each target; why not its MSRV? There's no reason to expect them to increase per-target now and the majority (all but 3) are 1.32. Otherwise we'll have to add a separate build step to build and test cross on the MSRV in addition to testing each target. I could also imagine updating the three standouts to use 'stable' instead of specific versions if that seems better? |
I think it's fine as is. |
Are there any other changes you'd like me to make or are we OK to merge this? |
Ready to merge. |
bors r=reitermarkus |
433: Change CI to build with Rust 1.32 and document that as MSRV r=reitermarkus a=adamgreig This PR adds a documented MSRV, per rust-embedded/wg#445 As far as I can tell none of the Azure pipelines use Xargo or nightly Rust, so I think this small change should be sufficient, but we'll see what CI makes of it. Co-authored-by: Adam Greig <adam@adamgreig.com>
Build failed:
|
bors retry |
433: Change CI to build with Rust 1.32 and document that as MSRV r=reitermarkus a=adamgreig This PR adds a documented MSRV, per rust-embedded/wg#445 As far as I can tell none of the Azure pipelines use Xargo or nightly Rust, so I think this small change should be sufficient, but we'll see what CI makes of it. Co-authored-by: Adam Greig <adam@adamgreig.com>
@adamgreig, some Docker images don't build anymore. |
bors r- |
Canceled. |
bors r+ |
433: Change CI to build with Rust 1.32 and document that as MSRV r=reitermarkus a=adamgreig This PR adds a documented MSRV, per rust-embedded/wg#445 As far as I can tell none of the Azure pipelines use Xargo or nightly Rust, so I think this small change should be sufficient, but we'll see what CI makes of it. Co-authored-by: Adam Greig <adam@adamgreig.com>
Build failed: |
Seems like updating the dependencies broke the MSRV. Maybe we should just switch to 1.42.0 for all targets for simplicity's sake? |
Sounds good, let's try that. |
bors r+ |
Build succeeded: |
This PR adds a documented MSRV, per rust-embedded/wg#445
As far as I can tell none of the Azure pipelines use Xargo or nightly Rust, so I think this small change should be sufficient, but we'll see what CI makes of it.