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 support for import_prefix to protobuf rule #393

Closed
wants to merge 3 commits into from

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Jun 13, 2019

This change switches closure_proto_library to use the binary descriptors returned by proto_library instead of invoking protoc directly on the source files.

To reduce the code duplication for Starlark *_proto_library implementations, rules_proto is used to generate JavaScript files from proto files.

Fixes #390

cc @sgammon

@sgammon
Copy link
Contributor

sgammon commented Jun 13, 2019

@Yannic you are my hero, thank you!! i'll give it a shot

@sgammon
Copy link
Contributor

sgammon commented Jun 15, 2019

@Yannic, sorry for the delay. i finally had a chance to check this out, and i figured out where to override it (we use rules_closure in a number of places in the project tree).

it does change the behavior, but, unfortunately, i get a crash of rules_closure. here's what i get as output:

ERROR: /private/var/tmp/_bazel_sam/.../external/project_with_protos/sources/.../BUILD.bazel:13:1: in @io_bazel_rules_closure//closure/protobuf:closure_proto_library.bzl%closure_proto_aspect aspect on proto_library rule @project_with_protos//sources/:proto_target: 
Traceback (most recent call last):
	File "/private/var/tmp/_bazel_sam/.../external/project_with_protos/.../BUILD.bazel", line 13
		@io_bazel_rules_closure//closure/protobuf:closure_proto_library.bzl%closure_proto_aspect(...)
	File "/private/var/tmp/_bazel_sam/.../external/io_bazel_rules_closure/closure/protobuf/closure_proto_library.bzl", line 49, in _closure_proto_aspect_impl
		proto_common.lang_proto_aspect_impl(actions = ctx.actions, toolchain =...], <6 more arguments>)
	File "/private/var/tmp/_bazel_sam/.../external/build_bazel_rules_proto/proto/proto_common.bzl", line 141, in proto_common.lang_proto_aspect_impl
		actions.run(inputs = depset(plugin_files, tr...]), <5 more arguments>)
Index 0 out of bounds for length 0

Obviously, the key here is Index 0 out of bounds for length 0. We have our own fork to test with now, at bloombox/rules_closure, so that's where I'll do some testing and see if I can dig in.

it looks like the code where this occurs is here in rules_closure, and it's calling into rules_proto here.

@Yannic
Copy link
Contributor Author

Yannic commented Jun 18, 2019

@sgammon Thanks for trying this out!

I've found some issues with rules_proto when external workspaces are involved while porting some more *_proto_library rules. This should be fixed in rules_proto@head, so let me just update this PR :).

I'm not sure if that'll also fix the bug you're seeing. From what I've seen, there isn't any array access near the location the crash happened, and the error message looks like the Exception Java throws for out-of-bounds array access. What version of Bazel are you using?

@laurentlb
Copy link
Contributor

Index 0 out of bounds for length 0
I think I've seen the error before when the set of inputs is empty.

@Yannic
Copy link
Contributor Author

Yannic commented Jun 18, 2019

Thanks! Creating a proto_library without srcs reproduced it.

@Yannic Yannic force-pushed the proto_use_descriptor branch from 93d7144 to cb3133d Compare June 18, 2019 17:32
@sgammon sgammon mentioned this pull request Jun 20, 2019
7 tasks
@Yannic
Copy link
Contributor Author

Yannic commented Jul 26, 2019

Blocked on bazelbuild/rules_proto#6

@Yannic
Copy link
Contributor Author

Yannic commented Sep 7, 2019

Withdrawing this for now. I'll send a new PR when [1] is accepted and implemented.

[1] https://docs.google.com/document/d/1u95vlQ1lWeQNR4bUw5T4cMeHTGJla2_e1dHHx7v4Dvg/edit

@Yannic Yannic closed this Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of strip_import_prefix with closure_proto_library
4 participants