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

Replace rust-protobuf with prost #201

Merged
merged 58 commits into from Apr 11, 2019

Conversation

Projects
None yet
4 participants
@ice1000
Copy link
Member

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 some commits Jan 30, 2019

Replace script with build script, use features
See pingcap/kvproto#349

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Cargo fmt
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Apply some suggestions from rust clippy
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Remove everything related to prost
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Refactor build.rs
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Save progress
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Refactor build.rs
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Refactor build.rs
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Save progress
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Replace common.sh with rust code
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Merge branch 'master' into prost
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix compilation error
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix typo
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Cargo fmt
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Merge remote-tracking branch 'tikv/master'
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Merge branch 'master' into prost
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix typo-caused compilation error
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Merge branch 'master' into prost
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix compilation errors
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Merge remote-tracking branch 'origin/master' into prost
Conflicts:
Cargo.toml
generate-proto.sh
src/lib.rs
src/storage.rs
src/util.rs
Add rsprost file to git repo
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

ice1000 added some commits Apr 4, 2019

Remove all uses of `protobuf::Message as Msg`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Remove `Codec` error and remove tests (not added for prost's errors b…
…ecause their constructors are private

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Remove even more protobuf stuff, apply some minor suggestions by clippy
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Clean up build script
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

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

@nrc

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

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

ice1000 added some commits Apr 4, 2019

Fix clippy warnings
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Use `merge`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Add rust-protobuf codes back
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

@ice1000 ice1000 force-pushed the ice1000:prost branch from d3e6a3d to f2c0219 Apr 4, 2019

@nrc
Copy link
Collaborator

left a comment

Looks great, just two very minor things remaining

Show resolved Hide resolved src/errors.rs
Show resolved Hide resolved src/storage.rs Outdated
@ice1000

This comment has been minimized.

Copy link
Member Author

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.

Revert unexpected comment changes
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@nrc

nrc approved these changes Apr 4, 2019

Copy link
Collaborator

left a comment

LGTM \o/

@nrc

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

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

@Hoverbear
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@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 label Apr 9, 2019

@nrc nrc merged commit fc6c059 into pingcap:master Apr 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ice1000 ice1000 deleted the ice1000:prost branch Apr 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.