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

Bump the MSRV to 1.64.0 #204

Merged
merged 3 commits into from
Mar 21, 2023
Merged

Conversation

Rustin170506
Copy link
Member

If the rust toolchain lower than v1.64.0 then you will get a compile error:

error[E0106]: missing lifetime specifier
  --> /Users/hi-rustin/.cargo/registry/src/github.com-1ecc6299db9ec823/inferno-0.11.15/src/flamegraph/color/mod.rs:83:25
   |
83 |     pub const VARIANTS: &[&'static str] = &[
   |                         ^ expected named lifetime parameter
   |
help: consider using the `'static` lifetime
   |
83 |     pub const VARIANTS: &'static [&'static str] = &[
   |                          +++++++
help: consider introducing a named lifetime parameter
   |
81 ~ impl<'a> Palette {
82 |     /// The valid set of palettes (via `FromStr`).
83 ~     pub const VARIANTS: &'a [&'static str] = &[

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@Rustin170506
Copy link
Member Author

Also, you can see an error for other packages:

error: package `rayon-core v1.11.0` cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.56.1
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

See: https://github.com/tikv/pprof-rs/actions/runs/4474804380/jobs/7863621636

error: package `prost v0.11.8` cannot be built because it requires rustc 1.60 or newer, while the currently active rustc version is 1.56.1
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

See: https://github.com/tikv/pprof-rs/actions/runs/4474804380/jobs/7863622817?pr=204

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@@ -35,12 +35,6 @@ jobs:
- name: Remove pre-generated prost files to force regeneration
run: rm proto/*.rs

- name: Specify dependency version for old Rust
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure does it make sense to keep or remove it. @YangKeao Could you please explain a bit why we use different Cargo.toml in CI and codebase? Thanks! It is a little bit confusing to have different Cargo.toml in the same project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:
Cargo always uses inferno 0.11.15 when building pprof-rs. You have to use cargo update -p inferno --precise 0.11.1 to work around.

Copy link
Member Author

@Rustin170506 Rustin170506 Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example:

vsc cargo new --bin pporf-rs-test
     Created binary (application) `pporf-rs-test` packagevsc cd pporf-rs-testpporf-rs-test git:(master) ✗ cargo add pprof -F flamegraph
    Updating crates.io indexpporf-rs-test git:(master) ✗ cargo b
error[E0106]: missing lifetime specifier
  --> /Users/hi-rustin/.cargo/registry/src/github.com-1ecc6299db9ec823/inferno-0.11.15/src/flamegraph/color/mod.rs:83:25
   |
83 |     pub const VARIANTS: &[&'static str] = &[
   |                         ^ expected named lifetime parameter
   |
help: consider using the `'static` lifetime
   |
83 |     pub const VARIANTS: &'static [&'static str] = &[
   |                          +++++++
help: consider introducing a named lifetime parameter
   |
81 ~ impl<'a> Palette {
82 |     /// The valid set of palettes (via `FromStr`).
83 ~     pub const VARIANTS: &'a [&'static str] = &[
   |

My toolchain:

active toolchain
----------------

1.60.0-aarch64-apple-darwin (directory override for '/Users/hi-rustin/vsc/pporf-rs-test')
rustc 1.60.0 (7737e0b5c 2022-04-04)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain a bit why we use different Cargo.toml in CI and codebase? Thanks! It is a little bit confusing to have different Cargo.toml in the same project.

For the MSRV actually. It proved pprof-rs can actually work on 1.56 with some dependency specified, so this only modified the dependencies for CI with 1.56 rust.

I'm not sure whether the dependencies of pprof-rs will be automatically updated when running cargo update -p pprof. If not, this behavior enables the user to update pprof-rs even when they are on 1.56 rust. (Even if not, it's also possible for them to downgrade the dependencies of pprof-rs to solve this problem)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the MSRV actually. It proved pprof-rs can actually work on 1.56 with some dependency specified, so this only modified the dependencies for CI with 1.56 rust.

Got it. Then probably the better solution is we set the correct version in the Cargo.toml instead of letting the user manually downgrade the deps of pprof-rs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there are some deps of pprof-rs that require a higher rust toolchain to build it. And Cargo does not consider the rust-version of Cargo.toml when it resolves the deps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of letting the user manually downgrade the deps of pprof-rs.

And there are no tips for users to work around this issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Then probably the better solution is we set the correct version in the Cargo.toml instead of letting the user manually downgrade the deps of pprof-rs.

I also don't want to specify a too low version (of dependencies) for users, because it will avoid the users from using newer dependencies. It cloud also cause the dependency conflict, so I usually use x.y but not x.y.z in most cases. Do you have any better idea on this? (Upgrading MSRV can be a choice, but it's actually avoidable, by specifying dependencies manually 🤦 .)

And Cargo does not consider the rust-version of Cargo.toml when it resolves the deps.

This is exactly the problem. The cargo will not automatically choose the newest valid version. As a developer of library, I also cannot specify different dependencies for different rust version.

Because there are some deps of pprof-rs that require a higher rust toolchain to build it.

All of them are here: inferno and criterion. And the solution is to downgrade them (by the user manually, as shown in the github.yml). The lower version are allowed in the range specified in Cargo.toml of pprof-rs. The sed in ci only sets the version precisely (from x.y to x.y.z).

Copy link
Member

@YangKeao YangKeao Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And there are no tips for users to work around #204 (comment)

Yes 🤦 . I should add document for it.

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Though the MSRV increased passively, we have to accept it 😮‍💨

@YangKeao YangKeao merged commit 5fac2a2 into tikv:master Mar 21, 2023
@Rustin170506
Copy link
Member Author

Thanks for your review! 💚 💙 💜 💛 ❤️

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 this pull request may close these issues.

2 participants