Skip to content

Bump dependencies after version 0.24 #1105

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

Merged
merged 6 commits into from
Apr 10, 2024
Merged

Bump dependencies after version 0.24 #1105

merged 6 commits into from
Apr 10, 2024

Conversation

wks
Copy link
Collaborator

@wks wks commented Apr 9, 2024

This PR updates Cargo crates dependencies and GitHub Actions dependencies after v0.24 was released, with two exceptions:

  • We do not update criterion because a later version unintentionally requires MSRV 1.74. The problem has been fixed in the upstream criterion repo, but has not been released, yet.
  • We replaced actions-rs/toolchain with manual invocations of rustup because actions-rs/toolchain is no longer maintained. For tasks that don't require a specific toolchain, we rely on rustup and cargo to automatically install the toolchain specified by the file rust-toolchain upon first invocation.

Also disabled Clippy checks on Darwin because the clippy program itself randomly crashes on Darwin.

@wks wks requested a review from qinsoon April 9, 2024 09:40
@qinsoon
Copy link
Member

qinsoon commented Apr 9, 2024

Some dependencies need Rust 1.74 or newer, which does not comply with our MSRV 1.71.1. I don't think it is worth to bump MSRV for this reason. Maybe revert the dependency?

@wks
Copy link
Collaborator Author

wks commented Apr 9, 2024

Some dependencies need Rust 1.74 or newer, which does not comply with our MSRV 1.71.1. I don't think it is worth to bump MSRV for this reason. Maybe revert the dependency?

The dependency is pulled in by criterion 0.5.1. clap is a command line parsing library, and I wonder why criterion needs that. I'll see if we can disable that dependency by disabling some of its Cargo features. But our "N-1" MSRV policy does allow us to bump MSRV up to 1.76, given that the version in rust-toolchain is 1.77.0.

@wks
Copy link
Collaborator Author

wks commented Apr 9, 2024

Criterion depends on clap version 4, but clap started to require Rust 1.74 after version 4.4.9. The Criterion developers noticed this problem, too (because Criterion intends to support MSRV 1.70), and locked the version of clap in a recent commit: bheisler/criterion.rs@9f1db4a which has not been released, yet.

I don't see what new feature of Rust 1.74 Clap is using, but according to Clap's doc, it only intends to supports N-2 MSRV. So it may be bumping MSRV for no reason. While that's compatible with our N-1 MSRV policy, I don't think it's necessary for us to bump our rust-version in Cargo.toml because (1) we don't directly depend on clap, and (2) the next release of criterion is likely to revert to MSRV 1.70, and (3) we don't use new features of criterion 0.5, either. So I reverted the criterion dependency to 0.4 and kept our rust-version unchanged.

@wks wks added the PR-extended-testing Run extended tests for the pull request label Apr 9, 2024
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM

@wks wks added this pull request to the merge queue Apr 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2024
@wks wks removed the PR-extended-testing Run extended tests for the pull request label Apr 10, 2024
@wks
Copy link
Collaborator Author

wks commented Apr 10, 2024

I also removed dependencies on actions-rs/toolchain.

  • For minimal-test-core/${target-triple}/${version}, I directly use rustup to install the needed toolchain.
  • For other tests, such as check-public-api-changes and msrv, I don't invoke rustup explicitly, but upon the first invocation of rustup show, cargo --version or any other commands related to cargo, it will automatically install the toolchain specified in the file mmtk-core/rust-toolchain. The output of rustup show and cargo --version show that those tests are using the desired toolchains.

There is one small problem. rustup gave a warning when trying to install the i686-* toolchain. See #1107 for more details. That would require much changes to the CI scripts and I am not going to do that change in this PR. This PR focuses on removing outdated dependencies.

@wks wks added this pull request to the merge queue Apr 10, 2024
Merged via the queue into mmtk:master with commit 0540bb5 Apr 10, 2024
23 checks passed
@wks wks deleted the bump-202404 branch April 10, 2024 08:14
@wks wks mentioned this pull request May 7, 2024
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