-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use a published version of the rust-proto branch #31
Open
nrc
wants to merge
1
commit into
tikv:master
Choose a base branch
from
nrc:proto-dep
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
[package] | ||
name = "protobuf-build" | ||
version = "0.10.0" | ||
version = "0.11.0" | ||
authors = ["Nick Cameron <[email protected]>"] | ||
edition = "2018" | ||
license = "Apache-2.0" | ||
|
@@ -16,8 +16,8 @@ prost-codec = ["syn", "quote", "prost-build"] | |
grpcio-prost-codec = ["grpcio-compiler/prost-codec", "prost-codec"] | ||
|
||
[dependencies] | ||
protobuf = { version = "2", optional = true } | ||
protobuf-codegen = { version = "2", optional = true } | ||
protobuf = { package = "nrc-protobuf", version = "2", optional = true } | ||
protobuf-codegen = { package = "nrc-protobuf-codegen", version = "2", optional = true } | ||
grpcio-compiler = { version = "0.5.0-alpha", default-features = false, optional = true } | ||
prost-build = { version = "0.5", optional = true } | ||
regex = { version = "1.1" } | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I feel worried about it, especially when using protobuf-build in grpc-rs. What's the problem of keeping using a branch?
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.
It means we can't publish kvproto, which blocks us publishing the Rust client.
I think I must make it a feature in any case because of grpcio-compiler.
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.
oh, what is "nrc-protobuf" 😭
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.
it's a minor fork of rust-proto to use Prost naming conventions and adding support for some TiKV-specific pretty printing
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.
I don't feel it right to let rust client to depend on a forked version of rust-proto either.
For "TiKV-specific thing", we can leave it inside TiKV project. For naming conventions, we better discussing with rust-protobuf project and get it merged into the project instead of creating a new one.
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.
I don't think they will accept the naming changes - it would be a big breaking change. OTOH, not using those changes makes supporting both Prost and rust-proto very messy.
I agree it is sub-optimal to use a forked version, but we are already doing that, the only change here is to use a published version of the fork.
And to clarify, if we want to publish KvProto, then we can't depend on Git repos - it is not allowed by crates.io. Whereas depending on a fork is sub-optimal, but not impossible (and we already use a forked version of GRPC among other things, which seems worse to me).
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.
rust-protobuf supports customization, I don't think it has to be a breaking change.
https://github.com/stepancheg/rust-protobuf/blob/53dc4ae8b2d20b3e6cb971c68042968a45d23882/protobuf-codegen/src/customize.rs#L10-L44
Forked version of gRPC is not the same use case here. Almost all changes to the forked are just bugfix cherry-picks, we don't mean to introduce any custom features to the forked version. And it will not introduce a different crate or make different types.