proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

Yannic · 2019-12-27T15:10:50Z

rebello95 · 2019-12-31T00:54:26Z

Thanks for opening this, @Yannic!

I was able to confirm that cleaning/building Envoy Mobile with this patch applied to the bazel binary resolves the issue we're seeing in envoyproxy/envoy-mobile#617 🎉

Fixes #617, which resulted in duplicate symbols from protobuf being present in the final Envoy Mobile binary. The [root cause](bazelbuild/bazel#10484) is being [patched in Bazel](bazelbuild/bazel#10493). In the meantime, we can revert these two commits in protobuf ([first](protocolbuffers/protobuf@7b28278), [second](protocolbuffers/protobuf@a03d332)) and apply the diff as a patch until we're able to update Bazel with the fix. More context on the protobuf issue is available [here](protocolbuffers/protobuf#7046). Notes: - This should be reverted after updating Bazel with the fix - The version of protobuf specified in the `http_archive` should be in lockstep with upstream Envoy's version - Upstream Envoy's `@envoy//bazel:protobuf.patch` patch is also included here Signed-off-by: Michael Rebello <me@michaelrebello.com>

keith · 2020-01-02T07:18:45Z

@Yannic any change this could get merged soon so we can make the next cut?

gnossen · 2020-01-07T23:33:09Z

@Yannic Bump. Great change! This PR resolves an issue that's been blocking gRPC from upgrading its version of protobuf.

Yannic · 2020-01-08T11:51:29Z

@cushon landed a similar change (01df1e5), but the issue was the difference between direct sources and original direct sources, so I don‘t think the issue is resolved by that commit.

This issue is blocking users from upgrading past Protobuf 3.9(?), so it would be great if we could get it merged soon.

/cc @laurentlb

lberki · 2020-01-09T09:13:31Z

I was about to lament that #getOriginalDirectProtoSources() is arguably a hack and demand that you at least add a test case but then I realized that:

You actually have a test case
It was me who did not add a test case in 46c95dc
3 (I don't know what I was thinking)
It might actually be better to check for the direct source file since (at least in theory) the same source file might give rise to multiple virtual ones if multiple proto_library rules reference it (I haven't thought it through as to whether that's a realistic risk)

Let me try to run this through our internal test battery.

lberki · 2020-01-09T09:21:34Z

...aaand, failures :(

Yannic · 2020-01-09T12:06:14Z

I agree that this is a hack, but blacklisted_protos = ["a.proto", "b.proto"] will always reference the original file, so we have to filter by original files for now. We probably can (and should!) get rid of getOriginalDirectProtoSources() (& friends) when blacklisted_protos requires the ProtoInfo provider.

Let me know if there's anything I can do to fix the failing tests.

cushon · 2020-01-10T06:14:04Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java

-        blacklistedProtos.addTransitive(protoInfo.getTransitiveProtoSources());
+        // TODO(yannic): Switch to getTransitiveProtoSources() and remove references to
+        // original proto sources when ProtoInfo is mandatory.
+        blacklistedProtos.addAll(protoInfo.getOriginalDirectProtoSources());


Using direct instead of transitive sources un-does a change made in 01df1e5 that I think is going to break some uses of proto_lang_toolchain.blacklisted_protos.

Yep, this is exactly what I found out after debugging the aforementioned test failures. Adding both here isn't very nice so I'll try adding the set of transitive original sources (I'd totally ask Yannic to do so, but since the tests that break are internal, he'd be flying blind...)

...and it blends! I'm sending this out (+my changes) for internal review.

meteorcloudy · 2020-01-14T14:21:08Z

This PR is causing some breakage in the downstream:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1349

Culprit finder result on Envoy:
https://buildkite.com/bazel/culprit-finder/builds/248#ca25feb7-a89b-43a4-ac25-4c8522c0e197

/cc @buchgr

Yannic · 2020-01-14T14:40:02Z

Suspecting that this shouldn't have been changed to transitive original sources: 4162cc5#diff-42ff97786b96eb7e34507430633100d5L279

This will fail for all proto_library targets that have at least one blacklisted source as a transitive dependency.

meteorcloudy · 2020-01-15T10:35:47Z

@Yannic Can you send a fix for this?

meteorcloudy · 2020-01-15T10:55:27Z

@Yannic Sorry, I'll rollback this change to get our CI green again.

*** Reason for rollback *** Broken Bazel CI in Downstream pipeline #10588 *** Original change description *** proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos Fixes #10484 Closes #10493. PiperOrigin-RevId: 289828390

…dProtos Fixes bazelbuild#10484 Closes bazelbuild#10493. PiperOrigin-RevId: 289411825

Fixes envoyproxy/envoy-mobile#617, which resulted in duplicate symbols from protobuf being present in the final Envoy Mobile binary. The [root cause](bazelbuild/bazel#10484) is being [patched in Bazel](bazelbuild/bazel#10493). In the meantime, we can revert these two commits in protobuf ([first](protocolbuffers/protobuf@7b28278), [second](protocolbuffers/protobuf@a03d332)) and apply the diff as a patch until we're able to update Bazel with the fix. More context on the protobuf issue is available [here](protocolbuffers/protobuf#7046). Notes: - This should be reverted after updating Bazel with the fix - The version of protobuf specified in the `http_archive` should be in lockstep with upstream Envoy's version - Upstream Envoy's `@envoy//bazel:protobuf.patch` patch is also included here Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

proto_lang_toolchain: Add original sources of ProtoInfo to blackliste…

6375e57

…dProtos Fixes bazelbuild#10484

Yannic requested a review from hlopko as a code owner December 27, 2019 15:10

googlebot added the cla: yes label Dec 27, 2019

Yannic mentioned this pull request Dec 27, 2019

proto_lang_toolchain and blacklisted_protos with ProtoInfo not propagated correctly #10484

Closed

rebello95 mentioned this pull request Dec 28, 2019

core: duplicate symbols coming from protobuf envoyproxy/envoy-mobile#617

Closed

rebello95 mentioned this pull request Dec 31, 2019

core: apply protobuf patch to prevent duplicate symbols envoyproxy/envoy-mobile#618

Merged

irengrig removed the request for review from hlopko January 2, 2020 14:13

irengrig added the team-Rules-Server Issues for serverside rules included with Bazel label Jan 2, 2020

irengrig assigned lberki Jan 2, 2020

gnossen mentioned this pull request Jan 7, 2020

Update third_party/protobuf to v3.11.2 grpc/grpc#21590

Merged

Merge branch 'master' into fix_10484

5984994

Yannic mentioned this pull request Jan 8, 2020

Blacklist .proto source files if Bazel allows us to protocolbuffers/protobuf#7065

Merged

cushon requested changes Jan 10, 2020

View reviewed changes

Yannic mentioned this pull request Jan 10, 2020

[bazel] Fix blacklisted_protos in cc_toolchain and add test protocolbuffers/protobuf#7075

Merged

bazel-io closed this in 4162cc5 Jan 13, 2020

Yannic deleted the fix_10484 branch January 14, 2020 14:36

Yannic added a commit to Yannic/bazel that referenced this pull request Jan 15, 2020

proto_lang_toolchain: Add original sources of ProtoInfo to blackliste…

66c6012

…dProtos Fixes bazelbuild#10484 Closes bazelbuild#10493. PiperOrigin-RevId: 289411825

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

Yannic commented Dec 27, 2019

rebello95 commented Dec 31, 2019

keith commented Jan 2, 2020

gnossen commented Jan 7, 2020

Yannic commented Jan 8, 2020

lberki commented Jan 9, 2020

lberki commented Jan 9, 2020

Yannic commented Jan 9, 2020

cushon Jan 10, 2020

lberki Jan 10, 2020 •

edited

lberki Jan 10, 2020

meteorcloudy commented Jan 14, 2020 •

edited

Yannic commented Jan 14, 2020

meteorcloudy commented Jan 15, 2020

meteorcloudy commented Jan 15, 2020

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

Conversation

Yannic commented Dec 27, 2019

rebello95 commented Dec 31, 2019

keith commented Jan 2, 2020

gnossen commented Jan 7, 2020

Yannic commented Jan 8, 2020

lberki commented Jan 9, 2020

lberki commented Jan 9, 2020

Yannic commented Jan 9, 2020

cushon Jan 10, 2020

Choose a reason for hiding this comment

lberki Jan 10, 2020 • edited

Choose a reason for hiding this comment

lberki Jan 10, 2020

Choose a reason for hiding this comment

meteorcloudy commented Jan 14, 2020 • edited

Yannic commented Jan 14, 2020

meteorcloudy commented Jan 15, 2020

meteorcloudy commented Jan 15, 2020

lberki Jan 10, 2020 •

edited

meteorcloudy commented Jan 14, 2020 •

edited