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

chore(build): bump toolchain to 2022-10-27 #6025

Merged
merged 12 commits into from
Oct 28, 2022
Merged

chore(build): bump toolchain to 2022-10-27 #6025

merged 12 commits into from
Oct 28, 2022

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Oct 26, 2022

Co-Authored-By: xxchan xxchan22f@gmail.com
Signed-off-by: Bugen Zhao i@bugenzhao.com

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR is mostly a replication of #5119. We use a workaround for zip_order_key to make it compile.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

close #5977

@skyzh skyzh changed the title chore: prepare for bump toolchain chore(build): bump toolchain to 2022-10-16 Oct 27, 2022
@skyzh skyzh marked this pull request as ready for review October 27, 2022 21:43
@CLAassistant
Copy link

CLAassistant commented Oct 28, 2022

CLA assistant check
All committers have signed the CLA.

@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

@MrCroxx I've workaround the file cache lifetime problem. You should fix it if you have time later.

BugenZhao and others added 2 commits October 28, 2022 03:19
Co-Authored-By: xxchan <xxchan22f@gmail.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@skyzh skyzh added the mergify/can-merge Indicates that the PR can be added to the merge queue label Oct 28, 2022
Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh removed the mergify/can-merge Indicates that the PR can be added to the merge queue label Oct 28, 2022
@xxchan xxchan changed the title chore(build): bump toolchain to 2022-10-16 chore(build): bump toolchain to 2022-10-27 Oct 28, 2022
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@wangrunji0408
Copy link
Contributor

@BugenZhao Any idea why building simulation timed out on CI? 🥵

@BugenZhao
Copy link
Member Author

@BugenZhao Any idea why building simulation timed out on CI? 🥵

Nope. 🥺 Does it mean we should write code really carefully after this PR? 🥵

@wangrunji0408
Copy link
Contributor

Building nexmark_chaos.rs become extremely slow in the new toolchain. Need help from rustc experts 🆘

@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

Thanks for identifying 🤣

@BugenZhao
Copy link
Member Author

Building nexmark_chaos.rs become extremely slow in the new toolchain. Need help from rustc experts 🆘

There shouldn't be much black magic inside. Perhaps const generic?

@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

Probably yes. Also why don't we merge hello, nexmark_chaos, nexmark_q4 into one single binary?

@BugenZhao
Copy link
Member Author

BugenZhao commented Oct 28, 2022

That's okay. But I think there's no problem to separate integrated tests into multiple files? 🤣 This should be a best practice of Rust.

@BugenZhao
Copy link
Member Author

BugenZhao commented Oct 28, 2022

Still don't think we should get this merged before the compiler fixes the issue. It's abnormal to put such effort or attention on these hacks while programming. 🤔 The attempts to bump still deserve acknowledgment though.

@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

Still don't think we should get this merged before the compiler fixes the issue. It's abnormal to put such effort or attention on these hacks while programming. 🤔

I think it's more like we are abusing the type system. Sometimes we just insert random 'a at some random place and we even don't know why it works.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
Copy link
Member Author

Agree on some storage stuffs, but it makes little sense on such a simple integrated test. 🤣

@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

Agree on some storage stuffs, but it makes little sense on such a simple integrated test. 🤣

The potential problems lying in the integrated test are something I've been fixing before. All interfaces exposed by lib and not needed for inlining should have BoxFuture. Otherwise it will be recompiled in every binary. Let's see if it improves compile speed.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

I feel like it's also a bug of lifetime inference. FnMut seems to be the root cause for slow integration test compile. Will reproduce later. Let's workaround it first.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@skyzh
Copy link
Contributor

skyzh commented Oct 28, 2022

Removing impl<ToString> and impl<IntoIterator> seem to fix everything.

@skyzh skyzh added the mergify/can-merge Indicates that the PR can be added to the merge queue label Oct 28, 2022
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #6025 (70665fa) into main (fe021b0) will increase coverage by 0.06%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #6025      +/-   ##
==========================================
+ Coverage   74.64%   74.70%   +0.06%     
==========================================
  Files         931      933       +2     
  Lines      148816   149009     +193     
==========================================
+ Hits       111078   111312     +234     
+ Misses      37738    37697      -41     
Flag Coverage Δ
rust 74.70% <83.33%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/batch/src/execution/grpc_exchange.rs 55.55% <ø> (ø)
src/batch/src/execution/local_exchange.rs 56.84% <ø> (ø)
src/batch/src/executor/test_utils.rs 67.15% <ø> (ø)
src/batch/src/lib.rs 100.00% <ø> (ø)
src/batch/src/task/broadcast_channel.rs 0.00% <ø> (ø)
src/batch/src/task/env.rs 31.81% <0.00%> (ø)
src/batch/src/task/fifo_channel.rs 86.11% <ø> (ø)
src/batch/src/task/hash_shuffle_channel.rs 0.00% <ø> (ø)
src/cmd_all/src/bin/risingwave.rs 2.63% <ø> (ø)
src/common/src/lib.rs 100.00% <ø> (ø)
... and 100 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BugenZhao
Copy link
Member Author

BugenZhao commented Oct 28, 2022

Removing impl<ToString> and impl<IntoIterator> seem to fix everything.

This is unreasonable. 🤣🤣 There should be tons of occurrences in well-known library crates of them. Why do they still behave well? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue type/chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking: bump compiler toolchain to 1.65
6 participants