Minimum rustc version error #449

arnaudgolfouse · 2023-05-05T16:54:30Z

Add an error when the user is running cargo-semver-checks with a rustc version < 1.65.

obi1kenobi

Thanks for opening this PR, it's super helpful and I'm excited to merge it soon.

I have a couple of suggestions to make it even better.

src/lib.rs

obi1kenobi · 2023-05-05T17:06:15Z

test_crates/minimum_rustc/fails/rust-toolchain.toml

@@ -0,0 +1,2 @@
+[toolchain]
+channel = "1.64"


Unfortunately I think the way our tests are set up will explicitly ignore this.

It might be less work to write this test as a GitHub Action job. That way we don't have to change how the test setup works, and people don't have to keep Rust 1.64 installed just to run tests locally.

So, I managed to have the test actually use the 1.64 channel automatically. However I agree that forcing people to have Rust 1.64 installed is pretty annoying.
My original idea was that it would catch bumps of the minimal version, so that the error detection + message might be updated. But looking at it now, I guess such a bump would fail CI anyway, and it would make it clear that the check rust_version < rustc_version_1_65 must be updated... I think ?

So, should I remove the tests entirely ?

I would remove the tests here but then set up an equivalent test in GitHub Actions, where it explicitly uses Rust 1.64 to semver-check a crate, captures the output, and asserts the exit code was non-zero and contained the warning about using an old Rust.

There are some tests like that in GitHub Actions already, and you should be able to do what they do as well.

obi1kenobi

Functionality-wise this looks good. Just a couple of wording suggestions in the error messages and the test tweaks.

obi1kenobi · 2023-05-06T19:51:02Z

src/lib.rs

+                    config.shell_warn(format_args!(
+                        "while trying to query the current rustc version: {}",
+                        error
+                    ))?;


This error message text is "implementation-oriented." The user that would see it wouldn't know we were querying the rustc version (or why) so they'll just be confused and won't know what to do. Let's try to make this a user-oriented, actionable error message:

Suggested change

config.shell_warn(format_args!(

"while trying to query the current rustc version: {}",

error

))?;

config.shell_warn(format_args!(

"failed to determine the current rustc version: {}\n\nHELP: to avoid errors please ensure Rust 1.65+ is used",

error

))?;

or something to that effect. Bonus points if you could pull the rustc_version_needed into config itself and then somehow use its value to generate the "Rust 1.65+" portion of the error message string.

Does pulling the rustc_version_needed into config bring other benefits? Right now the message is generated by the local let rustc_version_needed = rustc_version::Version::new(1, 65, 0), and it works well.

This local let is unfortunately not authoritative on which version is actually supported — it's totally possible that we bump the underlying trustfall_rustdoc dependency (which is authoritative on the supported rustdoc versions) while forgetting to change this value here.

It's somewhat less likely that we forget it if it's either a global static, or in the config, than a random local variable in the middle of the file. So it's just about improving the probability of us remembering to change it, nothing else :)

src/lib.rs

obi1kenobi · 2023-05-06T19:54:14Z

test_crates/minimum_rustc/fails/rust-toolchain.toml

@@ -0,0 +1,2 @@
+[toolchain]
+channel = "1.64"


I would remove the tests here but then set up an equivalent test in GitHub Actions, where it explicitly uses Rust 1.64 to semver-check a crate, captures the output, and asserts the exit code was non-zero and contained the warning about using an old Rust.

There are some tests like that in GitHub Actions already, and you should be able to do what they do as well.

obi1kenobi

Functionality-wise this looks good. Just a couple of wording suggestions in the error messages and the test tweaks.

obi1kenobi

The new test looks great. Just a couple of minor nits left.

I'll fix the nightly problem tomorrow or on Monday, don't worry about it for now. It isn't required to pass to merge PRs.

obi1kenobi · 2023-05-14T02:19:58Z

src/lib.rs

@@ -316,6 +316,26 @@ impl Check {
            .deps(false)
            .silence(!config.is_verbose());

+        // If both the current and baseline rustdoc are given explicitely as a file path, we don't need to use the installed rustc, and this check can be skipped.


Suggested change

// If both the current and baseline rustdoc are given explicitely as a file path, we don't need to use the installed rustc, and this check can be skipped.

// If both the current and baseline rustdoc are given explicitly as a file path, we don't need to use the installed rustc, and this check can be skipped.

Typo fix + let's try to keep comments and string literals to below 100 chars both here and below

obi1kenobi · 2023-05-14T02:20:24Z

src/lib.rs

+        if !(matches!(self.current.source, RustdocSource::Rustdoc(_))
+            && matches!(self.baseline.source, RustdocSource::Rustdoc(_)))
+        {
+            let rustc_version_needed = rustc_version::Version::new(1, 65, 0);


If we can lift this to a global static (ideally) or into the global config (next best), that'd be great.

obi1kenobi

The new test looks great. Just a couple of minor nits left.

I'll fix the nightly problem tomorrow or on Monday, don't worry about it for now. It isn't required to pass to merge PRs.

obi1kenobi

Looks great, thanks for all the help!

obi1kenobi · 2023-05-14T12:43:57Z

If you might be interested in continuing to contribute to the project, we could certainly use the help! #451 in particular is an unfortunate (and kind of embarrassing) issue that shouldn't be too hard to fix. The full list of contributor-friendly issues is here: https://github.com/obi1kenobi/cargo-semver-checks/issues?q=is%3Aissue+is%3Aopen+label%3AE-help-wanted

.github/workflows/ci.yml

obi1kenobi reviewed May 5, 2023

View reviewed changes

arnaudgolfouse force-pushed the minimum-rust-version-error branch 2 times, most recently from c9c25bd to 4193e67 Compare May 6, 2023 11:06

obi1kenobi reviewed May 6, 2023

View reviewed changes

arnaudgolfouse force-pushed the minimum-rust-version-error branch 4 times, most recently from 5b386e0 to 67433b2 Compare May 13, 2023 10:31

obi1kenobi reviewed May 14, 2023

View reviewed changes

arnaudgolfouse force-pushed the minimum-rust-version-error branch from 67433b2 to deafd14 Compare May 14, 2023 09:26

obi1kenobi approved these changes May 14, 2023

View reviewed changes

obi1kenobi enabled auto-merge (squash) May 14, 2023 12:40

obi1kenobi reviewed May 14, 2023

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

arnaudgolfouse added 3 commits May 15, 2023 11:21

Check that the version of rustc currently used is >=1.65

d64f970

Pull minimum_rustc_version into GlobalConfig

aae7270

Test the error message on rustc 1.64 in CI

122ff4d

auto-merge was automatically disabled May 15, 2023 09:23
Head branch was pushed to by a user without write access

arnaudgolfouse force-pushed the minimum-rust-version-error branch from ee1a37f to 122ff4d Compare May 15, 2023 09:23

obi1kenobi approved these changes May 15, 2023

View reviewed changes

obi1kenobi merged commit 933cc10 into obi1kenobi:main May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimum rustc version error #449

Minimum rustc version error #449

arnaudgolfouse commented May 5, 2023

obi1kenobi left a comment

obi1kenobi May 5, 2023

arnaudgolfouse May 6, 2023

obi1kenobi May 6, 2023

obi1kenobi left a comment

obi1kenobi May 6, 2023

arnaudgolfouse May 13, 2023

obi1kenobi May 14, 2023

arnaudgolfouse May 14, 2023

obi1kenobi May 6, 2023

obi1kenobi left a comment

obi1kenobi left a comment

obi1kenobi May 14, 2023

obi1kenobi May 14, 2023

obi1kenobi left a comment

obi1kenobi left a comment

obi1kenobi commented May 14, 2023

	// If both the current and baseline rustdoc are given explicitely as a file path, we don't need to use the installed rustc, and this check can be skipped.
	// If both the current and baseline rustdoc are given explicitly as a file path, we don't need to use the installed rustc, and this check can be skipped.

Minimum rustc version error #449

Minimum rustc version error #449

Conversation

arnaudgolfouse commented May 5, 2023

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi commented May 14, 2023