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

Update C++ code generation to work with Bazel 0.29 . #19860

Merged
merged 1 commit into from
Aug 12, 2019
Merged

Update C++ code generation to work with Bazel 0.29 . #19860

merged 1 commit into from
Aug 12, 2019

Conversation

lberki
Copy link
Contributor

@lberki lberki commented Aug 6, 2019

The above Bazel version changes proto compilation slightly: some proto
files are put into a _virtual_imports directory and thus
_get_include_directory needs to be updated accordingly.

Ideally, it would use instead the ProtoInfo provider to tease out the
proto import directories, but that's a bit more intrusive change.

The above Bazel version changes proto compilation slightly: some proto
files are put into a `_virtual_imports` directory and thus
`_get_include_directory` needs to be updated accordingly.

Ideally, it would use instead the `ProtoInfo` provider to tease out the
proto import directories, but that's a bit more intrusive change.
@lberki
Copy link
Contributor Author

lberki commented Aug 6, 2019

This is to fix bazelbuild/bazel#9032 . It should be benign, since previous Bazel versions did not use the _virtual_imports directory.

@gnossen
Copy link
Contributor

gnossen commented Aug 6, 2019

Glad to see bazelbuild/bazel#7157 is getting taken care of. Thanks for the PR!

@lberki
Copy link
Contributor Author

lberki commented Aug 7, 2019

@gnossen , do I understand correctly that the above breakages are also at HEAD and thus okay?

@lberki
Copy link
Contributor Author

lberki commented Aug 7, 2019

It's my job to take care of these things! Unfortunately, this time, the collateral damage was greater than zero but it also made the way proto_library works significantly nicer, so there should be less breakages in the future.

@gnossen
Copy link
Contributor

gnossen commented Aug 7, 2019

@lberki Yes. The failures I listed above are known flakes or deterministic failures on master that should be unrelated to the changes in this PR and should not block merging.

@lberki
Copy link
Contributor Author

lberki commented Aug 9, 2019

(friendly ping)

Note that this is breaking Envoy and google-cloud-cpp, so it would be real nice if this got fixed.

@gnossen
Copy link
Contributor

gnossen commented Aug 9, 2019

CC @nicolasnoble @jtattermusch @a11r

@lberki
Copy link
Contributor Author

lberki commented Aug 12, 2019

(friendly ping)

@lberki
Copy link
Contributor Author

lberki commented Aug 12, 2019

Thanks! What's the process for merging this? (I don't have a bit fat green "Merge" button...)

@jtattermusch
Copy link
Contributor

Known failures:
#19877
#18616

@jtattermusch jtattermusch merged commit 5bf40ae into grpc:master Aug 12, 2019
@lberki
Copy link
Contributor Author

lberki commented Aug 13, 2019

Thanks! Now on for propagating this downstream (but that's my problem!)

@lock lock bot locked as resolved and limited conversation to collaborators Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants