protocolbuffers
bazel: Remove hardcoded dependency on `//:protoc` from language runtimes
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
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.
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.
If I understand correctly protobuf team would like to eventually remove --protocol_compiler flag and also isn't completely decided to go in the direction of --incompatible_enable_toolchain_resolution. That's fine.
The PR is in my opinion an improvement, because it improves the consistency of protobuf code (protoc is always obtained in the same way), even if only of the legacy and to be removed code. @mkruskal would you mind accepting it as such? The (Google's) standard for code reviews is that the PR is an improvement of the code.
It doesn't get us much closer to precompiled protoc binaries, but it also doesn't hurt us, because the legacy code may still be removed as well as --protocol_compiler and --incompatible_enable_toolchain_resolution flags.
As another data point, I made a patch from this pull request for bazel-contrib/rules_scala. I created it while working towards the upcoming Bzlmod-enabled rules_scala release (bazel-contrib/rules_scala#1482), when our Windows builds failed after upgrading past protobuf v21.7 in bazel-contrib/rules_scala#1710 .
The breakage was due to later protobuf versions pushing some file paths past the cursed 260 character limit, breaking Windows MSVC source builds. Since protobuf support for MSVC is officially dead for precisely this reason (references below), this patch became essential to fixing our Windows builds via --incompatible_enable_proto_toolchain_resolution.
- https://github.com/protocolbuffers/protobuf/issues/12947
- protobuf.dev News Announcements for Version 30.x:Poison MSVC + Bazel
- https://github.com/protocolbuffers/protobuf/issues/20085
The precompiled protoc toolchain mechanism is only mandatory for Windows builds, but also shaves minutes off the build for Linux and macOS. Though we'll have to maintain the patch as long as current protobuf releases are in widespread use, it would be nice to not have to require it for future releases.
Sorry for the delay in responding here. While we do understand that this is a strict improvement to the current situation, it only further endorses a deprecated set of Bazel features we want to turn down completely. We would prefer to focus on providing a future-facing solution to the root problem, and are working towards providing a supported mechanism to use prebuilt binaries with the protobuf rules.
Note: we have no intention of killing MSVC+Bazel support before this solution is in place. There is currently an opt-out flag to allow building from source with MSVC that will be in place until at least 34.0. Setting --output_user_root=C:/ should get you past the 255 character limit issue, as we've have CI to prevent further accidental breakages for a while now
@mkruskal-google Could you elaborate on what these deprecated Bazel features are? As far as I can tell, proto toolchainization is a much more robust and modern approach than --proto_compiler (which should definitely just go away).
Please also keep in mind that prebuilt binaries are not the end of the story. You won't be able to supply prebuilt binaries for all OS/architecture pairs, but users on such platforms can't be limited to from source builds.
--incompatible_enable_proto_toolchain_resolution and --proto_compiler are the deprecated features. The first one is marked "incompatible" today and we do not intend to promote it.
We're still in design discussions atm (with aspect.build), and generic toolchains vs just prebuilts is a key piece that hasn't been decided yet. You make a good point about arbitrary OS/arch pairs, but OTOH the problem with toolchains is just that there's so many ways to shoot yourself in the foot because of our cross-version compatibility constraints. If we did go that way there would need to be a lot of design consideration into ways to make that safer (or at least guide users away from bad patterns)
Note: we have no intention of killing MSVC+Bazel support before this solution is in place. There is currently an opt-out flag to allow building from source with MSVC that will be in place until at least 34.0. Setting
--output_user_root=C:/should get you past the 255 character limit issue, as we've have CI to prevent further accidental breakages for a while now
rules_scala uses Bazel's Buildkite CI system, bazelbuild/continuous-integration. https://github.com/bazel-contrib/rules_scala/pull/1710#issuecomment-2691560698 shows the Windows MSVC build breaking on protobuf v29.3:
ERROR: C:/tools/msys64/home/b/_bazel_b/xknd5zlq/external/com_google_protobuf/src/google/protobuf/compiler/java/BUILD.bazel:87:11:
Compiling src/google/protobuf/compiler/java/java_features.pb.cc [for tool] failed: (Exit 2): cl.exe failed:
error executing CppCompile command
(from target @@com_google_protobuf//src/google/protobuf/compiler/java:java_features_bootstrap)
C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\cl.exe ...
(remaining 1 argument skipped)
--
| external/com_google_protobuf/src/google/protobuf/compiler/java/java_features.pb.cc(6): fatal error C1083:
Cannot open include file: 'google/protobuf/compiler/java/java_features.pb.h': No such file or directory
The full build results show bazel --output_user_root=C:/b. Perhaps we could try setting it to C:/ to see if that one character makes a difference. Trying to run under cmd.exe instead of MSYS2 may also help (since C:/tools/msys64/home/b/... appears in the ERROR: message), and be good for other reasons. That might take some work, since the bulk of rules_scala tests are Bash scripts invoking bazel in various ways. But both potential experiments are effectively moot, at least for the moment. Right now we're using the precompiled protoc to make progress, and it's making both CI and local development faster.
FWIW, I'm totally on board with replacing the old and busted with the new hotness. (This explains my commitment to completing bazel-contrib/rules_scala#1482.) So if there's a better solution around the corner, I'll be amongst the first to adopt it. But until then, or until this pull request lands, we have to keep patching protobuf to unblock Windows builds of rules_scala. We'll also keep patching it for the overall benefit of avoiding protoc recompiles on all platforms.
O yea msys2 is definitely the problem there. You're getting extra characters from tools/msys64/home, which is going to push you over the limit for sure. We're very close to the limit and I think C:\tmp is the extent of what Bazel+MSVC can handle today (it's what our CI covers).
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.
This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mkruskal-google I think we've confirmed that the toolchain flag isn't being removed in the near future, could we consider merging this one? Will be needed to get pre-built protoc working as expected.
