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

[feature request] Verify Compiled Protos on Travis #3094

Closed
cfromknecht opened this issue May 17, 2019 · 4 comments · Fixed by #4042
Closed

[feature request] Verify Compiled Protos on Travis #3094

cfromknecht opened this issue May 17, 2019 · 4 comments · Fixed by #4042
Labels
feature request Requests for new features intermediate Issues suitable for developers moderately familiar with the codebase and LN protos travis Modifications to the Travis CI system

Comments

@cfromknecht
Copy link
Contributor

As pointed out by @xsb in #2312, different authors may have different proto versions installed when doing development. To ensure all PRs use the correct of protoc and plugins, we should move to have Travis verify that the output is correct, and fail the build otherwise.

Steps to Completion

  1. Have travis build and install the protobuf dependencies specified here.
  2. Cache the protobuf and plugin source directories in the travis build cache, so that it doesn't need to be recompiled from scratch on every build.
  3. Add a new make target to Makefile, e.g. rpc-check, that first runs the rpc target, and then asserts that the source tree is clean. This can be done by using $(shell git describe --dirty) and asserting that the output doesn't contain the string "dirty"
  4. Add the rpc-check target as a dependency to travis-race, travis-itest, and travis-cover, preferably after lint so that the more expensive operations are done only if the protos are sound.
@cfromknecht cfromknecht added travis Modifications to the Travis CI system feature request Requests for new features protos makefile labels May 17, 2019
@halseth halseth added beginner Issues suitable for new developers good first issue Issues suitable for first time contributors to LND labels May 20, 2019
@johng
Copy link
Contributor

johng commented May 22, 2019

Would it not be better to not have the generated protobuf files in source control? Instead the generation would be part of the build step.

@cfromknecht
Copy link
Contributor Author

cfromknecht commented May 24, 2019

@johng the protos should still be checked into source. The intent is that by recompiling them in Travis, we should be able to tell if they’re correct if doesn’t leave the source tree in a dirty state

I started working on this in #3012 and #3010

@johng
Copy link
Contributor

johng commented May 28, 2019

@johng the protos should still be checked into source

Why?

@cfromknecht
Copy link
Contributor Author

@johng so that installing/building grpc and the related plugins isn’t required to compile lncli

@cfromknecht cfromknecht added intermediate Issues suitable for developers moderately familiar with the codebase and LN and removed beginner Issues suitable for new developers good first issue Issues suitable for first time contributors to LND labels Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for new features intermediate Issues suitable for developers moderately familiar with the codebase and LN protos travis Modifications to the Travis CI system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants