[WIP] Add `import_prefix` and `strip_import_prefix` to `proto_library`. #5536

jmillikin-stripe · 2018-07-07T02:35:35Z

WIP because I'd like feedback on this approach, and it's not ready to merge yet.

Figure out FIXMEs in ProtoCompileActionBuilder (I don't understand the implications of @AutoCodec)
Write documentation for the new attributes.
Update doc comment on adjustImportPath once its behavior is settled.
Remove printf debugging.

jmillikin-stripe · 2018-08-27T14:56:23Z

Pushed the in-progress ProtoInfo provider work so you can see where I'm stuck. My rough plan is:

Add the new provider to describe .proto inputs, path adjustments, and the resulting binary descriptor
Extend proto_library() to return that provider.
Change cc_proto_library() and other built-in protoc callers to use the provider for path calculation.
Test patched version of rules_go with the provider to verify the Envoy control plane and Kubernetes protos can use it.

lberki

(/cc @laurentlb for someone who (rightfully) wants to create a new Java `TransitiveInfoProvider and is having difficulties with it -- it really is not trivial, I myself also usually resort to copy-paste programming...)

lberki · 2018-08-28T11:02:22Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java

+
+@Immutable
+@AutoCodec
+public final class ProtoInfo extends NativeInfo implements ProtoInfoApi<Artifact> {


Why do you need to create a new provider? There is already one that tells which source files a proto_library provides, ProtoSourcesProvider.

ProtoSourcesProvider doesn't seem to be exactly what I want here -- it describes only the .proto source files, but doesn't have information at the target level.

I'm thinking a ProtoInfo provider would have a srcs :: ProtoSourcesProvider, and also provide fields to discover what the prefix handling should be.

Alternatively, we could try adding info into ProtoSourcesProvider such that each source is no longer just a file, but a (path, file) tuple.

Ooh, looking through this again I remember the other reasons I started on a new provider:

ProtoSourcesProvider is an "old-style" provider, so it's not possible to return it from a Skylark rule (ProtoSourcesProvider is not available as a Provider #3701).

The relationship between ProtoSourcesProviderApi and ProtoSourcesProvider is pretty unclear to me -- why is there this separation? The former is generic across implementations of FileApi, are there any cases where we'd use something other than Artifact as the file type of a protobuf library target?

I suspect the second is more of an Enterprise™ Java™ Best™ Practice™ thing vs a Bazel-specific issue, but it does aggravate the lack of documentation when I'm not able to figure out why there are multiple Java types that seem related to the same Skylark type.

Well, I'm also somewhat bemused at the presence of *Api classes, but I assume the Starlark folks know what they are doing, so let's follow that example.

I think that the best course of action here is to turn ProtoSourcesProvider into a new-style provider. The old and new styles can coexist for an indefinite period of time so you would not need to do the shave the "deprecate the old-style API for ProtoSourcesProvider" yak. Adding the new-style (provider-based) API does require a bit of boilerplate, but following the footsteps of JavaRuntimeInfo and JavaInfo should be pretty straightforward: you make ProtoSourcesProvider a subclass of BuiltinProvider, add a static field called PROVIDER to it, then add a method to proto_common (just like JavaSkylarkCommon#getJavaProvider) that returns the provider.

(Unfortunately, neither JavaInfo nor JavaRuntimeInfo is 100% correct: the former makes JavaInfo a top-level symbol, which is wrong, and the latter uses one of the deprecated APIs for Skylark providers defined in Java)

Looking at the code, there are plenty of places where we add the result of ProtoSourcesProvider#getTransitiveProtoSources() to nested sets so it's probably better to either another field returning a nested set of (path, artifact) tuples or, even better, that only for those artifacts whose path is different from the default one. Nested sets can contain anything that has a #equals() / #hashCode() method. In particular, AutoValue works.

lberki · 2018-08-28T11:02:48Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java

@@ -596,13 +605,27 @@ static void addIncludeMapArguments(
   * </code>
   */
  private static String getPathIgnoringRepository(Artifact artifact) {
+    // FIXME(jmillikin): How can I plumb `ruleContext` through the @AutoCodec closures


RuleContext should not be put into a TransitiveInfoProvider. Its lifetime is limited to the analysis of that particular configured target. Why do you need to do that? For the getPathIgnoringRepository() call?

Yes, I wanted some way to get at the properties of the rule (which AIUI are only part of RuleContext).

lberki · 2018-08-28T11:04:57Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java

+
+  public static final ProtoInfoProvider PROVIDER = new ProtoInfoProvider();
+
+  public static final ProtoInfo EMPTY = ProtoInfo.Builder.create().build();


That said, this looks like the right way to create a Java provider. How does this not work?

It just never appears in the Skylark context.

There's no error messages that I can see, and I don't see any sort of central registry of Skylark symbols, so I'm at a bit of a loss about why Skylark doesn't see this type.

Oh, that I know :) Just adding a PROVIDER field is not enough, you also need to tell Bazel that this should be available as a symbol. Here is now Java does it:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java#L87

Add a proto_common top-level symbol should be pretty straightforward by following that template. Let me know if it still doesn't work!

lberki · 2018-08-28T11:11:28Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java

-    commandLine.addAll(VectorArg.of(transitiveImports).mapped(EXPAND_TRANSITIVE_IMPORT_ARG));
+    for (Artifact proto : transitiveImports) {
+      commandLine.addFormatted(
+          "-I%s=%s", getPathIgnoringRepository(ruleContext, proto), proto.getExecPathString());


Why do you compute the path a particular proto file is accessible at here instead of the rule in whose srcs attribute the proto file is? That way, you would not need a RuleContext.

And fortunately, ProtoSourcesProvider (or your new provider) already providers a place to do that. What I would do is to make ProtoSourcesProvider#getTransitiveProtoSources() a NestedSet of not artifacts, but of import path - artifact pairs and compute the import path of a .proto file in the proto_library() rule that owns it. Alternatively, if you think that the version without import path munging is the common case, have two methods: one for default import paths (without import_prefix or strip_import_prefix attributes) and one for changed import paths.

Thinking again, the latter is preferable because it makes merging this code a bit easier due to the existing users of that method.

I was looking for where protoc was invoked, then trying to modify code closest to that invocation (for testing). It's certainly likely that this logic belongs in some other part of the build graph.

Are there any constraints on what types a NestedSet can contain? I'm not very familiar with Java, but IIRC there's no generic tuple types -- do I need to find a ProtoSourcePath type to handle that? Does it need to implement some sort of hashable interface to be put in a NestedSet?

lberki · 2018-08-28T11:12:54Z

src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java

@@ -77,7 +79,14 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
        the source root will be the workspace directory (default).
        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
        .add(attr("proto_source_root", STRING))
+        /* <!-- #BLAZE_RULE(proto_library).ATTRIBUTE(import_prefix) -->
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("import_prefix", STRING))


Extra kudos for making the logic consistent with what cc_library does :)

jmillikin-stripe · 2018-09-07T18:46:56Z

Sorry for the delayed response, having some task-switch overhead. Comments inline.

tautomaton · 2018-09-16T16:00:28Z

Any updates here? I think this PR will fix a major pain point of working with protos in Bazel. They inherit the same problems from the use of importing/including a specified path that C/C++ headers do. So adopting the same approach to amending the import path as the C/C++ rules seems like a great idea.

lberki

Erm, I apologize for the delay. This got lost in my inbox somewhere, not very professional.

lberki · 2018-10-01T11:05:28Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java

+
+  public static final ProtoInfoProvider PROVIDER = new ProtoInfoProvider();
+
+  public static final ProtoInfo EMPTY = ProtoInfo.Builder.create().build();


Oh, that I know :) Just adding a PROVIDER field is not enough, you also need to tell Bazel that this should be available as a symbol. Here is now Java does it:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java#L87

Add a proto_common top-level symbol should be pretty straightforward by following that template. Let me know if it still doesn't work!

lberki · 2018-10-01T11:15:47Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java

+
+@Immutable
+@AutoCodec
+public final class ProtoInfo extends NativeInfo implements ProtoInfoApi<Artifact> {


Well, I'm also somewhat bemused at the presence of *Api classes, but I assume the Starlark folks know what they are doing, so let's follow that example.

I think that the best course of action here is to turn ProtoSourcesProvider into a new-style provider. The old and new styles can coexist for an indefinite period of time so you would not need to do the shave the "deprecate the old-style API for ProtoSourcesProvider" yak. Adding the new-style (provider-based) API does require a bit of boilerplate, but following the footsteps of JavaRuntimeInfo and JavaInfo should be pretty straightforward: you make ProtoSourcesProvider a subclass of BuiltinProvider, add a static field called PROVIDER to it, then add a method to proto_common (just like JavaSkylarkCommon#getJavaProvider) that returns the provider.

(Unfortunately, neither JavaInfo nor JavaRuntimeInfo is 100% correct: the former makes JavaInfo a top-level symbol, which is wrong, and the latter uses one of the deprecated APIs for Skylark providers defined in Java)

Looking at the code, there are plenty of places where we add the result of ProtoSourcesProvider#getTransitiveProtoSources() to nested sets so it's probably better to either another field returning a nested set of (path, artifact) tuples or, even better, that only for those artifacts whose path is different from the default one. Nested sets can contain anything that has a #equals() / #hashCode() method. In particular, AutoValue works.

lberki · 2018-10-01T11:19:58Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/proto/ProtoInfoApi.java

+          + "more providers created with different ijar requests </li> </ul>",
+      structField = true
+  )
+  public SkylarkNestedSet getCompileTimeJars();


Why all these Java-specific things in ProtoInfo?

Dunno, this isn't my code. I think it got picked up by a rebase.

It looks like copypasta from JavaInfoApi.java .

jmillikin-stripe · 2018-11-27T22:44:57Z

Closing in favor of Lukács's branch.

[WIP] Add import_prefix and strip_import_prefix to proto_library.

bb230cd

googlebot added the cla: yes label Jul 7, 2018

jmillikin-stripe mentioned this pull request Jul 7, 2018

Please add proto_library attrs to adjust its "virtual path" #3867

Closed

laurentlb requested a review from iirina August 14, 2018 18:16

aiuto assigned iirina Aug 16, 2018

aiuto added the WIP label Aug 17, 2018

[WIP] flailing around adding a new ProtoInfo provider

2d7a872

jmillikin-stripe requested a review from lberki as a code owner August 27, 2018 14:54

lberki reviewed Aug 28, 2018

View reviewed changes

lberki reviewed Oct 1, 2018

View reviewed changes

pcj mentioned this pull request Oct 3, 2018

Using proto_source_root in proto_library breaks <lang>_grpc_[compile|library] pubref/rules_proto#4

Closed

jmillikin-stripe closed this Nov 27, 2018

jmillikin-stripe deleted the proto-library-virtual-path-attrs branch November 27, 2018 22:45

linzhp mentioned this pull request Jan 8, 2019

Fail to compile proto_library with multiple proto files and strip_import_prefix #7060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add `import_prefix` and `strip_import_prefix` to `proto_library`. #5536

[WIP] Add `import_prefix` and `strip_import_prefix` to `proto_library`. #5536

jmillikin-stripe commented Jul 7, 2018 •

edited

Loading

jmillikin-stripe commented Aug 27, 2018

lberki left a comment

lberki Aug 28, 2018

jmillikin-stripe Sep 7, 2018

jmillikin-stripe Sep 7, 2018

lberki Oct 1, 2018

lberki Aug 28, 2018

jmillikin-stripe Sep 7, 2018

lberki Aug 28, 2018

jmillikin-stripe Sep 7, 2018

lberki Oct 1, 2018

lberki Aug 28, 2018

jmillikin-stripe Sep 7, 2018

lberki Aug 28, 2018

jmillikin-stripe commented Sep 7, 2018

tautomaton commented Sep 16, 2018

lberki left a comment

lberki Oct 1, 2018

lberki Oct 1, 2018

lberki Oct 1, 2018

jmillikin-stripe Oct 1, 2018

lberki Oct 2, 2018

jmillikin-stripe commented Nov 27, 2018


		public static final ProtoInfoProvider PROVIDER = new ProtoInfoProvider();

		public static final ProtoInfo EMPTY = ProtoInfo.Builder.create().build();

[WIP] Add import_prefix and strip_import_prefix to proto_library. #5536

[WIP] Add import_prefix and strip_import_prefix to proto_library. #5536

Conversation

jmillikin-stripe commented Jul 7, 2018 • edited Loading

jmillikin-stripe commented Aug 27, 2018

lberki 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

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

jmillikin-stripe commented Sep 7, 2018

tautomaton commented Sep 16, 2018

lberki 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

jmillikin-stripe commented Nov 27, 2018

[WIP] Add `import_prefix` and `strip_import_prefix` to `proto_library`. #5536

[WIP] Add `import_prefix` and `strip_import_prefix` to `proto_library`. #5536

jmillikin-stripe commented Jul 7, 2018 •

edited

Loading