bazelbuild / rules_go

Go rules for Bazel
Apache License 2.0
1.36k stars 635 forks source link

Add support for `--incompatible_enable_proto_toolchain_resolution` #3919

Closed fmeum closed 1 month ago

fmeum commented 2 months ago

What type of PR is this?

Feature

What does this PR do? Why is it needed?

With --incompatible_enable_proto_toolchain_resolution, users of protoc are expected to obtain it through @rules_proto//proto:toolchain_type instead of the Bazel command line flag --proto_compiler. This change adds support for this in a backwards compatible way.

Note that the above is actually a lie: The proper way to use protoc would be to go through a Go-specific proto_lang_toolchain and use the methods on proto_common to interact with protoc. Since rules_go has a very bespoke setup with customizable compilers and the need to apply reset transitions in case protoc is built from source, this would require major changes. We should revisit this after Proto toolchainization has been enabled by default.

Which issues(s) does this PR fix?

Fixes #3895

Other notes for review

fmeum commented 2 months ago

@alexeagle None of us have worked with proto toolchains so far, so it would be great if you could also take a look.

fmeum commented 2 months ago

FYI @sluongng You might find this interesting :-)

fmeum commented 2 months ago

I'll take another stab at fixing the integration tests next week. Everything else is stable for review.

fmeum commented 1 month ago

I updated rules_proto to 6.0.0 and fixed the remaining tests, PTAL @linzhp @tyler-french

alexeagle commented 1 month ago

@sluongng FYI looks like this PR will merge as soon as you click "Resolve" on the two open threads; are they answered to your satisfaction?

sluongng commented 1 month ago

Hmm i cannot resolve the conversations. Perhaps @fmeum can.

The PR LGTM. I hope the legacy toolchain still provide an alternative option to use protoc from source.

Cheers!

avdv commented 1 week ago

Hi,

when trying to upgrade to 0.48.0 / 0.48.1 I see this error:

INFO: Repository com_github_golang_protobuf instantiated at:
  /home/runner/work/gazelle_cabal/gazelle_cabal/WORKSPACE:153:22: in <toplevel>
  /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/io_bazel_rules_go/go/private/repositories.bzl:200:12: in go_rules_dependencies
  /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/io_bazel_rules_go/go/private/repositories.bzl:295:18: in _maybe
Repository rule http_archive defined at:
  /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/bazel_tools/tools/build_defs/repo/http.bzl:372:31: in <toplevel>
ERROR: /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/com_github_bazelbuild_buildtools/edit/BUILD.bazel:3:11: error loading package '@com_github_bazelbuild_buildtools//build_proto': at /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/io_bazel_rules_go/proto/def.bzl:38:5: at /home/runner/.cache/bazel/_bazel_runner/048d85559eaccf97cf886d3788a80e96/external/io_bazel_rules_go/proto/compiler.bzl:20:5: cannot load '@rules_proto//proto:proto_common.bzl': no such file and referenced by '@com_github_bazelbuild_buildtools//edit:edit'

The release notes mention that rules_go now requires rules_proto 6.0.0 but that is not included in the workspace dependencies. What's the idea here? Do we need to add rules_proto manually?

fmeum commented 1 week ago

Yes. Since proto dependencies can't be handled for the users without surprising version conflicts, we let you supply them yourself when you are using protos (in this case indirectly through buildtools). You could also consider switching to MODULE.bazel, which automates this once again with proper version resolution.