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

bazel: Remove hardcoded dependency on //:protoc from language runtimes #19679

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fmeum
Copy link

@fmeum fmeum commented Dec 16, 2024

Without this change, language runtimes still result in a build of //:protoc even with a prebuilt proto_toolchain registered or --proto_compiler set to a precompiled protoc. Removing this hardcoded dependency allows a (fast) build of java_proto_library targets without a C++ toolchain assuming a prebuilt protoc.

Work towards #19558

Without this change, language runtimes still result in a build of `//:protoc` even with a prebuilt `proto_toolchain` registered or `--proto_compiler` set to a precompiled protoc.
@fmeum fmeum marked this pull request as ready for review December 16, 2024 15:25
@fmeum
Copy link
Author

fmeum commented Dec 16, 2024

CC @comius @alexeagle

@fmeum
Copy link
Author

fmeum commented Dec 16, 2024

@shaod2 Would you be available to review this?

@fmeum fmeum force-pushed the 19558-dont-hardcode-protoc branch from d92eae2 to 3bd939d Compare December 16, 2024 21:20
@zhangskz zhangskz added the bazel label Dec 18, 2024
@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 18, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 18, 2024
@mkruskal-google
Copy link
Member

Hey Fabian,

So IIUC, this PR is trying to make the protoc binary used by proto_library customizable. While that might be a possible long-term solution, it opens the door to a lot of potential misuse. Notably, mixing alternate implementations or different versions of protoc is typically unsupported and can have unexpected results. We have poison pills to avoid some of these situations outside of Bazel, but they haven't been added to all our languages yet. A more conservative change would be to just have some kind of flag that toggles between build-from-source and the prebuilt releases we publish to github, without allowing arbitrary binaries to be injected.

Additionally, this PR is touching protobuf.bzl, which just contains our legacy internal macros/rules (which we want to delete). These have been replaced by the rules in //bazel, which were recently moved from other Bazel repos.

@fmeum
Copy link
Author

fmeum commented Dec 30, 2024

Additionally, this PR is touching protobuf.bzl, which just contains our legacy internal macros/rules (which we want to delete). These have been replaced by the rules in //bazel, which were recently moved from other Bazel repos.

These macros are still used to bootstrap the Java well-known protos for the default Java proto_lang_toolchain, which is why protoc ends up being compiled from source even when using a precompiled protoc binary.

So IIUC, this PR is trying to make the protoc binary used by proto_library customizable. While that might be a possible long-term solution, it opens the door to a lot of potential misuse. Notably, mixing alternate implementations or different versions of protoc is typically unsupported and can have unexpected results.

Isn't the protoc binary used by proto_library already configurable via --proto_compiler or toolchain registration (the latter with --incompatible_enable_proto_toolchain_resolution)? The aim of this PR is specifically to ensure that if users use custom protoc binary, it's the only protoc used in the entire build. Right now, when configuring a non-default protoc binary in any of the existing ways, the //:protoc from this repo still ends up being used to bootstrap protos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants