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.

Sign up for GitHub

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

Jump to bottom

Implement proto toolchainisation #179

Closed
comius opened this issue Sep 15, 2023 · 8 comments
Closed

Implement proto toolchainisation #179

comius opened this issue Sep 15, 2023 · 8 comments
Assignees
@comius @thesayyn
Labels
feature request P2 We'll consider to work on this in future. (Assignee optional)

Comments

@comius
Copy link
Collaborator

comius commented Sep 15, 2023

Design document: https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit#heading=h.5mcn15i0e1ch
@comius comius added P2 We'll consider to work on this in future. (Assignee optional) feature request labels Sep 15, 2023
@comius comius self-assigned this Sep 15, 2023
@alexeagle
Copy link
Collaborator

@comius can you assign to @thesayyn - we are going to start on this pretty soon.

copybara-service bot pushed a commit that referenced this issue Sep 18, 2023
Create toolchain types for proto_library and lang_proto_library
7537ac6 
This handles only lang_proto_libraries defined in Bazel: Java, Java lite, Python, C++.

Design doc: https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit#heading=h.5mcn15i0e1ch
Issue: #179
PiperOrigin-RevId: 566380737
copybara-service bot pushed a commit to bazelbuild/rules_java that referenced this issue Sep 18, 2023
@comius @copybara-github
Create toolchain types for proto_library and lang_proto_library
9d9be9d 
BEGIN_PUBLIC
Create toolchain types for proto_library and lang_proto_library

This handles only lang_proto_libraries defined in Bazel: Java, Java lite, Python, C++.

Design doc: https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit#heading=h.5mcn15i0e1ch
Issue: bazelbuild/rules_proto#179
END_PUBLIC
PiperOrigin-RevId: 566380737
Change-Id: I71dd30ed45c0f1d727600989ed1e50e9e5d70411
copybara-service bot pushed a commit to bazelbuild/rules_cc that referenced this issue Sep 18, 2023
@comius @copybara-github
Create toolchain types for proto_library and lang_proto_library
eef16ef 
BEGIN_PUBLIC
Create toolchain types for proto_library and lang_proto_library

This handles only lang_proto_libraries defined in Bazel: Java, Java lite, Python, C++.

Design doc: https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit#heading=h.5mcn15i0e1ch
Issue: bazelbuild/rules_proto#179
END_PUBLIC
PiperOrigin-RevId: 566380737
Change-Id: I49b132f861bc7a871d98bbd333271d540faaf737
@comius
Copy link
Collaborator Author

comius commented Sep 18, 2023

cc @fowles, @haberman

I've sent invite to @thesayyn, @alexeagle.

There's quite some stuff that needs to be coordinated internally, I'll start pushing out changes.

copybara-service bot pushed a commit that referenced this issue Sep 18, 2023
Create proto_toolchain rule and macro
bb745f9 
The proto_toolchain rule serves to define what protoc is used and to generate descriptor sets by proto_library.

This is a stripped down version of proto_lang_toolchain. It's not possible to set plugins or runtime libraries on it.

Steps in migration:
- this and other changes to proto_lang_toolchain
- release rules_proto, rules_java, rules_cc, rules_python
- use new version and define toolchains in protobuf repo

Issue: #179
PiperOrigin-RevId: 566390506
@alexeagle
Copy link
Collaborator

Hm, I don't see any invites from GitHub...

copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Sep 20, 2023
@comius @copybara-github
Remove protoc from proto_lang_toolchain rule
f5fb2f6 
The protoc should be provided with a new proto_toolchain rule.

Issue: bazelbuild/rules_proto#179

RELNOTES[INC]: proto_compiler attribute removed from proto_lang_toolchain
(it was recently introduced, and there is no evidence of use)

PiperOrigin-RevId: 566937038
Change-Id: I7133be4d5cbcc816764c0ba35607329ea50d2406
copybara-service bot pushed a commit that referenced this issue Sep 20, 2023
Wrap proto_lang_toolchain into a macro creating a toolchain
673e59e 
Issue: #179
PiperOrigin-RevId: 566975629
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Sep 21, 2023
@comius @copybara-github
Use proto_toolchain in proto_library
d435c6d 
Change --incompatible_enable_proto_toolchain_resolution flag to a loading time flag. This is partial revert of 11cf1b7. We need a load time flag, because toolchain type might not exist at current versions of rules repos users use.

When the flag is set, proto_library defines the toolchain and doesn't use an implicit dependency at the same time.

In Bazel the flag may only be flipped after all the rules_* repo are upgraded to define toolchain type and protobuf repository registers the toolchains.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 567296665
Change-Id: Ib3fc27751dd14589fa403f45d2cbbad22537b9a3
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Sep 22, 2023
@comius @copybara-github
Use proto compiler from proto_toolchain rule
42800a8 
Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 567570286
Change-Id: Ia7e1ff08a0123a325f1df00f56b81212c01e2588
@alexeagle alexeagle mentioned this issue Sep 26, 2023
Closed
@alexeagle
Copy link
Collaborator

Note that #96 should be solved here as well.

@comius comius mentioned this issue Sep 29, 2023
Closed
@comius
Copy link
Collaborator Author

comius commented Sep 29, 2023

Hey, @alexeagle and @thesayyn, is there any progress on this? Any problems/issues/blockers?

I'm still waiting for reviews internally. The reviewer is out till Wednesday.

copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 2, 2023
@comius @copybara-github
Refactor proto toolchainsation support utilities
3b18d3f 
Implement reusable utils as part of proto_common and move out of semantics. Define toolchains that will be needed for lang_proto_libraries.

Because there are so many toolchains this will make the future code more readable.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570019432
Change-Id: Ie1675f1d847872bab3a250270eaa451ef4816ed8
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 2, 2023
@comius @copybara-github
Decouple java_lite_proto_library from java_proto_library
20bc11f 
Remove call to java_proto_library_impl from java_proto_library.

Remove hasattr from java_proto_library_impl, which handled java_lite_proto_library.

Code reuse in this case made little sense, and it incurrent more maintenance cost than benefit.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570053518
Change-Id: Ie43cadfd1fc324b6aaad08acaca8eea7df1a3669
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 2, 2023
@comius @copybara-github
Use proto toolchains in java_proto_library
51970d2 
Retrieve proto_lang_toolchain using toolchains in java_proto_library when proto toolchain resolution is enabled.
Add mocks for proto_lang_toolchain from rules_proto.
Return ToolchainInfo from proto_lang_toolchain rule. That's needed in the resolution. Also returning ProtoLangToolchainInfo directly, to support legacy mechanism.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570055332
Change-Id: Ieb0510d48778900b8576a71ddb20abfdcda220be
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 2, 2023
@comius @copybara-github
Use proto toolchains in java_lite_proto_library
8c38be3 
Retrieve proto_lang_toolchain using toolchains in java_lite_proto_library when proto toolchain resolution is enabled.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570056144
Change-Id: Ia952003017d18baca71d4e0ba75fc3134b972817
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 3, 2023
@comius @copybara-github
Use proto toolchains in cc_proto_library
3e1e061 
Retrieve proto_lang_toolchain using toolchains in cc_proto_library when proto toolchain resolution is enabled.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570334276
Change-Id: If7b7e25254bf3036fdcad67d36474b8fe64a8ffd
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 9, 2023
@comius @copybara-github
Support automatic exec groups in proto_common.compile
5a24c8a 
Pass toolchain_type through ProtoLangToolchainInfo into proto_common.compile and use it on ctx.actions.run. Automatic exec groups require that toolchain type is set on the `ctx.actions.run`. This information is used to select correct execution platform.

Expose INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION in proto_common. This will be needed to support lang_proto_libraries that are not part of Bazel. For example py_proto_library. Other methods in `toolchains` struct in proto_common.bzl, are both temporary and can be written in Starlark, so don't expose them. It's possible to access the value in backward compatible manner (that is with `getattr(proto_common, ...)`).

Expose INCOMPATIBLE_PASS_TOOLCHAIN_TYPE in proto_common. Second "flag" is here to mark, that builtin `proto_lang_toolchain` rule has a `toolchain_type` attribute. This way `proto_lang_toolchain` macro can pass the value in a compatible fashion with older Bazel. This should make toolchainisation work with older versions of Bazel that don't know about automatic exec groups and don't need to pass in the value.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 571876657
Change-Id: I543ab862b318c9062d40430160e33ad197973094
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Remove protoc from proto_lang_toolchain rule
e95f116 
The protoc should be provided with a new proto_toolchain rule.

Issue: bazelbuild/rules_proto#179

RELNOTES[INC]: proto_compiler attribute removed from proto_lang_toolchain
(it was recently introduced, and there is no evidence of use)

PiperOrigin-RevId: 566937038
Change-Id: I7133be4d5cbcc816764c0ba35607329ea50d2406
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Use proto_toolchain in proto_library
d114f24 
Change --incompatible_enable_proto_toolchain_resolution flag to a loading time flag. This is partial revert of bazelbuild@11cf1b7. We need a load time flag, because toolchain type might not exist at current versions of rules repos users use.

When the flag is set, proto_library defines the toolchain and doesn't use an implicit dependency at the same time.

In Bazel the flag may only be flipped after all the rules_* repo are upgraded to define toolchain type and protobuf repository registers the toolchains.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 567296665
Change-Id: Ib3fc27751dd14589fa403f45d2cbbad22537b9a3
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Use proto compiler from proto_toolchain rule
36394f9 
Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 567570286
Change-Id: Ia7e1ff08a0123a325f1df00f56b81212c01e2588
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Refactor proto toolchainsation support utilities
0895719 
Implement reusable utils as part of proto_common and move out of semantics. Define toolchains that will be needed for lang_proto_libraries.

Because there are so many toolchains this will make the future code more readable.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570019432
Change-Id: Ie1675f1d847872bab3a250270eaa451ef4816ed8
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Decouple java_lite_proto_library from java_proto_library
8ea74cd 
Remove call to java_proto_library_impl from java_proto_library.

Remove hasattr from java_proto_library_impl, which handled java_lite_proto_library.

Code reuse in this case made little sense, and it incurrent more maintenance cost than benefit.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570053518
Change-Id: Ie43cadfd1fc324b6aaad08acaca8eea7df1a3669
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Use proto toolchains in java_proto_library
514b3e4 
Retrieve proto_lang_toolchain using toolchains in java_proto_library when proto toolchain resolution is enabled.
Add mocks for proto_lang_toolchain from rules_proto.
Return ToolchainInfo from proto_lang_toolchain rule. That's needed in the resolution. Also returning ProtoLangToolchainInfo directly, to support legacy mechanism.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570055332
Change-Id: Ieb0510d48778900b8576a71ddb20abfdcda220be
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Use proto toolchains in java_lite_proto_library
9524dac 
Retrieve proto_lang_toolchain using toolchains in java_lite_proto_library when proto toolchain resolution is enabled.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570056144
Change-Id: Ia952003017d18baca71d4e0ba75fc3134b972817
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Use proto toolchains in cc_proto_library
f30e59a 
Retrieve proto_lang_toolchain using toolchains in cc_proto_library when proto toolchain resolution is enabled.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 570334276
Change-Id: If7b7e25254bf3036fdcad67d36474b8fe64a8ffd
comius added a commit to comius/bazel that referenced this issue Jan 17, 2024
@comius
Support automatic exec groups in proto_common.compile
29b1f95 
Pass toolchain_type through ProtoLangToolchainInfo into proto_common.compile and use it on ctx.actions.run. Automatic exec groups require that toolchain type is set on the `ctx.actions.run`. This information is used to select correct execution platform.

Expose INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION in proto_common. This will be needed to support lang_proto_libraries that are not part of Bazel. For example py_proto_library. Other methods in `toolchains` struct in proto_common.bzl, are both temporary and can be written in Starlark, so don't expose them. It's possible to access the value in backward compatible manner (that is with `getattr(proto_common, ...)`).

Expose INCOMPATIBLE_PASS_TOOLCHAIN_TYPE in proto_common. Second "flag" is here to mark, that builtin `proto_lang_toolchain` rule has a `toolchain_type` attribute. This way `proto_lang_toolchain` macro can pass the value in a compatible fashion with older Bazel. This should make toolchainisation work with older versions of Bazel that don't know about automatic exec groups and don't need to pass in the value.

Issue: bazelbuild/rules_proto#179
PiperOrigin-RevId: 571876657
Change-Id: I543ab862b318c9062d40430160e33ad197973094
@alexeagle
Copy link
Collaborator

alexeagle commented Jan 30, 2024 •
edited
Loading

This bit me again because rules_ts accidentally needs a C++ toolchain just for protoc.

I'd like to send a PR to add a "pre-built binary toolchain" for protoc here.

The approach will be:

@thesayyn thesayyn mentioned this issue Feb 2, 2024
Closed
@alexeagle
Copy link
Collaborator

FYI @comius I'm implementing that proposal now. From interaction with the protobuf repo around publishing to BCR, I think it's clear that maintainers over there shouldn't be tasked with publishing a Bazel module.

@alexeagle
Copy link
Collaborator

I believe this is now complete, and the 6.0.0-rc3 release can be used to demonstrate that it works.
https://registry.bazel.build/modules/toolchains_protoc is an "existence proof" that a rules_proto user can now enable --incompatible_enable_proto_toolchain_resolution and then use a pre-built protoc binary, along with lang_proto_library rules and language runtimes.

hanwen pushed a commit to GerritCodeReview/gerrit that referenced this issue Jun 3, 2024
@davido
Bazel: Don't build protobuf from the source
231e698 
Gerrit is pure java project. Given that it depends on Google protobuf,
and given that Bazel was using @com_google_protobuf toolchain that was
built from source, this project ended up bulding the Google protobuf
from source.

Compiling protoc from source requires a functional C++ toolchain,
which is a burden for projects that have no C++ code. Also, Bazel
does not ship with a hermetic toolchain, so that it is possible
that for many Gerrit developers and contributors the Bazel build
is inherently broken.

In addition, building protobuf from source made OS upgrades difficult
because of the incompatibilities between the source code and the latest XCode versions.

That changed in Bazel 7.x release, with the new and shiny option:
--incompatible_enable_proto_toolchain_resolution, that allow to register
prebuilt protoc toolchains.

In addition rules_proto have added support for prebuilt toolchains in
context of this tracking issue: [1].

In this change we use toolchains_protoc project to consume predefined
protobuf toolchains: [2].

As the side effect of this change we have to consume protobuf-java
ourself and not transitively through standard @com_google_protobuf
toolchain. Given that Google protobuf is internal Google project and
all released versions already available within Google, we add the new
dependency to tools/nongoogle.bzl to exempt the updates for it from the
Library-Compliance label.

Now, that we stop building protobuf from source, we can remove C++
options in .bazelrc as well.

[1] bazelbuild/rules_proto#179
[2] https://github.com/aspect-build/toolchains_protoc

Release-Notes: Use prebuilt protobuf toolchain to avoid building protoc from source
Change-Id: I27975879819c4b632682990474ce88737f722d9a
mbland added a commit to mbland/rules_scala that referenced this issue Oct 4, 2024
@mbland
Begin toolchainized protoc implementation
3582369 
I solved the rest of the build failures from bazelbuild#1618. Now multiple jobs
are failing due to hanging scalac worker jobs as described in the PR
description.

- Proto Toolchainisation Design Doc
  https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit

- bazelbuild/bazel: Protobuf repo recompilation sensitivity
  bazelbuild/bazel#7095

- bazelbuild/rules_proto: Implement proto toolchainisation
  bazelbuild/rules_proto#179

- rules_proto 6.0.0 release notes mentioning Protobuf Toolchainisation
  https://github.com/bazelbuild/rules_proto/releases/tag/6.0.0

It occurred to me to try adopting Proto Toolchainisation to see if that
might resolve the issue. I've got it successfully generating the
`@io_bazel_rules_scala_protoc` repo and registering toolchains for
`@rules_proto//proto:toolchain_type`. However, it's currently dying
because there's no toolchain registered for
'@rules_java//java/proto:toolchain_type'.

```txt
bazel build //src/...
ERROR: .../src/protobuf/io/bazel/rules_scala/BUILD:4:14:
  in @@_builtins//:common/java/proto/java_proto_library.bzl%bazel_java_proto_aspect
  aspect on proto_library rule
  //src/protobuf/io/bazel/rules_scala:diagnostics_proto:

Traceback (most recent call last):
  File "/virtual_builtins_bzl/common/java/proto/java_proto_library.bzl",
    line 53, column 53, in _bazel_java_proto_aspect_impl
  File "/virtual_builtins_bzl/common/proto/proto_common.bzl",
    line 364, column 17, in _find_toolchain
Error in fail: No toolchains registered for '@rules_java//java/proto:toolchain_type'.
ERROR: Analysis of target '//src/protobuf/io/bazel/rules_scala:diagnostics_proto' failed
ERROR: Analysis of target '//src/java/io/bazel/rulesscala/scalac:scalac' failed; build aborted: Analysis failed
```
mbland added a commit to mbland/rules_scala that referenced this issue Oct 4, 2024
@mbland
Begin toolchainized protoc implementation
d39d35c 
I solved the rest of the build failures from bazelbuild#1618. Now multiple jobs
are failing due to hanging scalac worker jobs as described in the PR
description.

- Proto Toolchainisation Design Doc
  https://docs.google.com/document/d/1CE6wJHNfKbUPBr7-mmk_0Yo3a4TaqcTPE0OWNuQkhPs/edit

- bazelbuild/bazel: Protobuf repo recompilation sensitivity
  bazelbuild/bazel#7095

- bazelbuild/rules_proto: Implement proto toolchainisation
  bazelbuild/rules_proto#179

- rules_proto 6.0.0 release notes mentioning Protobuf Toolchainisation
  https://github.com/bazelbuild/rules_proto/releases/tag/6.0.0

It occurred to me to try adopting Proto Toolchainisation to see if that
might resolve the issue. I've got it successfully generating the
`@io_bazel_rules_scala_protoc` repo and registering toolchains for
`@rules_proto//proto:toolchain_type`. However, it's currently dying
because there's no toolchain registered for
'@rules_java//java/proto:toolchain_type'.

```txt
bazel build //src/...
ERROR: .../src/protobuf/io/bazel/rules_scala/BUILD:4:14:
  in @@_builtins//:common/java/proto/java_proto_library.bzl%bazel_java_proto_aspect
  aspect on proto_library rule
  //src/protobuf/io/bazel/rules_scala:diagnostics_proto:

Traceback (most recent call last):
  File "/virtual_builtins_bzl/common/java/proto/java_proto_library.bzl",
    line 53, column 53, in _bazel_java_proto_aspect_impl
  File "/virtual_builtins_bzl/common/proto/proto_common.bzl",
    line 364, column 17, in _find_toolchain
Error in fail: No toolchains registered for '@rules_java//java/proto:toolchain_type'.
ERROR: Analysis of target '//src/protobuf/io/bazel/rules_scala:diagnostics_proto' failed
ERROR: Analysis of target '//src/java/io/bazel/rulesscala/scalac:scalac' failed; build aborted: Analysis failed
```
EdwinKempin pushed a commit to GerritCodeReview/gerrit that referenced this issue Nov 11, 2024
@davido @lucamilanesio
Bazel: Don't build protobuf from the source
686a78b 
Gerrit is pure java project. Given that it depends on Google protobuf,
and given that Bazel was using @com_google_protobuf toolchain that was
built from source, this project ended up bulding the Google protobuf
from source.

Compiling protoc from source requires a functional C++ toolchain,
which is a burden for projects that have no C++ code. Also, Bazel
does not ship with a hermetic toolchain, so that it is possible
that for many Gerrit developers and contributors the Bazel build
is inherently broken.

In addition, building protobuf from source made OS upgrades difficult
because of the incompatibilities between the source code and the latest XCode versions.

That changed in Bazel 7.x release, with the new and shiny option:
--incompatible_enable_proto_toolchain_resolution, that allow to register
prebuilt protoc toolchains.

In addition rules_proto have added support for prebuilt toolchains in
context of this tracking issue: [1].

In this change we use toolchains_protoc project to consume predefined
protobuf toolchains: [2].

As the side effect of this change we have to consume protobuf-java
ourself and not transitively through standard @com_google_protobuf
toolchain. Given that Google protobuf is internal Google project and
all released versions already available within Google, we add the new
dependency to tools/nongoogle.bzl to exempt the updates for it from the
Library-Compliance label.

Now, that we stop building protobuf from source, we can remove C++
options in .bazelrc as well.

[1] bazelbuild/rules_proto#179
[2] https://github.com/aspect-build/toolchains_protoc

Release-Notes: Use prebuilt protobuf toolchain to avoid building protoc from source
Change-Id: I27975879819c4b632682990474ce88737f722d9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@comius comius

@thesayyn thesayyn

Labels
feature request P2 We'll consider to work on this in future. (Assignee optional)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@alexeagle @comius @thesayyn