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

Consider setting a lower MSRV #58

Closed
jyn514 opened this issue Jun 22, 2020 · 4 comments · Fixed by #59
Closed

Consider setting a lower MSRV #58

jyn514 opened this issue Jun 22, 2020 · 4 comments · Fixed by #59

Comments

@jyn514
Copy link
Contributor

jyn514 commented Jun 22, 2020

Motivation: I want to use saltwater in bindgen (rust-lang/rust-bindgen#1782), but it has an MSRV of 1.34, which target-lexicon doesn't compile with. It uses the following features not in 1.34:

  • #[nonexhaustive]: this can be replaced with a #[doc(hidden)] __Nonexhaustive variant and documentation that the enum is nonexhaustive
error[E0658]: non exhaustive is an experimental feature (see issue #44109)
  --> src/data_model.rs:39:1
   |
39 | #[non_exhaustive]
   | ^^^^^^^^^^^^^^^^^
  • Self::variant: this can be replaced with the explicit type name
error: enum variants on type aliases are experimental
  --> src/data_model.rs:15:13
   |
15 |             Self::U8 => 8,
   |             ^^^^^^^^
  • extern crate alloc in build.rs: this can be replaced by use std as alloc; as long as the std feature is enabled. I don't need 1.34 support with no_std.
error[E0658]: use of unstable library feature 'alloc': this library is unlikely to be stabilized in its current form or name (see issue #27783)
  --> build.rs:12:1
   |
12 | extern crate alloc;
   | ^^^^^^^^^^^^^^^^^^^

I understand that most bytecodealliance projects aren't considered stable and therefore have no MSRV, but this would help me a lot, I'm willing to make the PRs myself.

@sunfishcode
Copy link
Member

The Self and alloc issues seem reasonable to fix. Removing #[nonexhaustive] would be inconvenient; we added it because every time we bump our major version, we cause some of our downstream users to have to update in lock-step with other downstream users. While the proposed alternative here would also work, changing to it would be yet another major-version bump. Does rust-lang/rust-bindgen#1782 (comment) mean that perhaps the minimum Rust version could be bumped instead? nonexhaustive would only need Rust 1.40.

@jyn514
Copy link
Contributor Author

jyn514 commented Jun 25, 2020

Hmm this would be unfortunate for me personally since the latest version of rust we have at work is 1.34 and I'd like to eventually use saltwater via bindgen on a work project. If this is hard to do for target-lexicon I might add some more hacks to make sizeof() opt-out for saltwater.

@sunfishcode
Copy link
Member

Would it work to change #[nonexhaustive] to #[cfg_attr(feature = rust_1_40, non_exhaustive)], and add rust_1_40 as a default feature in Cargo.toml? And maybe even add a #[cfg(not(feature = rust_1_40))] #[doc(hidden)] __Nonexhaustive too.

@jyn514
Copy link
Contributor Author

jyn514 commented Jun 25, 2020

Sure, that works for me. Good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants