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

Require minimum Rust 1.56 #381

Merged
merged 5 commits into from
Aug 18, 2023
Merged

Conversation

Enet4
Copy link
Collaborator

@Enet4 Enet4 commented Jul 2, 2023

As discussed in #379, this PR does step 1: bump the hard minimum supported version of Rust to 1.56.

Summary

  • change rust-toolchain accordingly
  • remove Cargo features 1_34 and 1_46
  • simplify feature gated code
  • update guide and documentation to no longer mention existing features
  • mark Location as non_exhaustive, remove _other field
  • inline async_body function, remove report/no_async and report/yes_async
  • add rust-version to manifest
  • change CI min version test task to use Rust 1.56
  • remove v1.34 compatibility tests

@netlify
Copy link

netlify bot commented Jul 2, 2023

Deploy Preview for shepmaster-snafu ready!

Name Link
🔨 Latest commit 8b22564
🔍 Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/64dfb4412a6aff0008940689
😎 Deploy Preview https://deploy-preview-381--shepmaster-snafu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Enet4
Copy link
Collaborator Author

Enet4 commented Jul 2, 2023

Just replying to some of the feedback provided before the PR:

We probably need to grep for (variations on) 1.34 / 1.39 / 1.46 to find places we talk about them in prose.

My greps yield no more results in need of updates.

Why did snafu-derive get a default = [] feature?

My mistake, I probably kept it in while I was experimenting with making the rust_v* features enabled by default. It's removed now.

The yes_async::async_body module can probably be inlined.

IIUIC this will only be the case once we require a minimum version of Rust 1.61.

fn track_caller() can probably be inlined as well

👍 Done now.


By the way, should rust_1_61 be enabled by default? I noticed that the compatibility page claims so, but that does not appear to be the case.

@Enet4 Enet4 mentioned this pull request Jul 2, 2023
@shepmaster
Copy link
Owner

shepmaster commented Jul 5, 2023

Would you mind pulling Update compile_error test snapshots out into its own PR? It's not really about updating to 1.56, just that the current stable output has changed. (also I want it for #383 😇)

Maybe also rustfmt src/lib.rs if that's more drift from stable.

@shepmaster
Copy link
Owner

IIUIC this will only be the case once we require a minimum version of Rust 1.61.

Hmm, I thought it was because Rust 1.34 didn't know the async keyword (only 1.39+). That file also has a switch on 1.61, but that shouldn't be relevant for merging the file.

By the way, should rust_1_61 be enabled by default? I noticed that the compatibility page claims so, but that does not appear to be the case.

Hmm, there's two points here...

  1. The 0.7 docs are probably just wrong. They also are missing the 1.39 feature flag from that table.
  2. Since we are upping the MSRV and releasing a new semver-incompatible version, we will probably want to change the default feature-flagged version to be whatever is stable when we release 0.8 (likely 1.70 /1.71 which will effectively mean 1.61 as there are no other flags since then).

I'm generally planning on releasing one more 0.7 so I can fix those docs (and maybe cause some merge conflicts 😬).

@Enet4
Copy link
Collaborator Author

Enet4 commented Jul 5, 2023

Would you mind pulling Update compile_error test snapshots out into its own PR? It's not really about updating to 1.56, just that the current stable output has changed. (also I want it for #383 😇)

Makes sense indeed. I'll get to that once I'm able.

Maybe also rustfmt src/lib.rs if that's more drift from stable.

That was caused by the removal of the non-exhaustive lock field _other. I might just fuse the commit with a fixup.

Hmm, I thought it was because Rust 1.34 didn't know the async keyword (only 1.39+). That file also has a switch on 1.61, but that shouldn't be relevant for merging the file.

I guess it's easy to try if the code builds on Rust 1.56 anyway, and make changes if so. I'll look into it.

Hmm, there's two points here...

  1. The 0.7 docs are probably just wrong. They also are missing the 1.39 feature flag from that table.
  2. Since we are upping the MSRV and releasing a new semver-incompatible version, we will probably want to change the default feature-flagged version to be whatever is stable when we release 0.8 (likely 1.70 /1.71 which will effectively mean 1.61 as there are no other flags since then).

OK, in that case I will not touch the documentation (which already claims that rust_v1_61 would be enabled).

I'm generally planning on releasing one more 0.7 so I can fix those docs (and maybe cause some merge conflicts 😬).

*git rebase intensifies*

@Stargateur
Copy link
Contributor

You can also add:

[package]
rust-version = "1.56"

to all Cargo.toml file

@Enet4 Enet4 force-pushed the change/msrv_1_56 branch 2 times, most recently from 3db8cdc to 06ac7cd Compare July 7, 2023 16:43
@shepmaster
Copy link
Owner

Looking good. I made one small tweak to wording and force pushed (also rebased for good measure).

Enet4 added 5 commits August 18, 2023 14:10
- change rust-toolchain accordingly
- remove Cargo features 1_34 and 1_46
- simplify feature gated code
- update guide and documentation to no longer mention existing features
- mark Location as non_exhaustive,
  remove _other field
- change CI min version test task to use Rust 1.56
- remove v1.34 compatibility tests
- fix mention of Rust version compatibility
- all supported versions of Rust also support `async/await`
- remove report directory and folders
@shepmaster shepmaster merged commit 93502c2 into shepmaster:main Aug 18, 2023
@shepmaster shepmaster added the breaking change Likely requires a SemVer version bump label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Likely requires a SemVer version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants