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

Fix export of @bazel_tools//src/main/protobuf #11611

Closed
wants to merge 1 commit into from
Closed

Fix export of @bazel_tools//src/main/protobuf #11611

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2020

src/main/protobuf/BUILD referenced other parts of the Bazel source tree
which aren't available when repackaged into the embedded tools distribution.
This commit adds a parallel BUILD.tools which is stripped of any such
references.

Resolves #8738.

Testing Done:

`src/main/protobuf/BUILD` referenced other parts of the Bazel source tree
which aren't available when repackaged into the embedded tools distribution.
This commit adds a parallel `BUILD.tools` which is stripped of any such
references.

Resolves #8738.

Testing Done:
- Referred to `@bazel_tools//src/main/protobuf:worker_protocol.proto` in
  an internal rule w/o encountering #8738.
@ghost
Copy link
Author

ghost commented Jun 18, 2020

Not sure what the scope of any test case(s) should be. My best guess would be something similar to //src/test/shell/bazel:embedded_tools_deps_test.

@aiuto aiuto requested review from meisterT and philwo June 22, 2020 14:30
@aiuto
Copy link
Contributor

aiuto commented Jun 22, 2020

I assigned reviewers based on who had the TODO and who the blame layer points to.

simuons added a commit to simuons/rules_scala that referenced this pull request Aug 25, 2020
Replace local copy of `worker_protocol.proto` with
`@bazel_tools//src/main/protobuf:worker_protocol_java_proto`

Local copy requires keeping track of changes in worker protocol (though
it should be pretty stable). To be honest I'm not sure
`@bazel_tools//src/main/protobuf:worker_protocol_java_proto` is part of
public interface (see  bazelbuild/bazel#11611).
@philwo
Copy link
Member

philwo commented Oct 21, 2020

@meisterT Friendly ping :P

@philwo
Copy link
Member

philwo commented Oct 21, 2020

@beasleyr-vmw Could you just copy that protocol buffer file to your repository instead? Protobufs are supposed to be backwards compatible, so even if you don't update your version for some time, it should be fine until you want to use newer fields. It's also easier for tools like Code Search (if you use that) if the protobuf is in the repo, instead of an embedded repo inside the Bazel binary.

I tried to import it, but I'm not sure if the overhead of maintaining yet another parallel BUILD file that might get out of date in our code is worth it.

@ghost
Copy link
Author

ghost commented Oct 22, 2020

@philwo If that's the official position, I'm fine with it. I'm unfamiliar w/ protobuf best practices, but my gut feeling was to stick to canonical locations instead of privately snapshotting the .proto files. LMK and I can withdraw this.

@meisterT
Copy link
Member

@beasleyr-vmw thanks for your understanding - I agree with Philipp that the separate BUILD file is brittle (it's untested) and overhead.

@meisterT meisterT closed this Oct 22, 2020
@ghost ghost deleted the topic/beasleyr-vmw/8738-src_main_protobuf branch October 22, 2020 14:06
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.

Unable to reference @bazel_tools//src/main/protobuf targets
5 participants