Replace script with build script, use features

[ice1000]

See pingcap/kvproto#349

Part of #177

Signed-off-by: ice1000 [email protected]

[ice1000]

@BusyJay @overvenus PTAL

[ice1000]

Diff in /home/travis/build/pingcap/raft-rs/build.rs at line 137:

         let text = regex.replace_all(
             &text,
             "if $1 == ::protobuf::wire_format::WireTypeVarint {\
-                $3 = $2.read_enum()?;\
+             $3 = $2.read_enum()?;\
              } else {\
-                return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type));\
+             return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type));\
              }",
         );

Why? It's asking me to generate some codes that will not pass the fmt check.
Ridiculous

[nrc]

Why? It's asking me to generate some codes that will not pass the fmt check.
Ridiculous

Bug in Rustfmt :-(

I think it is best to use #[rustfmt::skip] on this code

[nrc]

I think it would be better to share the code between kvproto and raft - we should either pull most of the buildscript out into its own crate, or build the raft proto in kvproto. OTOH if this is meant to just be temporary, then I think it is OK.

[ice1000]

OTOH if this is meant to just be temporary, then I think it is OK.

Does creating a separated library for sharing the codes in this build script sounds like a good idea?

[nrc]

Does creating a separated library for sharing the codes in this build script sounds like a good idea?

It does to me

[ice1000]

It does to me

Let's listen to @siddontang's idea

[siddontang]

before we finish all things, don't introduce prost in the project.

We need to verify in our own forked repo.

[ice1000]

@siddontang I've removed all prost dependencies, please take a look again.

[nrc]

LGTM, needs rustfmt-ing

[siddontang]

@ice1000

I think you can create a prost branch later, then we can introduce prost.

[overvenus]

There are other places use protobuf::, they also need to #[cfg(feature = "lib-rust-protobuf")]

[ice1000]

@ice1000

I think you can create a prost branch later, then we can introduce prost.

This is what I'm doing right now. I have a local dev branch dealing with prost and I'm trying my best to reduce prost-related things in this PR.

[ice1000]

Addressed @Hoverbear@'s and @overvenus@'s comments.

One task left: rewrite common.sh with rust

[ice1000]

So the current status of this PR is "blocked by protobuf dependency version selection"?

[ice1000]

Resolved merge conflicts.

[Hoverbear]

@ice1000 There are new conflicts from #184 . :( There is a merge resolve here: https://github.com/ice1000/raft-rs/pull/1 you can accept.

[ice1000]

Can we have a merge please?

Now it's redirected to another branch so no worry about breaking anything.

[Hoverbear]

@nrc is your LGTM still valid? :)

[nrc]

@ice1000 could you sort out the commit series please? Would be great to be based on master and not have the merge commits as well as having a coherent set of commits.

Could you use protobuf-build? Then we wouldn't need to depend on kvproto or duplicate code.

[ice1000]

@ice1000 could you sort out the commit series please?

Yeah sure

[ice1000]

protobuf-build is cool, will do.

[Hoverbear]

@nrc Is there a plan to publish that? Raft is a published crate, so it's dependencies must be as well.

[nrc]

@nrc Is there a plan to publish that? Raft is a published crate, so it's dependencies must be as well.

Yes, I think once we're ready to merge all the Prost work we can move it to PingCAP org and publish. For now it depends on a modified Prost so we can't.

[Hoverbear]

@nrc So you'd like to depend on it and prevent us from publishing a new release until the prost situation is solved?

[nrc]

@nrc So you'd like to depend on it and prevent us from publishing a new release until the prost situation is solved?

Yeah, I'm hopeful that won't take too long. We can make it work without the Prost dep if we need to publish a release before Prost does.

[Hoverbear]

Do you think we can just keep work to this branch while we get there?

[ice1000]

Yes.

[nrc]

Closing in favour of #201