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

Upgrade protobuf to 1.6. #60

Merged
merged 4 commits into from
May 18, 2018
Merged

Upgrade protobuf to 1.6. #60

merged 4 commits into from
May 18, 2018

Conversation

hicqu
Copy link
Contributor

@hicqu hicqu commented May 14, 2018

Upgrade protobuf to 1.6.

@hicqu hicqu requested review from BusyJay, overvenus and Hoverbear May 14, 2018 06:27
@BusyJay
Copy link
Member

BusyJay commented May 14, 2018

I don't think it's a good idea. This will require protoc on the build machine and also update source directly at compile time. The proto file barely changes, it doesn't have to be rebuild again and again.

@hicqu hicqu changed the title Upgrade protobuf to 1.6 and make eraftpb.rs be auto-generated. Upgrade protobuf to 1.6. May 14, 2018
@siddontang
Copy link
Contributor

LGTM

@siddontang
Copy link
Contributor

I think we should add a comment about how to generate the proto.

@siddontang
Copy link
Contributor

PTAL @BusyJay

BusyJay
BusyJay previously approved these changes May 14, 2018
Hoverbear
Hoverbear previously approved these changes May 14, 2018
@Hoverbear
Copy link
Contributor

@hicqu This looks ok but I'd also like to see some instruction how to regenerate the proto if needed.

@siddontang
Copy link
Contributor

any update?

@hicqu hicqu dismissed stale reviews from Hoverbear and BusyJay via ec3e1b9 May 15, 2018 10:01
@hicqu
Copy link
Contributor Author

hicqu commented May 15, 2018

PTAL @Hoverbear .

If proto file `eraftpb.proto` changed, run the command to regenerate `eraftpb.rs`:

```bash
protoc proto/eraftpb.proto --rust_out=src
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to set the output path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's seted with --rust_out.

@hicqu hicqu merged commit 63570e6 into tikv:master May 18, 2018
@hicqu hicqu deleted the upgrade-protobuf branch May 18, 2018 04:25
@Hoverbear Hoverbear modified the milestone: 0.2.0 Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants