rust: replace bash script with build script by nrc · Pull Request #347 · pingcap/kvproto

nrc · 2019-01-28T03:29:08Z

With this change, preparing the Rust code becomes the idiomatic cargo build, rather than requiring a script. We would like to migrate from rust-protobuf to Prost, and this change should make that easier.

The Cargo build script is pretty much a 1:1 re-implementation of the old script.

cc @ice1000

CLAassistant · 2019-01-28T03:29:16Z

All committers have signed the CLA.

overvenus

LGTM, thank you!

Cargo.toml

ice1000

LGTM

siddontang · 2019-01-29T13:45:29Z

LGTM

PTAL @BusyJay

BusyJay

Rest LGTM

build.rs

BusyJay · 2019-01-30T14:39:28Z

Another thought: it may be bad to modify the source directory. It's more natural to generate sources into the target build directory and link it into source directory. For example, for now whenever do a cargo clean && cargo build, git status will show that the workspace is in dirty state.

nrc · 2019-01-30T19:38:31Z

Another thought: it may be bad to modify the source directory

@BusyJay agree. I address this in #349 by making code generation opt-in, so a regular cargo build does not rebuild. I thought this is better than generating into the OUT_DIR because it means you can see the code and that might be helpful when debugging and so forth.

ice1000 · 2019-01-30T20:22:25Z

prost support will generate codes in OUT_DIR and use include! macro to get them compiled.

brson · 2019-01-30T20:23:56Z

Awesome start @nrc.

nrc · 2019-01-30T20:24:54Z

prost support will generate codes in OUT_DIR and use include! macro to get them compiled.

I don't think we have to do this - we could copy the files from the OUT_DIR to the src directory rather than use include!. I'm not sure which approach is better - using OUT_DIR is more idiomatic, but using src lets users see the generated code, which I think is valuable.

BusyJay · 2019-01-30T21:11:04Z

I'm not familiar with how cargo compile dependencies. If I compile several projects in parallel, which all depend on kvproto, will kvproto also be compiled in parallel? So will the source files get written concurrently? Is it a race condition that can lead to corrupted files?

BusyJay · 2019-01-30T21:14:17Z

...when debugging and so forth

Do you mean to debug it in the dependent project or inside kvproto? If debugging in dependent project, it may be easier to look at when the code is under target directory instead of $HOME/.cargo.

nrc · 2019-01-30T22:02:35Z

I'm not familiar with how cargo compile dependencies. If I compile several projects in parallel, which all depend on kvproto, will kvproto also be compiled in parallel? So will the source files get written concurrently? Is it a race condition that can lead to corrupted files?

It will be rebuilt for each project in the project's target directory - compiled code is not shared between projects, although there is work in the pipeline to do so

Do you mean to debug it in the dependent project or inside kvproto? If debugging in dependent project, it may be easier to look at when the code is under target directory instead of $HOME/.cargo.

Both. And actually just writing code as a downstream user - it is nice to look on GitHub and see the generated code.

BusyJay · 2019-01-31T04:50:59Z

compiled code is not shared between projects,

What I mean is the generated code, as they are flushed into the source directory.

nrc · 2019-01-31T06:16:21Z

What I mean is the generated code, as they are flushed into the source directory.

Oh I see, if there are several projects using kvptoto via a path dependency, then I think you could have issues. But I don't think that should happen (it would mean a path dep which points outside of its own repo), so I don't think we should worry.

In any case #349 changes things to opt-in regeneration so it will not be an issue. If you like we can close this PR and discuss #349, but I thought it would be easier to land one piece at a time.

BusyJay · 2019-01-31T06:27:22Z

OK, I will give my approval when mod_names is sorted.

Makes generating Rust code easier and should help with the Prost migration

The changes are due to: * change in version of rust-protobuf * non-determinism in file generation (stepancheg/rust-protobuf#256) * formatting

nrc · 2019-01-31T20:55:48Z

@BusyJay comment addressed and rebased PTAL

ice1000

Giving my approval back

* Replace generate_rust with build script Makes generating Rust code easier and should help with the Prost migration * Commit generated files The changes are due to: * change in version of rust-protobuf * non-determinism in file generation (stepancheg/rust-protobuf#256) * formatting * rust: sort module names

nrc requested review from BusyJay and Hoverbear January 28, 2019 03:29

nrc force-pushed the build-rs branch from 612a92f to ccf81ed Compare January 28, 2019 03:36

overvenus previously approved these changes Jan 28, 2019

View reviewed changes

BusyJay reviewed Jan 28, 2019

View reviewed changes

Cargo.toml Show resolved Hide resolved

ice1000 previously approved these changes Jan 28, 2019

View reviewed changes

ice1000 mentioned this pull request Jan 29, 2019

[WIP] Replace with prost #348

Closed

1 task

nrc mentioned this pull request Jan 29, 2019

rust: use features to support Prost or rust-protobuf #349

Merged

BusyJay reviewed Jan 30, 2019

View reviewed changes

build.rs Show resolved Hide resolved

nrc mentioned this pull request Jan 31, 2019

Support generating either Prost or rust-protobuf code #350

Closed

nrc dismissed stale reviews from ice1000 and overvenus via da87669 January 31, 2019 20:47

nrc force-pushed the build-rs branch from ccf81ed to da87669 Compare January 31, 2019 20:47

nrc added 3 commits February 1, 2019 09:52

Replace generate_rust with build script

20a71da

Makes generating Rust code easier and should help with the Prost migration

Commit generated files

3353132

The changes are due to: * change in version of rust-protobuf * non-determinism in file generation (stepancheg/rust-protobuf#256) * formatting

rust: sort module names

8231c2a

nrc force-pushed the build-rs branch from da87669 to 8231c2a Compare January 31, 2019 20:54

ice1000 approved these changes Jan 31, 2019

View reviewed changes

BusyJay approved these changes Feb 1, 2019

View reviewed changes

ice1000 merged commit f9b9e7d into pingcap:master Feb 1, 2019

This was referenced Feb 13, 2019

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

Closed

Refine CI tikv/client-rust#22

Merged

breezewish mentioned this pull request Feb 25, 2019

Needs protoc dependency tikv/tikv#4271

Closed

Conversation

nrc commented Jan 28, 2019

Uh oh!

CLAassistant commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overvenus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ice1000 left a comment

Choose a reason for hiding this comment

Uh oh!

siddontang commented Jan 29, 2019

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BusyJay commented Jan 30, 2019

Uh oh!

nrc commented Jan 30, 2019

Uh oh!

ice1000 commented Jan 30, 2019

Uh oh!

brson commented Jan 30, 2019

Uh oh!

nrc commented Jan 30, 2019

Uh oh!

BusyJay commented Jan 30, 2019

Uh oh!

BusyJay commented Jan 30, 2019

Uh oh!

nrc commented Jan 30, 2019

Uh oh!

BusyJay commented Jan 31, 2019

Uh oh!

nrc commented Jan 31, 2019

Uh oh!

BusyJay commented Jan 31, 2019

Uh oh!

nrc commented Jan 31, 2019

Uh oh!

ice1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CLAassistant commented Jan 28, 2019 •

edited

Loading