cc_proto_library does not filter blacklisted protos inside the protobuf repo

[Yannic]

For Bazel //:src/google/protobuf/any.proto and @com_google_protobuf//:src/google/protobuf/any.proto are different files, which causes blacklisting of .proto sources to fail inside the protobuf repo.

protocolbuffers/protobuf#7075 (comment)

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

git clone --branch proto_blacklist_test https://github.com/Yannic/protobuf.git
cd protobuf
bazel test @com_google_protobuf//:cc_proto_blacklist_test //:cc_proto_blacklist_test
# @com_google_protobuf//:cc_proto_blacklist_test passes
# //:cc_proto_blacklist_test fails
bazel test --proto_toolchain_for_cc=//:cc_toolchain @com_google_protobuf//:cc_proto_blacklist_test //:cc_proto_blacklist_test
# //:cc_proto_blacklist_test passes
# @com_google_protobuf//:cc_proto_blacklist_test fails

What operating system are you running Bazel on?

N/A

What's the output of bazel info release?

Tested with several versions between 0.22.0 and 2.0.0

Any other information, logs, or outputs that you want to share?

This is mostly an issue for the protobuf repo or for users defining their own proto_lang_toolchain, which shouldn't be too many.

I see two possible fixes:

• a: Teach Bazel that //:src/google/protobuf/any.proto and @com_google_protobuf//:src/google/protobuf/any.proto are equivalent.
• b: Filter .proto files based on their import path.

b Is blocked on making ProtoInfo mandatory for proto_lang_toolchain#blacklisted_protos (cc @cushon). I have no idea how hard a would be. Is it related to --incompatible_remap_main_repo (cc @aehlig)?

[oquenchil]

It looks like those PRs should have fixed this. Closing.

[Yannic]

No they have not, it still doesn't work.

[meteorcloudy]

https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2273#f6cb5743-edc1-464d-aaf3-91ded9be20c3

This trick bazelbuild/continuous-integration@f02992e no longer works after 630be02

/cc @Wyverald

[Wyverald]

I don't completely understand what the original bug is about. So I cloned the protocolbuffers/protobuf repo, and tried to run the tests in the top comment with last_green bazel:

bazel test @com_google_protobuf//:cc_proto_blacklist_test //:cc_proto_blacklist_test
# //:cc_proto_blacklist_test fails
bazel test --proto_toolchain_for_cc=//:cc_toolchain @com_google_protobuf//:cc_proto_blacklist_test //:cc_proto_blacklist_test
# //:cc_proto_blacklist_test passes

It seems like Bazel now considers @com_google_protobuf//:cc_proto_blacklist_test and //:cc_proto_blacklist_test to be the same target (which seems correct to me). However, the test is failing without manually setting --proto_toolchain_for_cc=//:cc_toolchain. Any more insight here?

[Yannic]

The .proto file skip list works by checking whether the Artifact of any srcs files is equal to the artifact of any skip-listed file. In other words, it checks whether they have the same PathFragment [1].

In the case of the protobuf repo, the excluded .protos are (e.g.) external/com_google_protobuf/src/google/protobuf/any.proto, which is not the same as what we compare them to (src/google/protobuf/any.proto).

The real fix for this issue is to skip-list based on proto import path and not on artifact path, which would also allow us to get rid of the horrible difference between "proto source" and "original proto source". Everything in Bazel is ready for this, can't tell if Blaze is ready for that switch.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java;l=121;drc=89e2f875842eaf8abc5e831e0741d41a8cd83823

[Wyverald]

Thanks for the context, Yannic!

In the case of the protobuf repo, the excluded .protos are (e.g.) external/com_google_protobuf/src/google/protobuf/any.proto,

I was a bit confused how this happens, so I went and did a bit of digging. Apparently, we implicitly insert a local_repository(name="$WORKSPACE_NAME", path=".") clause into the WORKSPACE file (code). This effectively means that there are two distinct packages @com_google_protobuf//:__pkg__ and @//:__pkg_, and both are loaded through the same BUILD file.

I believe this is terribly wrong and the actual source of this bug (probably what you referred to as "a" in the original comment). What makes this weirder is that the package @com_google_protobuf//:__pkg__ is not normally accessible. If you wrote in a BUILD file some_rule(name="X",dep="@com_google_protobuf//:some_target"), that "dep" will go through repo mapping and get resolved to @//:some_target. It's only through corner cases where repo mapping is skipped, such as the built-in default flag value for --proto_toolchain_for_cc, that the @com_google_protobuf//:__pkg__ package can be named at all. (Before 630be02, target patterns on the command line also skipped repo mapping, which is why the old trick worked.)

This can probably be fixed with the "delayed label" thing that @meteorcloudy and I chatted about, which basically tries to make labels in flag defaults also go through repo mapping. Alas, that's unlikely to land in the near future.

In the meantime, @Yannic, your idea to "compare proto import path instead of artifact path" could also mitigate this specific case, though I have no idea how ready Blaze is.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[github-actions]

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!