Skip to content

Replace rust-protobuf with prost#201

Merged
nrc merged 58 commits intotikv:masterfrom
ice1000:prost
Apr 11, 2019
Merged

Replace rust-protobuf with prost#201
nrc merged 58 commits intotikv:masterfrom
ice1000:prost

Conversation

@ice1000
Copy link
Copy Markdown
Contributor

@ice1000 ice1000 commented Mar 30, 2019

This PR:

  • Introduce prost dependencies
  • Added build.rs to (re)generate prost structs and their corresponding protobuf wrappers (thanks to @nrc!)
  • The generated wrappers are manually edited a little bit, please don't run regenerate yourself ATM
  • Passed all tests by changing direct usages of protobuf APIs into prost APIs

I suppose test-passing means ready-for-review, please leave your comments!

ice1000 and others added 30 commits January 31, 2019 06:41
@ice1000
Replace script with build script, use features
3fe4369 
See pingcap/kvproto#349

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Cargo fmt
39996ad 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Apply some suggestions from rust clippy
9323c47 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Remove everything related to prost
62d1f8e 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Refactor build.rs
e63166a 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Save progress
3e5ecc3 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Refactor build.rs
96143b3 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@Hoverbear
Merge branch 'master' into master
2e6afde
@ice1000
Refactor build.rs
e14ae28 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Save progress
78e460f 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Merge branch 'prost' of https://github.com/ice1000/raft-rs into prost
ed462e3
@ice1000
Documentation in src/lib.rs
e9ceaa4
@ice1000
Merge remote-tracking branch tikv/master
d056ffe
@ice1000
Add [cfg(feature = "lib-rust-protobuf")] to every protobuf places
e7e5ac0
@ice1000
Replace common.sh with rust code
2ffb284 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Merge branch 'master' into prost
35da6d4 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Fix compilation error
0993e17 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Fix typo
e1e5868 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Cargo fmt
a52bace 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@Hoverbear
Merge branch 'master' into master
54a29e3
@Hoverbear
Merge branch 'master' into master
7f064df
@ice1000
Merge remote-tracking branch 'tikv/master'
cdbfffa 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Merge branch 'master' of https://github.com/ice1000/raft-rs
f98f212
@ice1000
Merge branch 'master' into prost
824f6d9 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Fix typo-caused compilation error
b49d940 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Merge branch 'master' into prost
c95c4dc 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Fix compilation errors
7a322ca 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Merge remote-tracking branch 'origin/master' into prost
90659f2 
Conflicts:
Cargo.toml
generate-proto.sh
src/lib.rs
src/storage.rs
src/util.rs
@ice1000
Add rsprost file to git repo
e78a99c 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Save progress
b406b3a
ice1000 added 6 commits April 4, 2019 11:59
@ice1000
Rename generated mod to prost from rsprost
19a42e2 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Remove RepeatedField::from_vec
bf88990 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Remove all uses of protobuf::Message as Msg
3b9a8bb 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Remove Codec error and remove tests (not added for prost's errors b…
28294fa 
…ecause their constructors are private

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Remove even more protobuf stuff, apply some minor suggestions by clippy
64b3497 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Clean up build script
651dfd9 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Copy Markdown
Contributor Author

ice1000 commented Apr 4, 2019 •
edited
Loading

The rest of the refactoring depends on tikv/protobuf-build#2

@nrc
Copy link
Copy Markdown
Contributor

nrc commented Apr 4, 2019

The rest of the refactoring depends on tikv/protobuf-build#2

For at least the proto which is exported via kvproto, the Message impl is used by TiKV, so I think we need to keep those Message impls around for now.

@ice1000
Copy link
Copy Markdown
Contributor Author

ice1000 commented Apr 4, 2019

That sounds reasonable. I'll add the protobuf stuff back.

ice1000 added 3 commits April 4, 2019 16:37
@ice1000
Fix clippy warnings
e56bbe7 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Use merge
cf52151 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Add rust-protobuf codes back
f2c0219 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
nrc
nrc reviewed Apr 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks great, just two very minor things remaining

@ice1000
Copy link
Copy Markdown
Contributor Author

ice1000 commented Apr 4, 2019

Test failing because

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I have no idea about this.

@ice1000
Revert unexpected comment changes
f2dce76 
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
nrc
nrc approved these changes Apr 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

LGTM \o/

@nrc
Copy link
Copy Markdown
Contributor

nrc commented Apr 4, 2019

Travis is failing on Windows only, @Hoverbear can we ignore that?

Hoverbear
Hoverbear approved these changes Apr 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

Seems the vast majority of these changes are fairly mechanical. After chatting a bit and investigating the situation around new_() (which I do not like!) and how we'll fix the situation, I feel this is a good step of progress to merge.

I would prefer if we rapidly move to a state where we have functions without this naming convention. Preferably before 0.6.0

@Hoverbear
Copy link
Copy Markdown
Contributor

Hoverbear commented Apr 9, 2019 •
edited
Loading

@nrc We cannot! But this failure is related to travis, not you. We can force a few rebuilds and it should be green. I'll shepherd it.

@Hoverbear Hoverbear added this to the 0.6.0 milestone Apr 9, 2019
@Hoverbear Hoverbear added the Feature Related to a major feature. label Apr 9, 2019
@nrc nrc merged commit fc6c059 into tikv:master Apr 11, 2019
@Hoverbear Hoverbear mentioned this pull request Apr 11, 2019
Closed
@ice1000 ice1000 mentioned this pull request Apr 11, 2019
Closed
@ice1000 ice1000 deleted the prost branch April 13, 2019 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddontang siddontang siddontang left review comments

+2 more reviewers

@Hoverbear Hoverbear Hoverbear approved these changes

@nrc nrc nrc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nrc nrc

@ice1000 ice1000

Labels

Feature Related to a major feature.

Projects

None yet

Milestone

0.6.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ice1000 @nrc @Hoverbear @siddontang