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

[bazel] Correctly handle proto files under _virtual_imports #1095

Merged
merged 2 commits into from
Dec 16, 2019

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Dec 9, 2019

This can happen if:

  1. proto_library contains at least one of import_prefix/strip_import_prefix, or
  2. (for Bazel >= 1.0) at least one of proto_library's srcs is generated.

Fixes #1094

@prestonvanloon I took the liberty to include a commit from your repro in this PR to have a target that fails without the patch and passes with it. I hope you don't mind :).

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@Yannic Yannic changed the title Fix proto [bazel] Correctly handle proto files under _virtual_imports Dec 9, 2019
@Yannic Yannic force-pushed the fix_proto branch 3 times, most recently from d04975d to 7f6f6ac Compare December 9, 2019 20:15
This can happen if:
  1. `proto_library` contains at least one of `import_prefix/strip_import_prefix`, or
  2. (for Bazel >= 1.0) at least one of `proto_library`'s `srcs` is generated.

Fixes grpc-ecosystem#1094
@codecov-io
Copy link

codecov-io commented Dec 9, 2019

Codecov Report

Merging #1095 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1095   +/-   ##
=======================================
  Coverage   53.89%   53.89%           
=======================================
  Files          42       42           
  Lines        4180     4180           
=======================================
  Hits         2253     2253           
  Misses       1681     1681           
  Partials      246      246

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c06998...612e0b6. Read the comment docs.

Copy link
Contributor

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

I’m glad I could help! Thanks

@achew22
Copy link
Collaborator

achew22 commented Dec 10, 2019

Sorry, I spent all day on an airplane. I'll confirm the CLA stuff in the morning and merge

@prestonvanloon
Copy link
Contributor

This PR doesn't totally resolve my issue.

When I use this in my real project, I am still having virtual import issues.

eth/v1alpha1/attestation.proto: File not found.
eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/beacon_block.proto: Import "eth/v1alpha1/attestation.proto" was not found or had errors.
eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/beacon_block.proto:67:14: "ethereum.eth.v1alpha1.Attestation" seems to be defined in "eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/attestation.proto", which
is not imported by "eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/beacon_block.proto".  To use it here, please add the necessary import.
eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/beacon_block.proto:174:5: "ethereum.eth.v1alpha1.AttestationData" seems to be defined in "eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/attestation.proto", which is not imported by "eth/v1alpha1/_virtual_imports/proto/eth/v1alpha1/beacon_block.proto".  To use it here, please add the necessary import.

Link to PR: prysmaticlabs/ethereumapis#33

Any idea @Yannic ?

@Yannic
Copy link
Contributor Author

Yannic commented Dec 10, 2019

@prestonvanloon This is weird. I see there is //tools:proto_descriptor_sets.bzl which contains the same mistake I fixed here, so just to make sure the failure you mention above is caused by protoc_gen_swagger, can you run bazel build //eth/v1alpha1:swagger in your repo and report back? If that works, I'll guide you through fixing proto_descriptor_sets.

@prestonvanloon
Copy link
Contributor

Confirmed bazel build //eth/v1alpha1:swagger works! I forgot about the proto descriptor set rule.

Maybe that could be a feature for rules_bazel? I needed the descriptor set to give to envoy grpc gateway proxy.

@Yannic
Copy link
Contributor Author

Yannic commented Dec 10, 2019

I assume you mean rules_proto. Can you open a feature request there?

If you need a single descriptor set with transitive imports, I recommend
bazel build --protocopt=--include_imports --protocopt=--include_source_info //eth/v1alpha1:proto for now. (Disclaimer: Don't put these --protocopt into your .bazelrc, these descriptor sets can get quite large which does impact build performance.)

@prestonvanloon
Copy link
Contributor

Oh yes, I meant rules_proto. Sorry about that! Oh awesome, this resolves my problem for now. Thanks!

prestonvanloon added a commit to prysmaticlabs/ethereumapis that referenced this pull request Dec 10, 2019
@Yannic
Copy link
Contributor Author

Yannic commented Dec 16, 2019

@achew22 friendly ping?

@achew22
Copy link
Collaborator

achew22 commented Dec 16, 2019

@Yannic, thanks for pinging. Sorry about that.

@prestonvanloon could you follow the instructions from the CLA bot:

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Based on the dashboard I think that should be sufficient. Thanks so much!

@prestonvanloon
Copy link
Contributor

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@achew22
Copy link
Collaborator

achew22 commented Dec 16, 2019

Let's ship it. Thanks to both of you!

@achew22 achew22 merged commit 50c55a9 into grpc-ecosystem:master Dec 16, 2019
@Yannic Yannic deleted the fix_proto branch December 16, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot generate swagger with generated protobuf
5 participants