rust: use features to support Prost or rust-protobuf by nrc · Pull Request #349 · pingcap/kvproto

nrc · 2019-01-29T19:56:30Z

This change is backwards compatible for clients, and will allow us to develop Prost support without removing rust-protobuf. Based on top of #347

cc @ice1000

ice1000

Great work! I can't wait to get this merged.

build.rs

ice1000 · 2019-01-30T13:31:21Z

Comment for future development: the local mod name protobuf is the same as the library protobuf. If this library also uses API from the external crate protobuf, there'll be name conflict problems.
Sounds like this library doesn't, it's fine to use the name.

build.rs

nrc · 2019-02-04T03:12:44Z

rebased and adds in @ice1000 's refactoring.

PTAL @overvenus and @BusyJay

siddontang · 2019-02-04T06:37:36Z

@nrc

can we create a prost branch for all associated projects - kvproto, TiKV, grpc-rs, raft-rs

IMO, I don't want to introduce prost feature in master now.

nrc · 2019-02-04T06:39:07Z

can we create a prost branch for all associated projects - kvproto, TiKV, grpc-rs, raft-rs

Sure. How should we handle review? When we land on the branch or when we merge to master?

siddontang · 2019-02-04T14:14:39Z

@nrc

You can drive this as fast as you can to prove it can work(you may work with @ice1000 and @overvenus ). Then you can send a PR and we can review it carefully.

You already have the privilege to create the branch.

build.rs

BusyJay · 2019-02-20T09:13:49Z

build.rs

+    // as a dependency. Therefore, the user must opt-in to regenerating the
+    // Rust files.
+    if env::var_os("CARGO_FEATURE_REGENERATE").is_none() {
+        println!("cargo:rerun-if-changed=build.rs");


Shouldn't the changes inside proto directory trigger rebuild too?

I think only if we are regenerating the proto files

Changes to proto files only make sense when code is generated.

aha, can we regenerate them all the time?

We don't want to generate them all the time, we want to opt-in to building so that downstream crates don't require protoc

This is not a good idea. If we use kvproto as a dependency, all the .protos are treated as "changed" locally so a regeneration will get triggered if we do so.

In my case, when building TiKV, I get this message:

--- stdout cargo:rerun-if-changed=proto/errorpb.proto cargo:rerun-if-changed=proto/kvrpcpb.proto cargo:rerun-if-changed=proto/raft_serverpb.proto cargo:rerun-if-changed=proto/coprocessor.proto cargo:rerun-if-changed=proto/pdpb.proto cargo:rerun-if-changed=proto/import_sstpb.proto cargo:rerun-if-changed=proto/metapb.proto cargo:rerun-if-changed=proto/raft_cmdpb.proto cargo:rerun-if-changed=proto/debugpb.proto cargo:rerun-if-changed=proto/tikvpb.proto cargo:rerun-if-changed=proto/import_kvpb.proto

where I don't expect one.

nrc · 2019-02-25T00:35:26Z

@siddontang there is no Prost dep here now, do you think it's OK to land it in order to make protoc etc., not required to build? (#354)

siddontang · 2019-02-25T02:08:05Z

Cargo.toml

-protoc-rust = "2.0"
-protoc-grpcio = "0.3.1"
-regex = "1.1"
+kvproto-build = { git = "https://github.com/nrc/kvproto-build.git" }


I like this, but seem this repo is common for all the protobuf generated, not only for kvproto.

So maybe we can rename to protobut-build?

siddontang · 2019-02-25T02:10:33Z

Hi @nrc

SGTM

nrc · 2019-02-25T07:13:14Z

@BusyJay @overvenus @ice1000 PTAL I'd like to land this PR

overvenus

LGTM, thank you!

ice1000

LGTM, thank you!

As suggested in tikv/raft-rs#175

Also removes use of features in build.rs and use cfg! instead of env var.

nrc · 2019-03-03T22:43:03Z

Rebased and using a published version of protobuf-build.

No substantial changes, so I don't think this needs re-review and can be merged once CI passes.

nrc · 2019-03-04T08:02:36Z

@siddontang or @BusyJay or @overvenus please could you re-approve?

* 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> * Documentation in src/lib.rs * Add [cfg(feature = "lib-rust-protobuf")] to every protobuf places * Replace common.sh with rust code 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> * Fix typo-caused compilation error Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Fix compilation errors Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Add rsprost file to git repo Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Save progress * Save progress * Successfully generate! * Refactor, only one error is left now * Fix all errors occurred in `src`, start working on `tests` * Test compiles, remove rust-protobuf support * Compatible with stable rust, pass 150 tests now Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Sounds like all tests are passed Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Passing all doc-tests Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Use git dependency instead of relative path Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Partially address comments Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Fix tests after merge Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Rename generated mod to `prost` from `rsprost` Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Remove `RepeatedField::from_vec` Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * 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 because 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> * 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> * Revert unexpected comment changes Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

* rust: use features to support Prost or rust-protobuf * Rust: remove unnecessary `extern crate`s * Use features in generated code * Rename the feature flags and make code generation opt-in * Refactor build script As suggested in tikv/raft-rs#175 * Use kvproto-build crate * Change feature names to match GRPC-rs Also removes use of features in build.rs and use cfg! instead of env var. * Use published protobuf-build

* 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> * Documentation in src/lib.rs * Add [cfg(feature = "lib-rust-protobuf")] to every protobuf places * Replace common.sh with rust code 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> * Fix typo-caused compilation error Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Fix compilation errors Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Add rsprost file to git repo Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Save progress * Save progress * Successfully generate! * Refactor, only one error is left now * Fix all errors occurred in `src`, start working on `tests` * Test compiles, remove rust-protobuf support * Compatible with stable rust, pass 150 tests now Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Sounds like all tests are passed Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Passing all doc-tests Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Use git dependency instead of relative path Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Partially address comments Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Fix tests after merge Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Rename generated mod to `prost` from `rsprost` Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * Remove `RepeatedField::from_vec` Signed-off-by: ice1000 <ice1000kotlin@foxmail.com> * 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 because 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> * 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> * Revert unexpected comment changes Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

nrc force-pushed the feature branch from d511e51 to dc22240 Compare January 30, 2019 09:41

ice1000 previously approved these changes Jan 30, 2019

View reviewed changes

ice1000 reviewed Jan 30, 2019

View reviewed changes

build.rs Outdated Show resolved Hide resolved

This was referenced Jan 30, 2019

Replace script with build script, use features tikv/raft-rs#175

Closed

rust: replace bash script with build script #347

Merged

overvenus reviewed Jan 30, 2019

View reviewed changes

build.rs Outdated Show resolved Hide resolved

nrc mentioned this pull request Jan 31, 2019

Support generating either Prost or rust-protobuf code #350

Closed

nrc dismissed ice1000’s stale review via 232957c February 4, 2019 03:10

nrc force-pushed the feature branch from dc22240 to 232957c Compare February 4, 2019 03:10

nrc mentioned this pull request Feb 13, 2019

Installation requires cmake, protoc, golang, perl... #354

Closed

4 tasks

BusyJay reviewed Feb 20, 2019

View reviewed changes

nrc force-pushed the feature branch 2 times, most recently from 008a6a4 to a9f6579 Compare February 25, 2019 00:32

siddontang reviewed Feb 25, 2019

View reviewed changes

nrc force-pushed the feature branch from a9f6579 to 305f250 Compare February 25, 2019 02:19

overvenus previously approved these changes Feb 25, 2019

View reviewed changes

ice1000 previously approved these changes Feb 25, 2019

View reviewed changes

nrc dismissed stale reviews from ice1000 and overvenus via 5a4e30d February 25, 2019 20:34

nrc force-pushed the feature branch from 305f250 to 5a4e30d Compare February 25, 2019 20:34

nrc added 2 commits March 4, 2019 11:32

rust: use features to support Prost or rust-protobuf

b5999ba

Rust: remove unnecessary extern crates

48fa8fd

nrc added 5 commits March 4, 2019 11:33

Use features in generated code

76978fd

Rename the feature flags and make code generation opt-in

a7fc324

Refactor build script

2c2dabe

As suggested in tikv/raft-rs#175

Use kvproto-build crate

afef2f7

Change feature names to match GRPC-rs

a8db0e1

Also removes use of features in build.rs and use cfg! instead of env var.

nrc force-pushed the feature branch from 5a4e30d to a8db0e1 Compare March 3, 2019 22:42

Use published protobuf-build

12f08b3

ice1000 approved these changes Mar 3, 2019

View reviewed changes

overvenus approved these changes Mar 5, 2019

View reviewed changes

ice1000 merged commit ab7debc into pingcap:master Mar 5, 2019

Conversation

nrc commented Jan 29, 2019

Uh oh!

ice1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ice1000 commented Jan 30, 2019

Uh oh!

Uh oh!

nrc commented Feb 4, 2019

Uh oh!

siddontang commented Feb 4, 2019

Uh oh!

nrc commented Feb 4, 2019

Uh oh!

siddontang commented Feb 4, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Feb 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddontang commented Feb 25, 2019

Uh oh!

nrc commented Feb 25, 2019

Uh oh!

overvenus left a comment

Choose a reason for hiding this comment

Uh oh!

ice1000 left a comment

Choose a reason for hiding this comment

Uh oh!

nrc commented Mar 3, 2019

Uh oh!

nrc commented Mar 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants