search

bazelbuild / rules_closure

Closure rules for Bazel
https://developers.google.com/closure
Apache License 2.0
153 stars 114 forks source link

Issues building protos, and using them #388

Open sgammon opened 5 years ago

sgammon commented 5 years ago

Hey there rules_closure folks,

I was wondering if someone could help me out with compilation and use of protos from Soy (in JS and Java). First things first, I'm aware there is not yet support in rules_closure for proto use from Java, although I was not able to answer that question fully for JS.

SomeModel.proto:


syntax = "proto3";
package sample;

message Sample {
    string example = 1;
}

WORKSPACE:

workspace(name = "some_workspace")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

## Closure
http_archive(
    name = "io_bazel_rules_closure",
    strip_prefix = "rules_closure-9e82c789d83ddc57e98e11ecb6c0f0479ebbbb5b",
    url = "https://github.com/bazelbuild/rules_closure/archive/9e82c789d83ddc57e98e11ecb6c0f0479ebbbb5b.zip")

BUILD:

package(default_visibility = ["//visibility:public"])

load("@io_bazel_rules_closure//closure:defs.bzl",
     "closure_js_proto_library",
     "closure_proto_library")

# Protos
proto_library(
    name = "SomeModel",
    srcs = ["SomeModel.proto"])

closure_js_proto_library(
    name = "SomeModel_js_proto",
    srcs = ["SomeModel.proto"])

closure_proto_library(
    name = "SomeModel_closure_proto",
    deps = [":SomeModel"])

I can't get Soy to recognize the proto types, whether I'm using closure_js_proto_library or closure_proto_library. The behavior seems rather strange:

When using closure_js_proto_library:

It seems to me, that one of closure_js_template_library or closure_js_proto_library is expecting the protos to be in source form (hence the message No syntax specified for the proto file)? What am I doing wrong?

sgammon commented 5 years ago

When using closure_proto_library:

soy/templates/base/sample.soy:66: error: Unknown type 'sample.Sample'. {@param proto2: sample.Sample} // Sample proto with unqualified path. ^ 2 errors

INFO: Elapsed time: 74.218s, Critical Path: 49.91s INFO: 630 processes: 572 local, 58 worker. FAILED: Build did NOT complete successfully

Yannic commented 5 years ago

Regarding closure_proto_library: Unfortunately, that's a known limitation of the new rule. #314 aimed to fix that, but it was decided that it's a bit weird to have this special handling for templates. @gkdn I agree with your previous comment in #314, but it might be worth to merge it anyway, so people don't have to use the legacy closure_js_proto_library rule.

Regarding closure_js_proto_library: I haven't used that in a very long time. I think https://github.com/bazelbuild/rules_closure/blob/master/closure/templates/test/BUILD might help you to figure out the right way to do what you're trying to do.

sgammon commented 5 years ago

@Yannic, i actually got it to work somehow, i'm still trying to figure out now if i'm using a fork and it's my local state, or if it was fixed in master for my use case. but thank you for your response in any case.

regarding closure_proto_library, the restriction for only one entry in deps is rather cumbersome. are there still plans to lift this restriction? we keep a whole tree of dependencies in sync with proto_library definitions, so, if there would be some way to make that sufficient, we would be very much in favor of it, and would help in any way we could to see that happen.

sgammon commented 5 years ago

@Yannic then again, if that one dep supports aliases, and transitive dependencies, that would be fine with us (in terms of closure_proto_library). thank you again, this project is awesome and we want to help any way we can

sgammon commented 5 years ago

@gkdn please, could we merge that PR, pending a better approach? 😁

Yannic commented 5 years ago

@sgammon We have this restriction to one entry to be in sync what {cc,java}_proto_library do, so I'd rather not lift that restriction.

I'm not sure what you mean by alias, but transitive dependencies should just work.

proto_library(
    name = "a_proto",
    srcs = ["a.proto"],
)

closure_proto_library(
    name = "a_closure_proto",
    deps = [":a_proto"],
)

proto_library(
    name = "b_proto",
    srcs = ["b.proto"],
    deps = [":a_proto"],
)

proto_library(
    name = "c_proto",
    srcs = ["c.proto"],
    deps = [":b_proto"],
)

closure_proto_library(
    name = "c_closure_proto",
    deps = [":c_proto"],
)

closure_js_library(
    name = "my_lib",
    srcs = [
        # Use everything from `a.proto` and `c.proto`, no need to add an explicit dependency on `b.proto`.
        # Using stuff from `b.proto` directly (e.g. `goog.require`'ing it) is possible if you suppress the indirect dependency warnings.
    ],
    deps = [":a_closure_proto", ":c_closure_proto"],
)

Does this answer your question?

sgammon commented 5 years ago

yes, i think that does help. so it's just that we need to get that PR merged, so we can use that new rule with Soy (just clarifying)?

Yannic commented 5 years ago

I think so, but that's @gkdn's decision.

sgammon commented 5 years ago

@Yannic one more question, if you don't mind. our proto_library definitions include a strip_import_prefix value, which we need to be able to compile them correctly.

i would use closure_js_proto_library in the meantime, because it does work for what we need it to do, but when i try to invoke it, it complains about not being able to import dependencies (which is likely an issue with strip_import_prefix, which fixed that for us).

is there any equivalent to strip_import_prefix with closure_js_proto_library?

AFAICT, the new rule fixes this by using binary descriptors, rather than needing proto sources. am I right about that?

Yannic commented 5 years ago

AFAICT, there is no equivalent to strip_import_prefix with closure_js_proto_library.

I'm not sure if strip_import_prefix works with closure_proto_library because it's a rather new attribute. I don't think we use binary descriptors yet, but maybe that's a good idea. Would you mind trying out whether this works, create a new issue if it doesn't and ask @gkdn to assign it to me? If that doesn't work, I'll also have to fix it for closure_grpc_library...

sgammon commented 5 years ago

@Yannic actually, using closure_proto_library seems to work perfectly, when depending on proto_library declarations with strip_import_prefix.

i'm using it with soy now, building templates (that reference protos) that render correctly, using the factuno-db fork on the soy-fix branch, specifically 2736f1d.

sgammon commented 5 years ago

@Yannic, pardon me, i spoke too soon. it was working perfectly because the underlying proto had no imports in it.

with a more complex example, i get the following output (i'll file an issue and list this there as well):

ERROR: /private/var/tmp/_bazel_sam/.../external/schema/vendor/google/bq/BUILD.bazel:16:1: Generating JavaScript Protocol Buffer bq failed (Exit 1): protoc failed: error executing command 
  (cd /private/var/tmp/_bazel_sam/.../execroot/SomeWorkspace && \
  exec env - \
  bazel-out/host/bin/external/com_google_protobuf/protoc -Ibazel-out/darwin-fastbuild/genfiles/external/schema -Ibazel-out/darwin-fastbuild/genfiles/external/com_google_protobuf '--js_out=import_style=closure,library=bq,add_require_for_enums:bazel-out/darwin-fastbuild/bin/external/schema/vendor/google/bq')
Execution platform: @bazel_tools//platforms:host_platform
Missing input file.
INFO: Elapsed time: 74.141s, Critical Path: 15.87s
INFO: 239 processes: 229 local, 10 worker.
FAILED: Build did NOT complete successfully

The key item there, of course, is Missing input file.. It's looking for a vendored proto file and can't find it.