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

MSRV policy #18

Closed
2 tasks done
Emoun opened this issue Sep 21, 2020 · 9 comments
Closed
2 tasks done

MSRV policy #18

Emoun opened this issue Sep 21, 2020 · 9 comments
Labels
D-accepted A decision (D) has been made and the issue will be worked on I-feature This issue (I) regards a (potential) feature in the project T-accepted Triage (T): Initial review accepted issue/PR as valid

Comments

@Emoun
Copy link
Owner

Emoun commented Sep 21, 2020

Short Description:

Decide on a "Minimum Supported Rust Version" (MSRV).

Motivation:

#17 proposes changes to the implementation to allow the crate to work on rust 1.34.
This is a clear indication of a need for an official MSRV from a direct user of the crate.
The reason for using 1.34 specifically is that apparently the current Debian stable has this version installed.
This could be a good starting point for an MSRV as its backwards compatible with the current implementation, and it can be lowered even more in the future, without having to bump the major version. And from #17 it doesn't seem like we depend on important features of newer rust versions.

Design

Commit to an official MSRV of 1.34

Misc:

  • This crate currently has no MSRV, however, the CI setup does test against rust 1.45 specifically. So, in a way 1.45 is our current MSRV.

Unresolved Questions

  • - Do all our dependencies have MSRVs that are compatible with this proposal
  • - What features have been introduced since 1.34 and are we sure we can live without them in the future.
@Emoun Emoun added D-discussion A decision (D) has not been made yet and is open to discussion I-feature This issue (I) regards a (potential) feature in the project labels Sep 21, 2020
@ghost ghost added the T-new Triage (T): Has yet to be reviewed label Sep 21, 2020
@Emoun Emoun added T-accepted Triage (T): Initial review accepted issue/PR as valid and removed T-new Triage (T): Has yet to be reviewed labels Sep 22, 2020
@Emoun
Copy link
Owner Author

Emoun commented Sep 22, 2020

Review of features introduced by rust > 1.34 that might be of interest to this crate:

  1. v1.40: Introduced the ability of function-like and attribute procedural macros to produce macro_rules!.
    This could impact this crate in the future, however, currently there are no planned features that require it.
  2. v1.40: Introduced the #[cfg(doctest)] attribute to include an item only when running documentation tests with rustdoc.
    This is currently used by the crate to only test the cargo readme when running doctests. We would need an alternative.
  3. v1.45: Introduced Function like procedural macro being able to be used in expression, pattern, and statement positions.
    This doesn't impact this crate directly. It instead impacts what users can use this crate for. However, if users need the functionality, they could update their rust version.

@Emoun
Copy link
Owner Author

Emoun commented Sep 22, 2020

Review of this crate's dependencies' MSRV policies:

  • proc-macro-error: effective MSRV seems to be 1.31.
  • convert_case: No MSRV and no CI setup.
    Since this is not a user-facing dependency, we can mitigate their lack of MSRV by sticking to a specific version of the crate that is know to work with our MSRV.
  • macrotest: No MSRV policy and CI setup only test against stable rust.
    Since this is a dev-dependency, we can mitigate their lack of policy by always depending on a specific version that is known to work with out MSRV.
  • doc-comment: No MSRV policy but CI setup tests against v 1.38.
    Same mitigations as macrotest can be used.

@Emoun
Copy link
Owner Author

Emoun commented Sep 22, 2020

I tried to change the CI setup to use rust 1.34, however, it failed. It seems that cargo-expand fails to build.
cargo-expand is a dependency of ours, as macrotest uses it, meaning its MSRV policy must comply too.
It doesn't seem to have one.

@Emoun
Copy link
Owner Author

Emoun commented Sep 23, 2020

The failure of cargo-expand to build using rust 1.34 is caused by their use of a lock file that uses a format introduced by rust 1.38. See rust-lang/cargo#7579. However, since we only need the cargo-expand binary, we can use stable rust to build it even though the rest of the test is using 1.34.

@Emoun
Copy link
Owner Author

Emoun commented Sep 23, 2020

It seems the ability to apply custom attributes to modules was introduced in rust 1.42, even though the release notes do not mention this (as far as I can see). Therefore, for 1.34, this is illegal.
This hinders the module_disambiguation feature, that specifically targets applying duplicate to modules.

We will therefore need a separate MSRV for this feature. And since it is enabled by default, this will raise the MSRV of this crate when using default features.

@Emoun
Copy link
Owner Author

Emoun commented Sep 23, 2020

Regarding #[cfg(doctest)] not being available for rust 1.34:

When run on 1.34, this attribute will be false causing the test of the readme to not be run. Crucially, this does not cause an error.
Therefore, we can keep this as is, as this test will be run when the stable channel is tested. We only really need it to be true on the current stable at publish time. Therefore, I think we can ignore this and keep it as is.

@Emoun
Copy link
Owner Author

Emoun commented Sep 23, 2020

Final decision on MSRV policy:

The base MSRV is 1.34 (this is the lowest possible MSRV.)
However, specific features may bump the MSRV to some higher value. Therefore, the default MSRV is equal to the highest MSRV among all default features.
Currently, the following features bump the MSRV from the base (the rest of the features don't bump the MSRV from the base):

module_disambiguation (default): bump to 1.42

Increasing the base MSRV or the MSRV of a feature is regarded as a breaking change and will be accompanied by a major version bump.
Increasing the default MSRV will not result in a major version bump. This is because the default feature list is intended for new(ish) users and is the most user-friendly way to use the crate. We want to be able to add features to this list easily. Therefore, anyone who needs strict MSRV adherence must turn off default features and specify features manually.

@Emoun Emoun added D-accepted A decision (D) has been made and the issue will be worked on and removed D-discussion A decision (D) has not been made yet and is open to discussion labels Sep 23, 2020
@laarmen
Copy link
Contributor

laarmen commented Sep 23, 2020

Thanks for being so thorough, I highly appreciate it!

@Emoun
Copy link
Owner Author

Emoun commented Sep 25, 2020

Implemented and will part of the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-accepted A decision (D) has been made and the issue will be worked on I-feature This issue (I) regards a (potential) feature in the project T-accepted Triage (T): Initial review accepted issue/PR as valid
Projects
None yet
Development

No branches or pull requests

2 participants