-
Notifications
You must be signed in to change notification settings - Fork 403
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
Conversation
See pingcap/kvproto#349 Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Conflicts: Cargo.toml generate-proto.sh src/lib.rs src/storage.rs src/util.rs
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
…ecause their constructors are private Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
The rest of the refactoring depends on tikv/protobuf-build#2 |
For at least the proto which is exported via kvproto, the |
That sounds reasonable. I'll add the protobuf stuff back. |
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
Signed-off-by: ice1000 <[email protected]>
There was a problem hiding this 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
Test failing because
I have no idea about this. |
Signed-off-by: ice1000 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM \o/
Travis is failing on Windows only, @Hoverbear can we ignore that? |
There was a problem hiding this 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
@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. |
This PR:
build.rs
to (re)generate prost structs and their corresponding protobuf wrappers (thanks to @nrc!)I suppose test-passing means ready-for-review, please leave your comments!