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

Add current_proto_toolchain #214

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

Conversation

psalaberria002
Copy link

@psalaberria002 psalaberria002 commented May 21, 2024

After asking in https://bazelbuild.slack.com/archives/C04281DTLH0/p1716279966809639 I gave it a tried, and it seems to be working.

The implementation is based on https://github.com/bazelbuild/rules_python/blob/730a2e39bd2702910f28629d4583b3ec49f4ee5e/python/current_py_toolchain.bzl

Tested with the following snippet:

load("@rules_proto//proto:defs.bzl", "current_proto_toolchain")

current_proto_toolchain(
    name = "current_proto_toolchain",
    visibility = ["//visibility:public"],
)

genrule(
    name = "x",
    srcs = [":current_proto_toolchain"],
    outs = ["x"],
    cmd = "$(PROTOC) > $(OUTS)",
    toolchains = [":current_proto_toolchain"],
)

How should we test this feature?

Is there any other preferred alternative for exposing the current protoc binary after toolchain resolution?

@psalaberria002 psalaberria002 requested review from comius and a team as code owners May 21, 2024 14:03
@psalaberria002 psalaberria002 marked this pull request as draft May 21, 2024 14:03
thesayyn
thesayyn previously approved these changes May 21, 2024
Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe a genrule test for it so we know it works?

@psalaberria002
Copy link
Author

LGTM, maybe a genrule test for it so we know it works?

I get While resolving toolchains for target //tests/current_toolchain:current_proto_toolchain (312a038): No matching toolchains found for types //proto:toolchain_type. because there aren't any registered toolchains in this module.

Is it possible to register toolchains only for running tests, but not when publishing the module?
Does that require a totally separate folder with its own MODULE.bazel?

@thesayyn
Copy link
Collaborator

i think so yes, https://bazel.build/rules/lib/globals/module#register_toolchains has an attribute dev_dependency.

@thesayyn
Copy link
Collaborator

we already depend on protobuf repo which has a protoc binary which can be registered as a toolchain only as a dev dep.

bazel_dep(name = "protobuf", version = "23.1", dev_dependency = True, repo_name = "com_google_protobuf")

BUILD Show resolved Hide resolved
@psalaberria002 psalaberria002 marked this pull request as ready for review May 21, 2024 19:31
@comius
Copy link
Collaborator

comius commented May 29, 2024

current toolchain pattern is something we're deprecating in other toolchains. What is the reason to introduce it for protos?

@psalaberria002
Copy link
Author

current toolchain pattern is something we're deprecating in other toolchains. What is the reason to introduce it for protos?

It was recommended in the Slack workspace.

Whats the alternative for accessing the protoc binary when using the toolchain?
I have some rust code that expects protoc to be available as the PROTOC environment variable.

@thesayyn
Copy link
Collaborator

toolchain pattern is something we're deprecating in other toolchains.

Hmm, what is it being replaced with? Currently, in order to use a toolchain in a genrule, you need a target to materialize a toolchain type and return some providers according to this issue.

This target is also important to workaround issues such as bazelbuild/bazel#19645

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

Successfully merging this pull request may close these issues.

3 participants