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

Upgrade to Rust 2018 #4138

Merged
merged 10 commits into from Jan 31, 2019

Conversation

Projects
None yet
6 participants
@brson
Copy link
Contributor

brson commented Jan 28, 2019

This upgrades the tikv package and all of the component packages to 2018.

It does not yet upgrade any of the fuzzing code to 2018, mostly because I didn't want to go through the long process of revalidating the fuzzers. Somebody else can make those changes while they are working on the fuzzers, if they feel like it's important.

The commits are all pretty straightforward at this point.

After this there is still work that can be done to bring tikv up to date with 2018 idioms. I'll file separate bugs on that.

Fixes #3896

brson added some commits Jan 28, 2019

*: run rustfix --edition
Signed-off-by: Brian Anderson <andersrb@gmail.com>
components/*: turn on Rust 2018
Signed-off-by: Brian Anderson <andersrb@gmail.com>
components/*: fix 2018 path ambiguities
Signed-off-by: Brian Anderson <andersrb@gmail.com>
tikv: turn on Rust 2018
Signed-off-by: Brian Anderson <andersrb@gmail.com>
tikv: fix ambiguous paths
Signed-off-by: Brian Anderson <andersrb@gmail.com>
*: fix mutability warnings for 2018
Signed-off-by: Brian Anderson <andersrb@gmail.com>
*: use a 'test' import that rustfmt won't mangle
rustfmt wants to convert ::test into test, which doesn't build.
Hopefully it will leave crate::test alone.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
*: run rustfmt
Signed-off-by: Brian Anderson <andersrb@gmail.com>

@brson brson force-pushed the brson:rust-2018-try3 branch from 3154d04 to 747b634 Jan 28, 2019

@brson brson requested review from breeswish and rleungx Jan 28, 2019

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 28, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Jan 29, 2019

Great work!!!

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Jan 29, 2019

2019/01/29 08:08:59.170 mod.rs:563: [DEBG] Storage de-referenced

2019/01/29 08:08:59.170 mod.rs:563: [DEBG] Storage de-referenced

2019/01/29 08:08:59.170 mod.rs:563: [DEBG] Storage de-referenced

what is DEBG here. @breeswish @overvenus

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Jan 29, 2019

test raftstore::test_snap::test_server_snap_gc ... thread 'raftstore::test_snap::test_server_snap_gc' panicked at 'snap files is still not empty: ["/tmp/test_cluster.YtCdz2PzRYEQ/rev_1_6_929.meta", "/tmp/test_cluster.YtCdz2PzRYEQ/rev_1_6_929_lock.sst", "/tmp/test_cluster.YtCdz2PzRYEQ/rev_1_6_929_default.sst"]', tests/integrations/raftstore/test_snap.rs:180:13
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:491
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:398
   6: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:353
   7: integrations::raftstore::test_snap::test_server_snap_gc
             at tests/integrations/raftstore/test_snap.rs:180
   8: integrations::raftstore::test_snap::test_server_snap_gc::{{closure}}
             at tests/integrations/raftstore/test_snap.rs:101
   9: core::ops::function::FnOnce::call_once
             at /rustc/14997d56a550f4aa99fe737593cd2758227afc56/src/libcore/ops/function.rs:238
  10: <F as alloc::boxed::FnBox<A>>::call_box
             at src/libtest/lib.rs:1471
             at /rustc/14997d56a550f4aa99fe737593cd2758227afc56/src/libcore/ops/function.rs:238
             at /rustc/14997d56a550f4aa99fe737593cd2758227afc56/src/liballoc/boxed.rs:673
  11: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:102

@hicqu CI failed

@@ -11,8 +11,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::grpc::{RpcContext, RpcStatus, RpcStatusCode, UnarySink};

This comment has been minimized.

@siddontang

siddontang Jan 29, 2019

Contributor

em, why we use crate:: for grpc but not other libraries?

This comment has been minimized.

@brson

brson Jan 29, 2019

Author Contributor

Rustfix did this. I suspect it's because of details in how the 2018 crate imports work. The crate is not called grpc but grpcio, so the path that resolves to the crate would be grpcio::{RpcContext.... grpc is a name introduced with use as and only exists within this crate.

@rleungx

This comment has been minimized.

Copy link
Member

rleungx commented Jan 29, 2019

It seems that we don't need extern crate anymore?

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 29, 2019

It seems that we don't need extern crate anymore?

That's right. I'll file a follow on bug for updating those and other 2018 idioms.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 29, 2019

@zhouqiang-cl has discovered that the entropy bug we were running into earlier, where the compiler itself runs out of entropy still exists and is affecting CI (#4143). Fixing that could mean reverting the toolchain upgrade, so we might want to wait until that's resolved to land this.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 29, 2019

I posted another PR that tries to upgrade to a newer nightly, hoping its upgraded rand crate will fix the intermittent CI problems: #4144

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 30, 2019

Re my previous comment about CI being affecting by this toolchain's entropy bug, that is incorrect. @zhouqiang-cl has upgraded the CI machines to a new kernel that is not affected. The CD machines though are affected still.

So I think we can move forward with this. I don't foresee reverting the toolchain at this point.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 30, 2019

/rebuild

@overvenus

This comment has been minimized.

Copy link
Member

overvenus commented Jan 30, 2019

what is DEBG here

They are debug level logs. @siddontang

@overvenus overvenus added the C: Build label Jan 30, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Jan 30, 2019

Refer to our RFC, seem should be "DEBUG", why use "DEBG" here?
@overvenus @breeswish

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Jan 30, 2019

@Hijiao PTAL about the logging

@nrc

nrc approved these changes Jan 31, 2019

Copy link

nrc left a comment

This is probably the most boring PR I've ever reviewed :-) But I've read every line and it LGTM

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 31, 2019

The raftstore::test_split_region::test_node_half_split_region failure seems to be intermittent. Locally it only happens rarely. I don't see an issue open for it.

/rebuild

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 31, 2019

I think we're ready to go here. 1 more approval? @siddontang @breeswish

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 31, 2019

II don't know where the "DEBG" messages are coming from or else I would file an issue on it.

@rleungx
Copy link
Member

rleungx left a comment

LGTM.

@brson brson merged commit 02ebbd6 into tikv:master Jan 31, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment