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

DO NOT REVIEW #12482

Closed
wants to merge 1 commit into from
Closed

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Aug 28, 2023

I just want to see if the builds pass, and I am too lazy to run them by hand.

I would break this PR down into a few different pieces.


This change is Reviewable

@dbolduc dbolduc temporarily deployed to internal August 28, 2023 21:25 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (f8827ee) 93.62% compared to head (d5382a0) 93.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12482      +/-   ##
==========================================
- Coverage   93.62%   93.62%   -0.01%     
==========================================
  Files        2039     2039              
  Lines      180676   180676              
==========================================
- Hits       169160   169151       -9     
- Misses      11516    11525       +9     

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbolduc
Copy link
Member Author

dbolduc commented Aug 29, 2023

FWIW the publish-docs build fails because the grafeas protos were being generated in google/cloud/binaryauthorization/grafeas/v1/*.pb.h, whereas they used to be generated in google/cloud/grafeas/.

I think we would need to generalize the --cpp_out directory used by protoc.

Q: So the proto headers from #8022 used to be generated in ${build_out}/external/googleapis/google/bigtable/v2 (for example). Now they are in ${build_out}/google/cloud/bigtable/google/bigtable/v2.....

@coryan - Is that a problem?

@coryan
Copy link
Contributor

coryan commented Aug 29, 2023

FWIW the publish-docs build fails because the grafeas protos were being generated in google/cloud/binaryauthorization/grafeas/v1/*.pb.h, whereas they used to be generated in google/cloud/grafeas/.

That seems odd.

I think we would need to generalize the --cpp_out directory used by protoc.

Maybe. Or maybe we should use the functions Protobuf provides to generate code (assuming they exist in all versions of Protobuf we use and support).

Q: So the proto headers from #8022 used to be generated in ${build_out}/external/googleapis/google/bigtable/v2 (for example). Now they are in ${build_out}/google/cloud/bigtable/google/bigtable/v2.....

@coryan - Is that a problem?

As long as the -I flags are right, it should not matter where the *.pb.h files are generated. Protobuf has some unfortunate rules about import directories for protos, but these are not affected by the output directory.

@dbolduc
Copy link
Member Author

dbolduc commented Aug 29, 2023

That seems odd.

Yeah, that was plain wrong.

As long as the -I flags are right, it should not matter where the *.pb.h files are generated.

Ack, that's what I thought, but I wasn't completely sure. Thanks.

@dbolduc dbolduc closed this Aug 29, 2023
@dbolduc dbolduc deleted the cmake-refactor-grafeas branch November 16, 2023 12:50
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.

2 participants