Use released protoc binaries #36

ribrdb · 2019-11-15T00:30:00Z

Here's my first stab at implementing issue #35
I wasn't sure if this code should go here, in https://github.com/protocolbuffers/protobuf or in a separate repo.

This creates a stub workspace with aliases for everything in @com_google_protobuf that either points to the real thing from the protobuf repo, or a binary if it's available. This is mostly working, however I have a few problems:

It appears aliases don't work for .bzl files. I think we'd either need to copy the .bzl files into this stub workspace, or put the binary aliases directly into the real @com_google_protobuf BUIlD file.
I'm not sure if this is working for java_proto_libraries. It seems like they may still be depending on the custom built protoc somehow.

This creates a stub workspace for com_google_protobuf that aliases protoc, protobuf_java, and protobuf_javalite to use the release binaries instead of building them from source. All the other public targets are aliased to the real com_google_protobuf workspace.

…a file.

hlopko · 2019-11-15T13:12:42Z

This looks good, but I'll take a deeper look once I get back from vacation (leaving for a week right now :)

Alternative I was thinking about was to introduce a protobuf toolchain using https://docs.bazel.build/versions/master/toolchains.html. That would save us from using all those aliases and selects (we would effectively replace them with toolchain resolution). Added benefit would be having the abstraction for protobuf toolchain there, and that would allow us to push it further and use .e.g system installed protoc or compile from scratch on the platform that has a precompiled variant ready.

Both approaches are equivalent in power, it's just that each optimizes for smth else. What do you think?

ribrdb · 2019-11-15T14:30:53Z

Part of my goal was that existing rules wouldn't have to change, anything depending on @com_google_protobuf//protoc should just keep working, but faster. For example I tested rules_go, rules_closure, and my own custom codegen rules.

It looks like toolchains are more flexible, e.g. someone can add their own build of protoc. But i think everything would have to be updated to use the toolchain instead of the binary. I guess that could be an advantage too. If there is some unforseen incompatibility, having users opt in instead of opt-out could be good.

Yannic · 2019-11-15T14:41:46Z

Overall, this looks good. I‘ll have a deeper look next month (there are a few things that I need to do first).

IMO, we eventually should use a proper toolchain for protoc eventually (I started working on this in #25 a while ago), but I‘m not sure how feasible this is right now while some (all) proto rules are still native rules.

Yannic

Sorry for the delay, I finally found the time to take a look :).

The overall approach looks good, but there are some things that need a bit of work until this is ready for merging.

/Re Marcel's comment: I might be missing something, but I think this will integrate nicely with proto_toolchain in the future (i.e. as outlined in the design-doc and #25).

proto/private/BUILD.protoc

proto/private/BUILD.release

Yannic · 2019-12-18T14:54:08Z

proto/private/BUILD.release

+    visibility = ["//visibility:public"],
+)
+
+# Redirect everything else to the source


How about making this

redirect_targets = [ "wrappers_proto", # ... ] [ alias( name = target, actual = "@com_github_protocolbuffers_protobuf//:" + target, visibility = ["//visibility:public"], ) for target in redirect_targets ]

Yannic · 2019-12-18T14:58:23Z

proto/private/BUILD.release

+    visibility = ["//visibility:public"],
+)
+
+alias(


This shouldn't be an alias. Otherwise, java_proto_library will depend on @com_github_protocolbuffers_protobuf//:protobuf_java instead of the jar from maven.

Also, please add a proto_lang_toolchain for javalite: protocolbuffers/protobuf#6882

Yannic · 2019-12-18T15:03:03Z

proto/repositories.bzl


 def rules_proto_dependencies():
    for name in dependencies:
        maybe(http_archive, name, **dependencies[name])
+    for name in maven_dependencies:
+        maybe(native.maven_jar, name, **maven_dependencies[name])


native.maven_jar is deprecated and, AFAICT, is removed in Bazel 2.0.
The preferred way of declaring Java dependencies seems to be https://github.com/bazelbuild/rules_jvm_external these days, but I don't think we can use that here. Hopefully, the federation will solve this issue for us.

//cc @fweikert who seems to be working on integrating rules_proto into the federation.

Switched to java_import_external

Yannic · 2019-12-18T15:05:01Z

proto/private/dependencies.bzl

+    ctx.actions.run_shell(
+        inputs = [ctx.file.src],
+        outputs = [ctx.outputs.executable],
+        command = "cp $1 $2&&chmod +x $2",


I'd rather not add a dependency on bash (because of Windows), but I can't think of a better way to copy binaries :/.

I switched to a genrule with both cmd_shell and cmd_bat. Does that work? I don't have a way to test it on windows though.

kalbasit · 2020-04-09T00:06:42Z

@ribrdb are you planning on getting this off the finish line?

ribrdb · 2020-04-09T17:57:07Z

I might be able to get back to this next week. Does anyone have ideas on how to handle the java dependencies?
What versions of bazel are you supporting? My main project is still on 1.2.1.

…to binaries

Yannic

Thanks! Mostly looks good.

I'll look for someone who can upload to mirror.bazel.build. In the meantime, can you already add the urls? Convention is http(s)://foo.bar/baz -> https://mirror.bazel.build/foo.bar/baz.

proto/private/BUILD.protoc

Yannic · 2020-04-13T23:46:09Z

proto/private/BUILD.release

+    "protoc_lib",
+    "protobuf",
+    "protobuf_lite",
+    "protobuf_java_util",


Is there a reason not to use the maven version for protobuf_java_util (haven't checked)?

I don't think so, just missed it.

proto/private/dependencies.bzl

+        "build_file": "@rules_proto//proto/private:BUILD.protoc",
+        "sha256": "3994233e61c287a377a9134e658ca3034924849f0e3a82d12b0e6efa2bed4b46",
+        "urls": [
+            "https://github.com/protocolbuffers/protobuf/releases/download/v3.11.3/protoc-3.11.3-linux-aarch_64.zip",


proto/private/dependencies.bzl

Yannic · 2020-04-14T00:19:58Z

As far as I'm concerned, rules_proto supports the latest Bazel version (currently 3.0.0) + HEAD. That said, currently, anything > 1.0 probably works (I think the absolute minimum may actually be when native.proto_common was introduced (IIRC, around 0.23), but that's definitely not tested or supported).

kalbasit · 2020-04-14T06:10:47Z

Does this allow me to configure Bazel to use the path/label to the binary as opposed to build it or download it?

- Add script to generate sha256 sums - Get rid of BUILD.protoc_windows

ribrdb · 2020-04-15T17:23:06Z

Does this allow me to configure Bazel to use the path/label to the binary as opposed to build it or download it?

No.
I would guess that's what the proto toolchains are supposed to do, but I haven't seen the design doc for that.

kalbasit · 2020-04-22T16:38:27Z

Does this allow me to configure Bazel to use the path/label to the binary as opposed to build it or download it?

No.
I would guess that's what the proto toolchains are supposed to do, but I haven't seen the design doc for that.

That's too bad, but I can live with it for now.

Are we close to getting this done? Seems the CI is failing the build due to rules_proto_dependencies lacking docstring.

ribrdb · 2020-04-22T16:45:49Z

Yeah, rules_proto_dependencies was already missing a docstring. I guess I could add one, but I don't know what it would say.

We've been using this branch at my company for the last week with no problems, and our builds are much faster.
Are we still waiting on getting things copied to to the bazel mirror?

Yannic · 2020-04-22T16:49:00Z

proto/private/BUILD.protoc

+        ":windows": ["bin/protoc.exe"],
+        "//conditions:default": ["bin/protoc"]
+    }),
+    executable = "protoc_bin",


Shouldn't this be protoc.exe then?

Yannic · 2020-04-22T16:50:21Z

proto/private/BUILD.protoc

+    constraint_values = [
+        "@platforms//os:windows",
+    ],
+)


Please run buildifier on this file (should have trailing newline, CI doesn't check these files, unfortunately).

proto/private/BUILD.release

Yannic · 2020-04-22T16:53:03Z

proto/private/dependencies.bzl

        "sha256": "cf754718b0aa945b00550ed7962ddc167167bd922b842199eeb6505e6f344852",
        "strip_prefix": "protobuf-3.11.3",
        "urls": [
+            "https://mirror.bazel.build/mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz",


Suggested change

"https://mirror.bazel.build/mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz",

"https://mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz",

Are you asking me to upgrade everything to 3.11.4 instead of 3.11.3? Or just fix the broken urls?

Sorry, yes, this is only about the broken URL. (If you feel like it, you can upgrade to 3.11.4, but I'm not asking for it as part of this PR).

proto/private/dependencies.bzl

proto/repositories.bzl

proto/private/dependencies.bzl

proto/private/generate_sums.sh

Yannic · 2020-04-22T17:10:05Z

Sorry for the delay, forgot to actually submit my review.

Yannic · 2020-04-22T17:11:51Z

@meteorcloudy Can you help uploading these files to bazel mirror?

Co-Authored-By: Yannic <contact@yannic-bonenberger.com>

sitaktif

Just spotted a typo when going through the code again. Maybe address @eklitzke's minor comments too? (LGTM otherwise)

sitaktif · 2020-07-20T10:19:08Z

proto/private/dependencies.bzl

+    attrs = {
+        "_build": attr.label(default = "@rules_proto//proto/private:BUILD.release"),
+        "_protobuf_bzl": attr.label(default = "@com_github_protocolbuffers_protobuf//:protobuf.bzl"),
+        "_protobuf_deps_bzl": attr.label(default = "@com_github_protocolbuffers_protobuf//:protobuf.bzl"),


s/protobuf.bzl/protobuf_deps.bzl

kalbasit · 2020-07-21T20:56:57Z

Seems like @lberki 's approval is required because they are code owners.

Co-authored-by: Evan Klitzke <evan@eklitzke.org>

Avoid compiling protoc during bazel build. Based on change in rules_proto bazelbuild/rules_proto#36 rules_proto_dependencies() declares a @com_google_protobuf//:protoc which points to released protoc binary. The tricks is to load that before scala_repositories() which has conditional load for @com_google_protobuf

* Use released protoc binaries Avoid compiling protoc during bazel build. Based on change in rules_proto bazelbuild/rules_proto#36 rules_proto_dependencies() declares a @com_google_protobuf//:protoc which points to released protoc binary. The tricks is to load that before scala_repositories() which has conditional load for @com_google_protobuf * lint fix * Align rules_proto versions * Update readme * Revert "Update readme" This reverts commit b9a3606.

nikunjy · 2021-01-20T17:24:59Z

Hey @ribrdb thanks for the fix. How do I actually use this ? For context I am using rules_go and rules_docker and rules_proto_grpc (making python work) in my bazel workspace which somehow already pull in rules_proto.

I tried adding the recent version to my WORKSPACE but it is still recompiling.

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "rules_proto",
    sha256 = "3bce0e2fcf502619119c7cac03613fb52ce3034b2159dd3ae9d35f7339558aa3",
    strip_prefix = "rules_proto-84ba6ec814eebbf5312b2cc029256097ae0042c3",
    urls = [
        "https://github.com/bazelbuild/rules_proto/archive/84ba6ec814eebbf5312b2cc029256097ae0042c3.tar.gz",
    ],
)

load("@rules_proto_grpc//:repositories.bzl", "rules_proto_grpc_toolchains", "rules_proto_grpc_repos")
rules_proto_grpc_toolchains()
rules_proto_grpc_repos()

load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")
rules_proto_dependencies()
rules_proto_toolchains()

I know it is hard to just look at this and find out the reason. Maybe you can tell me how to best debug what is happening?

rmohr · 2021-07-01T08:07:41Z

I know it is hard to just look at this and find out the reason. Maybe you can tell me how to best debug what is happening?

@nikunjy FYI, we use rules_go and rules_docker too. I had similar issues, but finally this worked: https://github.com/kubevirt/kubevirt/pull/5959/files#diff-5493ff8e9397811510e780de47c57abb70137f1afe85d1519130dc3679d60ce5

ribrdb added 2 commits November 10, 2019 10:14

Add a protoc_binary rule so that @com_google_protobuf//:protoc isn't …

ba01445

…a file.

ribrdb requested review from hlopko and lberki as code owners November 15, 2019 00:30

Yannic suggested changes Dec 18, 2019

View reviewed changes

ribrdb added 7 commits April 13, 2020 08:59

Merge branch 'master' of https://github.com/bazelbuild/rules_proto in…

6d03100

…to binaries

Address PR feedback.

ed90c15

Run buildifier.

cf51e7a

Fix typo

19458d4

Fix shasum for windows_x86_64

a0ea786

Fix lint warnings.

b607048

Fix another lint error.

4fd675f

Yannic reviewed Apr 14, 2020

View reviewed changes

- Add mirror.bazel.build URLS

33cd420

- Add script to generate sha256 sums - Get rid of BUILD.protoc_windows

Yannic reviewed Apr 22, 2020

View reviewed changes

ribrdb and others added 3 commits April 22, 2020 11:53

Add license header to generate_sums.sh

92bf3de

Co-Authored-By: Yannic <contact@yannic-bonenberger.com>

Add docstrings.

ed8ab65

Co-Authored-By: Yannic <contact@yannic-bonenberger.com>

Run buildifier and fix typos.

0abe018

comius approved these changes Jul 20, 2020

View reviewed changes

comius assigned comius and unassigned lberki Jul 20, 2020

comius self-requested a review July 20, 2020 10:00

comius previously approved these changes Jul 20, 2020

View reviewed changes

sitaktif reviewed Jul 20, 2020

View reviewed changes

meteorcloudy previously approved these changes Jul 22, 2020

View reviewed changes

Fix protobuf_deps in dependencies.bzl

193def1

comius dismissed stale reviews from meteorcloudy, eklitzke, Yannic, and themself via 193def1 August 17, 2020 08:36

comius and others added 3 commits August 17, 2020 10:38

Add double quotes in proto/private/generate_sums.sh

abf125b

Co-authored-by: Evan Klitzke <evan@eklitzke.org>

Add double quotes in proto/private/generate_sums.sh

16f1c5d

Co-authored-by: Evan Klitzke <evan@eklitzke.org>

Add double quotes in proto/private/generate_sums.sh

28b9ae8

Co-authored-by: Evan Klitzke <evan@eklitzke.org>

comius approved these changes Aug 17, 2020

View reviewed changes

comius merged commit 7477811 into bazelbuild:master Aug 17, 2020

alexeagle mentioned this pull request Oct 1, 2020

Builds protoc from source grpc/grpc-web#967

Open

simuons mentioned this pull request Nov 18, 2020

Use released protoc binaries bazelbuild/rules_scala#1142

Merged

thii mentioned this pull request Feb 5, 2021

RFC: Use precompiled libprotobuf.a and libprotobuf-lite.a for macOS x86_64 host/exec configuration #83

Closed

This was referenced Jun 30, 2021

Support using precompiled protoc binaries bazelbuild/rules_go#2902

Closed

Use precompiled protoc kubevirt/kubevirt#5959

Merged

rdesgroppes mentioned this pull request May 26, 2023

Add preliminary support for exe packaging type bazelbuild/rules_jvm_external#908

Merged

	"https://mirror.bazel.build/mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz",
	"https://mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz",

Use released protoc binaries #36

Use released protoc binaries #36

Conversation

ribrdb commented Nov 15, 2019

hlopko commented Nov 15, 2019

ribrdb commented Nov 15, 2019

Yannic commented Nov 15, 2019

Yannic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalbasit commented Apr 9, 2020

ribrdb commented Apr 9, 2020

Yannic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Yannic commented Apr 14, 2020

kalbasit commented Apr 14, 2020 • edited

ribrdb commented Apr 15, 2020

kalbasit commented Apr 22, 2020

ribrdb commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic Apr 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Apr 22, 2020

Yannic commented Apr 22, 2020

sitaktif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalbasit commented Jul 21, 2020

nikunjy commented Jan 20, 2021

rmohr commented Jul 1, 2021 • edited

kalbasit commented Apr 14, 2020 •

edited

Yannic Apr 22, 2020 •

edited

rmohr commented Jul 1, 2021 •

edited