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

refactor: bump toolchain to 2024-07-19 #18470

Merged
merged 13 commits into from
Sep 11, 2024
Merged

refactor: bump toolchain to 2024-07-19 #18470

merged 13 commits into from
Sep 11, 2024

Conversation

MrCroxx
Copy link
Contributor

@MrCroxx MrCroxx commented Sep 10, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

The rust toolchain version 1.81.0 is stable, which was branched from 2024-07-19.

Let's bump our toolchain version. Otherwise we cannot use lib whose MSRV is stable 1.81.0.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: MrCroxx <mrcroxx@outlook.com>
@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 10, 2024

image image

The opaque is !Unpin, which cannot be treat as pub type GooglePubSubSinkDeliveryFuture = impl TryFuture<Ok = (), Error = SinkError> + Unpin + 'static;

cc @xxhZs @wenym1

BugenZhao
BugenZhao previously approved these changes Sep 10, 2024
@xxchan
Copy link
Member

xxchan commented Sep 10, 2024

As mentioned in #17179, we might need to change all TAITs to bump toolchain... (unless rust-lang/rust#128440 is landed)

@BugenZhao BugenZhao dismissed their stale review September 10, 2024 06:16

more changes ongoing

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 10, 2024

image

😳 FIrst time seeing this.

Copy link
Contributor

@wenym1 wenym1 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

Do we consider bumping to the latest toolchain by the way?

@@ -102,7 +102,7 @@ impl BloomFilterReader {
true
} else {
let nbits = self.data.bit_len();
let delta = (h >> 17) | (h << 15);
let delta = h.rotate_left(15);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we forget the rotate_right(17)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, with u32 and 17 + 15 = 32, this operation equals to directly rotate_left(15). (clippy also recommends it.

@wenym1
Copy link
Contributor

wenym1 commented Sep 10, 2024

As mentioned in #17179, we might need to change all TAITs to bump toolchain... (unless rust-lang/rust#128440 is landed)

Actually it's not that many. I have fixed all the involved TAIT.

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 10, 2024

#[derive(Default)]
struct ExprInputRefFinder {
pub input_ref_index_set: HashSet<usize>,
}
impl ExprVisitor for ExprInputRefFinder {
fn visit_input_ref(&mut self, input_ref: &InputRef) {
self.input_ref_index_set.insert(input_ref.index);
}
}

@chenzl25 Is this code still using? It seems a dead code.

Signed-off-by: MrCroxx <mrcroxx@outlook.com>
Signed-off-by: MrCroxx <mrcroxx@outlook.com>
@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 10, 2024

Signed-off-by: MrCroxx <mrcroxx@outlook.com>
@MrCroxx MrCroxx requested a review from a team as a code owner September 10, 2024 07:41
Signed-off-by: MrCroxx <mrcroxx@outlook.com>
@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 10, 2024

UPDATES: dylint fixed via risingwavelabs/clippy#1

@graphite-app graphite-app bot requested a review from a team September 10, 2024 14:31
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

Thank you!!

Remember to bump CI image before merging, so that we don't install rust toolchain repeatedly in CI

@xxchan
Copy link
Member

xxchan commented Sep 11, 2024

Wait, I think we can bump to a newer version. No need to stick to latest stable 🤔

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 11, 2024

Wait, I think we can bump to a newer version. No need to stick to latest stable 🤔

Agreed. Let me try nightly-2024-09-10. BTW, are you familiar with the the issue in the madsim integration test?

info: syncing channel updates for 'nightly-2024-07-19-x86_64-unknown-linux-gnu'
info: latest update on 2024-07-19, rust version 1.81.0-nightly (5affbb171 2024-07-18)
info: downloading component 'cargo'
error: component download failed for cargo-x86_64-unknown-linux-gnu

Caused by:
    could not rename downloaded file from '/root/.rustup/downloads/724035a7625f76dc52925fc2e8858e9d2c7ce6c4b31a8dd15bb57b9154590abb.partial' to '/root/.rustup/downloads/724035a7625f76dc52925fc2e8858e9d2c7ce6c4b31a8dd15bb57b9154590abb'

Stack backtrace:
   0: rustup::dist::manifestation::Manifestation::update
   1: rustup::dist::dist::try_update_from_dist_
   2: rustup::install::InstallMethod::install
   3: rustup::toolchain::distributable::DistributableToolchain::install
   4: rustup::config::Cfg::find_or_install_override_toolchain_or_default
   5: rustup::cli::proxy_mode::main
   6: rustup_init::main
   7: std::sys_common::backtrace::__rust_begin_short_backtrace
   8: main
   9: <unknown>
  10: __libc_start_main
  11: <unknown>

The instance keep failing fetching the components of the toolchain over and over again. Is it related to the CI image?

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 11, 2024

Let me try nightly-2024-09-10.

There are some false-positive cases with elided named lifetime.

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Sep 11, 2024

Let's just upgrade the toolchain to 2024-07-19. I need it for some stable crates with 1.81.0.

@MrCroxx MrCroxx enabled auto-merge September 11, 2024 06:16
@MrCroxx MrCroxx added this pull request to the merge queue Sep 11, 2024
@graphite-app graphite-app bot requested a review from a team September 11, 2024 06:20
Merged via the queue into main with commit 6038298 Sep 11, 2024
31 of 32 checks passed
@MrCroxx MrCroxx deleted the xx/bump-2024-07-19 branch September 11, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants