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.

Sign up for GitHub

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

Jump to bottom

Bump the MSRV to 1.64.0 #204

Merged
merged 3 commits into from
Mar 21, 2023
Merged

Bump the MSRV to 1.64.0 #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bad92b0
Bump the MSRV to 1.64.0
Rustin170506 Mar 21, 2023
3331326
Update toolchain
Rustin170506 Mar 21, 2023
b9bd56b
Remove useless mv
Rustin170506 Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 17 additions & 29 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ jobs:
uses: actions-rs/toolchain@v1.0.7
with:
profile: minimal
toolchain: 1.56.1
toolchain: 1.64.0
override: true
components: rustfmt, clippy

- name: Install protobuf compiler
run: sudo apt-get install -y protobuf-compiler

Expand All @@ -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 •
edited
Loading

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` package
➜  vsc cd pporf-rs-test
➜  pporf-rs-test git:(master) ✗ cargo add pprof -F flamegraph
    Updating crates.io index
➜  pporf-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 •
edited
Loading

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.

run: |
cp Cargo.toml Cargo.toml.bak
sed -i "s|inferno = { version = \"0.11\"|inferno = { version = \"=0.11.1\"|g" ./Cargo.toml
sed -i "s|criterion = \"0.4\"|criterion = \"=0.3.0\"\ncsv = \"=1.1.0\"|g" Cargo.toml

- name: Run cargo clippy prost
uses: actions-rs/cargo@v1.0.3
with:
Expand All @@ -55,22 +49,22 @@ jobs:

- name: Check if the prost file committed to git is up-to-date
run: |
mv Cargo.toml.bak Cargo.toml
git diff --no-ext-diff --exit-code

build:
name: Build
strategy:
matrix:
os: [ubuntu-latest, macos-latest]
toolchain: [stable, nightly, 1.56.1]
target: [
x86_64-unknown-linux-gnu,
aarch64-unknown-linux-gnu,
x86_64-unknown-linux-musl,
x86_64-apple-darwin,
aarch64-apple-darwin,
]
toolchain: [stable, nightly, 1.64.0]
target:
[
x86_64-unknown-linux-gnu,
aarch64-unknown-linux-gnu,
x86_64-unknown-linux-musl,
x86_64-apple-darwin,
aarch64-apple-darwin,
]
exclude:
- os: ubuntu-latest
target: x86_64-apple-darwin
Expand All @@ -97,13 +91,6 @@ jobs:
target: ${{ matrix.target }}
override: true

- name: Specify dependency version for old Rust
if: ${{ matrix.toolchain == '1.56.1' }}
run: |
# sed -i is not compatible with Mac OS
mv Cargo.toml Cargo.toml.bak
sed "s|inferno = { version = \"0.11\"|inferno = { version = \"=0.11.1\"|g" Cargo.toml.bak > ./Cargo.toml

- name: Run cargo build prost
uses: actions-rs/cargo@v1.0.3
with:
Expand All @@ -129,11 +116,12 @@ jobs:
matrix:
os: [ubuntu-latest, macos-latest]
toolchain: [stable, nightly]
target: [
x86_64-unknown-linux-gnu,
x86_64-unknown-linux-musl,
x86_64-apple-darwin,
]
target:
[
x86_64-unknown-linux-gnu,
x86_64-unknown-linux-musl,
x86_64-apple-darwin,
]
exclude:
- os: ubuntu-latest
target: x86_64-apple-darwin
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ description = "An internal perf tools for rust programs."
repository = "https://github.com/tikv/pprof-rs"
documentation = "https://docs.rs/pprof/"
readme = "README.md"
rust-version = "1.64.0" # MSRV

[features]
default = ["cpp"]
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ Unit tests have been added to guarantee there is no `malloc` in sample functions

## Minimum Supported Rust Version

Rust 1.56 or higher.
Rust 1.64 or higher.

Minimum supported Rust version can be changed in the future, but it will be done with a minor version bump.

Expand Down