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

Expose --proto_compiler to Starlark rules #8485

Open
robbertvanginkel opened this issue May 28, 2019 · 8 comments
Open

Expose --proto_compiler to Starlark rules #8485

robbertvanginkel opened this issue May 28, 2019 · 8 comments
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Server Issues for serverside rules included with Bazel type: feature request

Comments

@robbertvanginkel
Copy link
Contributor

Description of the problem / feature request:

Expose value of --proto_compiler to starlark.

Feature requests: what underlying problem are you trying to solve with this feature?

I'm trying to use a different protoc binary other than @com_google_protobuf//:protoc through the --proto_compiler bazel flag. This mostly works, except for in my go rules that depend on a separately defined default I cannot modify in

https://github.com/bazelbuild/rules_go/blob/f900a9e520e9fdb7b81511f9aa5de2e43da19e2a/proto/compiler.bzl#L174-L178

The rules_go maintainers would be open to reading the --proto_compiler setting if it is exposed through Starlark. Is there a way to do this today or an idea of exposing a proto toolchain to starlark rules?

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 0.25.2

Have you found anything relevant by searching the web?

rules_go issue: bazelbuild/rules_go#2018 (comment)

Any other information, logs, or outputs that you want to share?

@laurentlb laurentlb added team-Rules-Server Issues for serverside rules included with Bazel and removed team-Starlark labels May 31, 2019
@hlopko
Copy link
Member

hlopko commented Jun 7, 2019

Proto toolchain is something that we will eventually do. Pretty much every user of protobuf shares your concern that recompiling protobuf all the time can slow down the build. We are in the middle of many big migrations that all block this effort (Starlark configuration options, and toolchains and platforms most notably)

But I find that in practice the issue is not so bad - especially if you use --disk_cache and/or remote cache, you will end up getting cache hits most of the time, while still getting the benefit that protoc will be recompiled when it actually changed (so you will not get protoc sources and protoc checked in binary out of sync).

If this is still not good enough workaround for the meantime, you can get away with some boilerplate. Say that --proto_compiler will be the source of truth, then you'll create a config_setting for that flag and select on that on your go_proto_compiler target (in a sense if //foo:protoc then use //foo:protoc). Does it make sense?

@hlopko hlopko added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request and removed untriaged labels Jun 7, 2019
@robbertvanginkel
Copy link
Contributor Author

Although performance of compiling protoc is one of the reasons I'd like to have a precompiled one, its not the only one. We use bazel in a few repositories with mostly go and proto code, and users now need a c toolchain just to compile protoc. We rely on bazel's autoconfiguration and don't have a hermetic c toolchain by default, this led to issues like protocolbuffers/protobuf#5376 where protoc wouldn't compile if users had installed custom c headers using something like homebrew.

Getting to a starlark defined c toolchain and disk/remote cache would take away most concerns, but these are reasonable complex to add. I was hoping we could use a precompiled protoc as intermediate solution.

Thank you for the config_setting suggestion, I'll see if we can do that and hope a protoc toolchain appears at some point in the future!

@grahamjenson
Copy link
Contributor

@robbertvanginkel Is this still an issue? Have you found a workaround? I would also like to use a precompiled version of protoc for the exact reasons you specify.

@robbertvanginkel
Copy link
Contributor Author

I believe that this is still a generic problem, maybe with moving the proto rules out to https://github.com/bazelbuild/rules_proto this will be resolved eventually.

As workaround, we apply a small patch on rules_go we do the following:

# WORKSPACE
http_archive(
    name = "io_bazel_rules_go",
    patch_args = ["-p1"],
    patches = ["//patches:rulesgo-protoc.patch"],
    ...
)
# patches/rulesgo-protoc.patch
diff --git a/proto/compiler.bzl b/proto/compiler.bzl
index 14149d61..f06b63a3 100644
--- a/proto/compiler.bzl
+++ b/proto/compiler.bzl
@@ -173,7 +173,7 @@ go_proto_compiler = go_rule(
         "_protoc": attr.label(
             executable = True,
             cfg = "exec",
-            default = "@com_google_protobuf//:protoc",
+            default = "@//rules/proto:protoc",
         ),
     },
 )
# rules/proto/BUILD.bazel
native_binary(
    name = "protoc",
    src = select({
        "//conditions:default": "@com_google_protobuf//:protoc",
        ":prebuilt_protoc_darwin": "@protoc_bin_osx_x86_64//:bin/protoc",
        ":prebuilt_protoc_linux": "@protoc_bin_linux_x86_64//:bin/protoc",
    }),
    out = "protoc",
    visibility = ["//visibility:public"],
)

config_setting(
    name = "prebuilt_protoc_darwin",
    constraint_values = [
        "@bazel_tools//platforms:osx",
        "@bazel_tools//platforms:x86_64",
    ],
    values = {"proto_compiler": "//rules/proto:protoc"},
)

config_setting(
    name = "prebuilt_protoc_linux",
    constraint_values = [
        "@bazel_tools//platforms:linux",
        "@bazel_tools//platforms:x86_64",
    ],
    values = {"proto_compiler": "//rules/proto:protoc"},
)

With build --proto_compiler //rules/proto:protoc in the .bazelrc.

@grahamjenson
Copy link
Contributor

@robbertvanginkel This is amazing, thank you 🥇

@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@fmeum
Copy link
Collaborator

fmeum commented Feb 15, 2023

@sgowroji Still relevant, although it may be fixed by toolchainifying proto rules instead.

@sgowroji sgowroji reopened this Feb 15, 2023
@sgowroji sgowroji added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Feb 15, 2023
@nchepanov
Copy link

nchepanov commented Jan 31, 2024

Hello from 2024 :) This was still relevant for me, discussed in https://bazelbuild.slack.com/archives/CKU1D04RM/p1705423119366659

Here's how you do this today with updated patch and platform settings (and slightly different naming)

bazel 6.3.2, io_bazel_rules_go v0.43.0, protobuf 22.1:

# WORKSPACE
http_archive(
    name = "io_bazel_rules_go",
    patch_args = ["-p1"],
    patches = ["//build/patches:rulesgo_protoc.patch"],
    ...
)

http_archive(
    name = "protoc_bin_osx_universal",
    build_file_content = """exports_files(["bin/protoc"])""",
    sha256 = "3c830b09192a8c40c599856eb184c89ee5029d7dab9df8ec6d3d6741dcb94b93",
    urls = [
        "https://github.com/protocolbuffers/protobuf/releases/download/v22.1/protoc-22.1-osx-universal_binary.zip",
    ],
)

http_archive(
    name = "protoc_bin_linux_x86_64",
    build_file_content = """exports_files(["bin/protoc"])""",
    sha256 = "3c830b09192a8c40c599856eb184c89ee5029d7dab9df8ec6d3d6741dcb94b93",
    urls = [
        "https://github.com/protocolbuffers/protobuf/releases/download/v22.1/protoc-22.1-linux-x86_64.zip",
    ],
)
# build/patches/rulesgo_protoc.patch
diff --git a/proto/BUILD.bazel b/proto/BUILD.bazel
index 2125a93..809a080 100644
--- a/proto/BUILD.bazel
+++ b/proto/BUILD.bazel
@@ -118,7 +118,7 @@ go_proto_compiler(
 
 non_go_reset_target(
     name = "protoc",
-    dep = "@com_google_protobuf//:protoc",
+    dep = "@//build/rules:protoc",
     visibility = ["//visibility:public"],
 )
# build/rules/BUILD.bazel
load("@bazel_skylib//rules:native_binary.bzl", "native_binary")

native_binary(
    name = "protoc",
    src = select({
        "//conditions:default": "@com_google_protobuf//:protoc",
        ":prebuilt_protoc_macos": "@protoc_bin_osx_universal//:bin/protoc",
        ":prebuilt_protoc_linux": "@protoc_bin_linux_x86_64//:bin/protoc",
    }),
    out = "protoc",
    visibility = ["//visibility:public"],
)

config_setting(
    name = "prebuilt_protoc_macos",
    constraint_values = [
        "@platforms//os:osx",
    ],
    values = {"proto_compiler": "//rules/proto:protoc"},
)

config_setting(
    name = "prebuilt_protoc_linux",
    constraint_values = [
        "@platforms//os:linux",
        "@platforms//cpu:x86_64",
    ],
    values = {"proto_compiler": "//rules/proto:protoc"},
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Server Issues for serverside rules included with Bazel type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants