Auto protobuf build

[dingxiangfei2009]

This is related to this merge request.

As a dependency of rusty_secrets, the current merkle crate is failing the build due to the recent breaking change in protobuf internal APIs. This is an attempt to fix the build failure by re-building the protobuf schemata automatically.

Note that rustfmt target in the Travis-CI build script is temporarily disabled, since the generated schema is not checked into the repository and it is not re-generated when rustfmt is run, while it has currently no capability to recognise conditional directives in the source code.

Protobuf is included into the cache by the build script, so that it can be reused in future builds on Travis-CI. The idea is taken from here. The cache is working as seen from this Travis log.

[psivesely]

Copying https://github.com/SpinResearch/RustySecrets/pull/69#issuecomment-391169475: would it be possible to ensure in build.rs that if the protoc binary found by protoc_rust is other than 3.5.1 the build fails with an informative error?

[romac]

LGTM. What do you think @nvesely?

[psivesely]

Edit: I think only the first two issues I bring up here should be considered potential blockers to merge. The second two I can fix in a follow-up PR unless @dingxiangfei2009 would like to take care of them as well. My issue comment https://github.com/SpinResearch/merkle.rs/pull/38#discussion_r190179425 also falls into this latter category.

Note that rustfmt target in the Travis-CI build script is temporarily disabled, since the generated schema is not checked into the repository and it is not re-generated when rustfmt is run, while it has currently no capability to recognise conditional directives in the source code.

I must not be understanding what @dingxiangfei2009 is saying here. Since the generated code will not be present in the rustfmt build (since rustfmt does not compile before checking and the generated code is not committed to the repository) why would it fail? If we do in fact need to disable it temporarily we should at least open an issue with a plan for a followup PR that would allow us to re-enable it.

---

https://github.com/SpinResearch/merkle.rs/pull/38/commits/e0af34702291a31722f77f62e620f6b38d29d789 seems like a good solution for Travis, but my intention when I wrote https://github.com/SpinResearch/merkle.rs/pull/38#issuecomment-391169689 was to suggest that the generated code should be consistent for all users. The build should fail on any platform where protoc 3.5.1 is not found.

---

src/proto/proof.rs should be added to .gitignore.

---

It would be nice if protoc-rust was an optional build dependency. E.g.,

build.rs:

#[cfg(feature = "serialization-protobuf")]
extern crate protoc_rust;

#[cfg(feature = "serialization-protobuf")]
fn build_protobuf(out_dir: &str, input: &[&str], includes: &[&str]) {
    use self::protoc_rust::{run, Args};
    run(Args {
        out_dir,
        input,
        includes,
    }).expect("protoc");
}

fn main() {
    #[cfg(feature = "serialization-protobuf")]
    build_protobuf("src/proto", &["protobuf/proof.proto"], &[]);
}

Cargo.toml (relevant sections):

[build-dependencies]
protoc-rust = { version = "1.7.1" , optional = true }

[dev-dependencies]
serde_json = "1.0.17"

[features]
serialization-protobuf = [ "protobuf", "protoc-rust" ]
serialization-serde = [ "serde", "serde_derive" ]

[dingxiangfei2009]

When I look into the rustfmt build, I find this error from rust-fmt in the Travis CI build log.

error[E0583]: file not found for module `proof`
 --> /home/travis/build/dingxiangfei2009/merkle.rs/src/proto/mod.rs:1:5
  |
1 | mod proof;
  |     ^^^^^
  |
  = help: name the file either proof.rs or proof/mod.rs inside the directory "/home/travis/build/dingxiangfei2009/merkle.rs/src/proto"

That missing proof sub-module is supposed to be generated from the protobuf schema. However, this build did not enable serialization-protobuf feature. The proof sub-module was not generated and so the error occurs. Interestingly, proto module is examined by rustfmt nonetheless because rustfmt does not know whether that serialization-protobuf feature is enabled or not. This is what I mean by rustfmt having no capability to understand compiler feature switches.

I also tried applying #[cfg_attr(rustfmt, rustfmt_skip)] directive to that import statement, but rustfmt still looks for the proof.rs file.

[psivesely]

How about running cargo build --verbose --all-features in the rustfmt Travis build (or faster/ better just running the necessary protoc command) and also creating a rustfmt.toml with just the line ignore = [ "src/proto/proof.rs", ]? I think this is a good immediate fix that will work.

It might be good also to file an issue for this in the rustfmt repo. It's possible there's either already a way to do this without having to do the build/ code generation (at least nothing in https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md seemed suitable to me), or there should be a way (e.g., by extending the interpretation of #[cfg_attr(rustfmt, rustfmt_skip)] / #[rustfmt::skip]).

[dingxiangfei2009]

Though I could not get rustfmt to ignore that import statement with rustfmt.toml, I used macro to hind this statement whenever rustfmt is run against the source code. rustfmt does not perform macro expansion, so this works.

Now the rustfmt target is added back to Travis CI.

[psivesely]

Though I could not get rustfmt to ignore that import statement with rustfmt.toml

The purpose of the rustfmt.toml is so rustfmt doesn't check the src/proto/proof.rs we just hypothetically generated with protoc in the rustfmt matrix build. Protoc code is not does not conform to rustfmt standards and the build would always fail.

[psivesely]

Okay, two more small things and then I'd say this is ready to merge:

1. Undo all of 830b4015c5fcb9d9193db56ec33321c02ac27999 except the change you made to .gitignore.
2. Add protoc --rust_out src/proto/ protobuf/proof.proto to the rustfmt matrix build just before running rustfmt.

I tested this locally and rustfmt passed, so it should work in Travis as well.

[dingxiangfei2009]

I do not know how you have configured protoc with this protoc-gen-rust plugin. I tried this locally and got complaints about this missing plugin. Travis reported something similar, and as expected the schema was not generated.

build.rs can generate the schema because it does not directly call protoc. It uses protobuf-codegen crate under the hood, which comes with this protoc-gen-rust plugin.

[psivesely]

This might work

    - env:
        - NAME='rustfmt'
        # use env so updating versions causes cache invalidation
        - PROTOBUF_CODEGEN_VERSION=2.0.0
        # So `protoc` can find `protoc-gen-rust`
        - PATH=$PATH:$HOME/cargo/bin
      rust: nightly
      before_script:
        - rustup component add rustfmt-preview
        # Protoc plugin needed to generate proof.rs from proof.proto
        - cargo install protobuf-codegen --version $PROTOBUF_CODEGEN_VERSION || echo "protobuf-codegen already installed"
        # TODO: see if we can avoid installing protobuf-codegen and generating
        # proof.rs in this build by using rustfmt options (see
        # https://github.com/SpinResearch/merkle.rs/pull/38#issuecomment-391336829,
        # paragraph 2).
        - protoc --rust_out src/proto/ protobuf/proof.proto
      script:
        - cargo fmt --all -- --check

[psivesely]

I think it's better to complicate the travis build than the code and build/ runtime dependencies, but ideally that TODO gets resolved at some point.

Also, referencing my yaml above, I'm not actually sure we need to pin a version of this package because when nightly updates I think it will necessarily rebuild. I haven't looked into this closely enough tbh, but my intuition is to remove it and then later add it back if needed. Also, I wonder if .cargo/bin is already automatically put in PATH on Travis Rust builds.

I'm actually a bit confused as to why protoc is available in the first place (and also how it knows protoc-gen-rust is needed for --rust_out, is a list of possible plugins hardcoded into protoc?). I guess it's not immediately clear that the before_install option would install protoc to all builds and not just the "top level" (i.e., not in the matrix builds). I guess all top-level options apply unless overwritten (e.g., script is used in every matrix build). I've been using Travis for years now and apparently am still confused at how it behaves.

[dingxiangfei2009]

Ok, I pushed in some changes as you have recommended. If this looks good to you, I will make a similar change to the other pull request in RustySecrets and fix a similar problem there.

[psivesely]

Blocked by #40. If you re-run Travis on this PR, it should fail.

[dingxiangfei2009]

Em, I don't see how Travis fails on this PR. It is still passing. The last commit is about adding some verbose messages in .travis.yml.

I am going to try merging #40 with this PR and see what happens.

[dingxiangfei2009]

Closing this PR since #44 attempts to properly build protobuf schema with a correct protoc-rust version.