Skip to content

Auto protobuf schema generation with Protobuf 2.0.2#44

Merged
romac merged 20 commits into
SpinResearch:masterfrom
dingxiangfei2009:refactor/new-auto-protobuf-gen
Jul 19, 2018
Merged

Auto protobuf schema generation with Protobuf 2.0.2#44
romac merged 20 commits into
SpinResearch:masterfrom
dingxiangfei2009:refactor/new-auto-protobuf-gen

Conversation

@dingxiangfei2009

Copy link
Copy Markdown
Contributor

Referring to #38, #39 and #40.

This is a merge between #38 and #40. A new change in this PR is bumping protoc-rust version to match the version of protobuf being used.

@psivesely

psivesely commented Jun 18, 2018

Copy link
Copy Markdown
Contributor

When this is ready if you could clean up the git history (mostly the most recent commits) that would be 💯 and I'll close #40.

@dingxiangfei2009 dingxiangfei2009 force-pushed the refactor/new-auto-protobuf-gen branch from 85226f2 to 2ae6dda Compare June 19, 2018 02:09
@dingxiangfei2009

Copy link
Copy Markdown
Contributor Author

I just remove the wip commits from this PR.

@psivesely

Copy link
Copy Markdown
Contributor

You have 3.5.1 in 6 places in 2 files. It would be nice if we could set the protobuf version in a single place. E.g., create a top-level file called protobuf-version.txt that just reads 3.5.1 and then have build.rs and install-protobuf.sh both read that file. Or if you have another idea for how to simply maintain a single point of version declaration.

@dingxiangfei2009

dingxiangfei2009 commented Jun 20, 2018

Copy link
Copy Markdown
Contributor Author

I have an idea to implement this. I will put libprotoc 3.5.1 string into PROTOC_VERSION file at the project root. build.rs will confirm the version of protoc from the (compile time) PROTOC_VERSION environment variable. I will also set up the Travis CI to load the version file into the environment variable.

@dingxiangfei2009 dingxiangfei2009 force-pushed the refactor/new-auto-protobuf-gen branch from b05987d to 46f0dd3 Compare June 20, 2018 08:04
@dingxiangfei2009 dingxiangfei2009 force-pushed the refactor/new-auto-protobuf-gen branch from 5c4961e to cca8d3e Compare June 20, 2018 08:34

@psivesely psivesely left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@psivesely

Copy link
Copy Markdown
Contributor

Why did you get rid of cargo cache? Could you have avoided whatever problem by adding a call to cargo update before building?

@dingxiangfei2009

Copy link
Copy Markdown
Contributor Author

Oh yes, my bad. Making a new change now.

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.

3 participants