Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Conversation

ilya-bobyr
Copy link
Contributor

@ilya-bobyr ilya-bobyr commented Apr 19, 2023

Dependabot now needs 1.68: dependabot/dependabot-core#7066

@ilya-bobyr ilya-bobyr changed the title Upgrade Rust to 1.68.0 Upgrade Rust to 1.68.2 Apr 19, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

FWIW, looks like Rust 1.69.0 is scheduled to be released tomorrow:
https://blog.rust-lang.org/inside-rust/2023/04/17/1.69.0-prerelease.html

Is it OK to wait one day and get the new version?

And regardless of which version we chose, we'll need CI images that contain the new version.

@ilya-bobyr
Copy link
Contributor Author

ilya-bobyr commented Apr 19, 2023

The changes look good to me.

FWIW, looks like Rust 1.69.0 is scheduled to be released tomorrow: blog.rust-lang.org/inside-rust/2023/04/17/1.69.0-prerelease.html

Is it OK to wait one day and get the new version?

I do not have a strong opinion here.
It would be nice to re-enable dependabot, but one or two days do not matter in this regard.
I wonder if Rust 1.70 could introduce any changes that would need to be address, slowing this the process?

And regardless of which version we chose, we'll need CI images that contain the new version.

Based on the Docker Hub history, it seems that @yihau is the one uploading those images.

@ryoqun
Copy link
Contributor

ryoqun commented Apr 20, 2023

Is it OK to wait one day and get the new version?

yeah, i'm aware of immeminent 1.69. but personally, i'd like to soak our 500k loc monorepo a bit with 1.68.x for a week or two without jumping... :):

$ git grep ^ origin/master :./**.rs | wc -l
483908

@ryoqun
Copy link
Contributor

ryoqun commented Apr 20, 2023

cc: @yihau also note that it still need to be decided to bundle nightly update as per: #31099 (comment)

@yihau
Copy link
Contributor

yihau commented Apr 20, 2023

@ryoqun do we prefer to upgrade the nightly version in this PR or in another one?

@ryoqun
Copy link
Contributor

ryoqun commented Apr 20, 2023

@ryoqun do we prefer to upgrade the nightly version in this PR or in another one?

i wanna try to see the sscache bug is still occurring with the very recent nightly.

@yihau
Copy link
Contributor

yihau commented Apr 20, 2023

okay! I'm going to upgrade those docker images. one sec

@yihau
Copy link
Contributor

yihau commented Apr 20, 2023

uploaded and retried the pipeline! I will create another PR for testing nightly version bumping

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #31276 (d341e9b) into master (7a393e4) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #31276   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         733      733           
  Lines      207009   207009           
=======================================
+ Hits       168731   168809   +78     
+ Misses      38278    38200   -78     

if ! cargo +"$toolchain" -V > /dev/null; then
echo "$0: Missing toolchain? Installing...: $toolchain" >&2
rustup install "$toolchain"
rustup +"$toolchain" componnent add clippy rustfmt
Copy link
Contributor

Choose a reason for hiding this comment

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

oops. there's no set -e here...? prefer separate pr...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are looking at an incremental change in the PR, rather than the PR as a whole.
Here is the commit in this PR that adds this line: solana-labs/solana@07602dc (#31276)

I've added this to see if the CI would make more progress before the images would be available.
The change adds this whole line

rustup +"$toolchain" component add clippy rustfmt

I do not fully understand how the CI is structured, so, let me remove this change altogether.

Overall, it seems that CI runs rustup with the "minimal" profile.
At the same time, expecting clippy and rustfmt. Both are not part of the "minumal" profile.
The command has no effect if the component was already installed by the preceding rustup install "$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.

Here is the component addition as a separate PR: #31280

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... Seems like this change is indeed necessary: https://buildkite.com/solana-labs/solana/builds/94223#01879d48-2e7a-4c7b-8367-869da1aa8102

I do not understand it, but let me bring the component addition change back into this PR.

Copy link
Contributor Author

@ilya-bobyr ilya-bobyr Apr 20, 2023

Choose a reason for hiding this comment

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

Ah, it did not use the updated image, it seems.

The CI log shows:

++exec docker run --workdir /solana --volume
  /var/lib/buildkite-agent/builds/ci37/solana-labs/solana:/solana --rm --volume
  /var/lib/buildkite-agent:/home --env RUSTC_WRAPPER=/home/.cargo/bin/sccache
  --env AWS_ACCESS_KEY_ID --env AWS_SECRET_ACCESS_KEY --env SCCACHE_BUCKET --env
  SCCACHE_REGION --env SCCACHE_S3_KEY_PREFIX --env HOME=/home --env
  CARGO_HOME=/home/.cargo --security-opt seccomp=unconfined --user 997:997 --env
  BUILDKITE --env BUILDKITE_AGENT_ACCESS_TOKEN --env BUILDKITE_JOB_ID --env CI
  --env CI_BRANCH --env CI_BASE_BRANCH --env CI_TAG --env CI_BUILD_ID --env
  CI_COMMIT --env CI_JOB_ID --env CI_PULL_REQUEST --env CI_REPO_SLUG --env
  CRATES_IO_TOKEN -e CODECOV_ENV -e CODECOV_TOKEN -e CODECOV_URL -e CODECOV_SLUG
  -e VCS_COMMIT_ID -e VCS_BRANCH_NAME -e VCS_PULL_REQUEST -e VCS_SLUG -e VCS_TAG
  -e CI_BUILD_URL -e CI_BUILD_ID -e CI_JOB_ID -e CI -e BUILDKITE -e
  BUILDKITE_BRANCH -e BUILDKITE_BUILD_NUMBER -e BUILDKITE_JOB_ID -e
  BUILDKITE_BUILD_URL -e BUILDKITE_PROJECT_SLUG -e BUILDKITE_COMMIT -t
  solanalabs/rust-nightly:2023-01-22 ci/test-checks.sh
error: toolchain '1.68.2-x86_64-unknown-linux-gnu' is not installed
ci/test-checks.sh: Missing toolchain? Installing...: 1.68.2

So it is using solanalabs/rust-nightly, rather than solanalabs/rust:latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the rustup components command.
But CI is failing now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah.. sorry. one sec.

Copy link
Contributor

Choose a reason for hiding this comment

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

just retried the pipeline. it's running 🪖

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, thanks for those extra effort for my picky review comments... really appreciate logically-cleaner pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is good you started this conversation, I think.
This is what the reviews are for :)

Copy link
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm

@ilya-bobyr ilya-bobyr merged commit a21e68a into solana-labs:master Apr 20, 2023
@ilya-bobyr ilya-bobyr deleted the pr/rust-1.68-update branch April 20, 2023 18:31
@ryoqun ryoqun mentioned this pull request Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants