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

Pin Rust to 1.81.0 #1073

Merged
merged 5 commits into from
Oct 22, 2024
Merged

Pin Rust to 1.81.0 #1073

merged 5 commits into from
Oct 22, 2024

Conversation

unexge
Copy link
Contributor

@unexge unexge commented Oct 21, 2024

  • Create rust-toolchain.toml
  • Use actions-rust-lang/setup-rust-toolchain GitHub action as it supports rust-toolchain.toml

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
@passaro
Copy link
Contributor

passaro commented Oct 21, 2024

@unexge, is the rust-toolchain action actually required? My understanding is that cargo will respect rust-toolchain.toml automatically.

@unexge
Copy link
Contributor Author

unexge commented Oct 21, 2024

@passaro, not sure. It's a popular request for dtolnay/rust-toolchain to support rust-toolchain file, so I assumed it won't work. But let me give it a try.

Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 14:30 — with GitHub Actions Inactive
- name: Set up stable Rust
uses: dtolnay/rust-toolchain@stable
- name: Set up Rust toolchain
uses: dtolnay/rust-toolchain@master
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion was to remove this step entirely, not to revert to stable

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative: we could look at some other actions, e.g. https://github.com/marketplace/actions/rust-toolchain-toml-install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh, I misunderstood. I think dtolnay/rust-toolchain installs rustup/cargo. We might look installing it in .github/actions/install-dependencies step, not sure if there are any downsides.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering about jumping to a new action, I saw this one that looks well maintained: https://github.com/actions-rust-lang/setup-rust-toolchain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use actions-rust-lang/setup-rust-toolchain. I had to override rustflags option though, it's -D warnings by default which turns all warnings into compile errors, and vendor/fuser has lots of warnings. It'd be nice to get rid of those warnings eventually, but not turning this on should be fine for now.

This new action also surfaces warnings on PRs, e.g., scroll to the bottom on https://github.com/awslabs/mountpoint-s3/pull/1073/files. We can turn this of it we find it noisy, but eventually I'd like to address these warnings as well.

Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:11 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:11 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:11 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:11 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:11 — with GitHub Actions Inactive
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge temporarily deployed to PR integration tests October 21, 2024 16:31 — with GitHub Actions Inactive
@unexge unexge added this pull request to the merge queue Oct 22, 2024
Merged via the queue into awslabs:main with commit 39c58a1 Oct 22, 2024
23 checks passed
@unexge unexge deleted the pin-rust-1.81.0 branch October 22, 2024 12:42
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.

3 participants