Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

cheister · 2020-03-15T06:26:04Z

This PR has two changes

Refactor the ProtoCommon code to make it easier to create a ProtoInfo object since it no longer requires the ruleContext. All of the calls to ruleContext were moved to the BazelProtoLibrary class.
Rename ProtoModuleApi to ProtoCommonApi since it exposes "proto_common" to Starlark.

cheister · 2020-03-20T16:21:06Z

@lberki Any chance you'll have time to look at this soon? I'd like to follow this PR up with another that adds a feature to ProtoCommon.

lberki · 2020-03-20T16:49:18Z

This will probably conflict with @Yannic 's #10939 (only textually, there are no philosophical conflicts, which is good)

lberki · 2020-03-20T16:37:37Z

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

@@ -37,13 +37,13 @@
 @AutoCodec
 public final class ProtoInfo extends NativeInfo implements ProtoInfoApi<Artifact> {
  /** Provider class for {@link ProtoInfo} objects. */
-  public static class Provider extends BuiltinProvider<ProtoInfo> implements ProtoInfoApi.Provider {
-    public Provider() {
+  public static class ProtoInfoProvider extends BuiltinProvider<ProtoInfo>


nit: Why this name change? .Provider is the usual name for these classes.

Looking through the code it's about half Provider and half the more specific Provider. I was just making this match the pattern used by JavaInfoProvider

/shrug . In that case, i'd lean towards less churn.

lberki · 2020-03-20T16:38:45Z

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

@@ -37,7 +37,7 @@
 public interface ProtoInfoApi<FileT extends FileApi> extends StructApi {
  /** Provider class for {@link ProtoInfoApi} objects. */
  @SkylarkModule(name = "Provider", documented = false, doc = "")
-  interface Provider extends ProviderApi {
+  interface ProtoInfoProviderApi extends ProviderApi {


Same here -- why is Provider not adequate? It's shorter so all other things being equal, it's better.

Referring to ProtoInfoProviderApi instead of ProtoInfoApi.Provider seems a little bit cleaner and it's the same pattern used by JavaInfoProviderApi

lberki · 2020-03-20T16:43:54Z

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

-    if (library == null) {
-      library = createLibraryWithoutVirtualSourceRoot(ruleContext, directProtoSources);
-    }
+      ImmutableList<Artifact> originalDirectProtoSources,


What's code is coming up? There are a few issues here:

This makes the interface of createProtoInfo quite a bit more verbose

It relies on the caller correctly computing protoSourceRoot and sourceImportPairs

Yes, it is more verbose but it is also more accurate. The ProtoInfo class should only have information related to compiling protos. Currently we're pulling the protoSourceRoot and sourceImportPairs information out of the ruleContext which lets us hide a lot of the interface in the ruleContext. I think it makes more sense to calculate these variables outside of the createProtoInfo function and pass them in.

I think that's reasonable, but you'll need to find a way to prevent people passing in inconsistent data (e.g. not every item in directProtoSources having an entry in sourceImportPathPairs or sourceImportPathPairs inconsistent with protoSourceRoot.

One possible way to go about this is to have two methods: one to create a Library that's guaranteed to be consistent and has access to ctx and createProtoInfo() like you propose here. I can't think up a way to avoid accessing ctx for during the creation of the Library because, well, it at least needs to know in which repository it is.

Mind if I add verification in a second PR? Is adding a TODO: tag ok for now?

Mind doing it in this change? I have become very wary of "temporary" lifting of restrictions because even with the best intent, they have the tendency to become permanent. And it's not more work to do it in one change.

What do you think about the idea of wrapping the creation of Library in a Starlark call instead? That way, you wouldn't need verification because you'd be guaranteed to have it always correct. Something like:

lib = proto_common.create_library(ctx.actions, [proto files], strip_import_prefix, import_prefix) info = proto_common.create_info(lib, [ProtoInfo instances])

It would then create symlink actions if needed and if strip_import_prefix is an exec path then create_library doesn't need ctx only ctx.actions, thereby limiting its power.

Having two functions that are only useful together doesn't sound like a nice API (to me). Depending on what the use-case for the API is, we might have more success by inventing proto_common.compile (or .create_descriptor_set, naming is hard) which does the same thing in one step.

info = proto_common.compile( actions = ctx.actions, srcs = [sequence of `.proto` files], deps = [sequence of ProtoInfo], exports = [sequence of ProtoInfo], import_prefix = None, # maybe just exec path, maybe `absolute -> exec path`, `relative -> relative to a file's owner's package`? strip_import_prefix = None, )

Ok, removed the second method from this PR instead. I think it will be better to discuss it in the follow up PR with the new Starlark method.

@lberki do you mind taking another look at this? I removed the second createProtoInfo method so this PR is only refactoring existing methods.

lberki · 2020-03-20T16:48:52Z

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

+            ruleContext.getPrerequisites("deps", TARGET, ProtoInfo.PROVIDER));
+    ImmutableList<ProtoInfo> exports = ImmutableList.copyOf(
+            ruleContext.getPrerequisites("exports", TARGET, ProtoInfo.PROVIDER));
+    String protoSourceRoot =


What are you planning to add to ProtoCommon? Depending on the magnitude of the feature, we may have to get bazel-discuss / bazel-dev involved.

This will not be very satisfying, but the reason why the code is structured this way at HEAD is that we have a Google-internal alternative implementation of BazelProtoLibrary (which we want to go away, but it's still there) and we found it useful to have all the duplicate code in ProtoCommon. That's why BazelProtoLibrary is so short.

Mind moving all the code you put here into a method in ProtoCommon that's separate from createProtoInfo() instead? (preferably after discussing your future plans)

I want to add a create_proto_info interface to ProtoCommon so Starlark rules can create ProtoInfo providers. This would let us build custom Proto rules that work with the existing proto rules. It would also let use pull the rules out to https://github.com/bazelbuild/rules_proto

For the Google-internal alternative would it work to leave the current createProtoInfo(ruleContext) method in place and add a second createProtoInfo method without the ruleContext from this PR and have the createProtoInfo(ruleContext) call the new createProtoInfo method?

Having two createProtoInfo() versions is reasonable, as long as one calls the other so that there is no duplicate code.

I like the idea of providing a way to create ProtoInfo from Starlark. We'll definitely need it sooner-or-later to rewrite the proto rules in Starlark and move them to the rules_proto repo.

@cheister It seems like you already have something in mind. Would you mind creating a "design-doc issue" (either here or rules_proto) to see what the API will look like (i.e. is it going to be a simple constructor-like function, will it be more of a Sandwich API, ...). We can always involve the mailing list(s) later (if necessary).

Sure. Do you mind if I do that with the follow-up PR that includes the create_proto_info method? What I had in mind was

ProtoInfoT createProtoInfo( String protoSourceRoot, FileApi descriptorSet, Sequence<?> sources, Sequence<?> deps, Sequence<?> exports, StarlarkThread thread) throws EvalException; }

TBH, I prefer an issue.
I'd also like to learn a little about use-cases for this API and maybe have an example of how usage will look like, which, IMO, is harder if we have a PR and look at the code. I absolutely don't expect more than a short write-up.

+1. Given our previous thread about the inconsistencies and consistency verification / creation in such a way that inconsistency is not possible, I think it makes sense to break the thread out into an issue. No need for anything formal, just a place where we can decide on the API.

lberki · 2020-03-20T17:25:26Z

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

+    ProtoInfo protoInfo =
+        ProtoCommon.createProtoInfo(
+            originalDirectProtoSources,
+            library.getSources(),


nit: why not just pass in library?

Even better, then you can put originalDirectProtoSources and directDescriptorSet also into it, which makes it a nice self-contained "things about this particular proto_library rule" class.

Do you mean add originalDirectProtoSources and directDescriptorSet to library ?

…oApi.Provider to ProtoInfoProviderApi, and stop passing ruleContext to every method in ProtoCommon

cheister · 2020-03-25T21:14:33Z

Are you ok with these changes now that the second createProtoInfo method has been removed from protoCommon?

lberki · 2020-04-01T13:47:46Z

Go go go! I sent out the internal change for review by @laurentlb .

lberki · 2020-04-01T15:14:25Z

Erm. Wrong window. I sent out the internal change for #10695 to be reviewed by him. Let me take a look at this now.

lberki

Let me see how this fares with our internal test battery. I may need to do some more work to make everything pass.

cheister requested a review from lberki as a code owner March 15, 2020 06:26

googlebot added the cla: yes label Mar 15, 2020

dslomov assigned lberki Mar 16, 2020

dslomov added the untriaged label Mar 16, 2020

cheister force-pushed the refactor-proto-info branch from 366ac16 to d2e24c4 Compare March 20, 2020 16:25

lberki reviewed Mar 20, 2020

View reviewed changes

lberki mentioned this pull request Mar 20, 2020

Remove import-path guessing from ProtoCompileActionBuilder #10939

Closed

lberki reviewed Mar 20, 2020

View reviewed changes

cheister force-pushed the refactor-proto-info branch from d2e24c4 to d639853 Compare March 20, 2020 18:45

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInf…

35666e1

…oApi.Provider to ProtoInfoProviderApi, and stop passing ruleContext to every method in ProtoCommon

cheister force-pushed the refactor-proto-info branch from d639853 to 35666e1 Compare March 23, 2020 18:40

lberki approved these changes Apr 1, 2020

View reviewed changes

bazel-io closed this in 1ca960c Apr 2, 2020

cheister deleted the refactor-proto-info branch April 2, 2020 07:25

This was referenced Apr 3, 2020

Feature Request: Add Starlark method to create a ProtoInfo provider #11060

Open

Add ProtoInfo constructor to Starlark #11061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

cheister commented Mar 15, 2020

cheister commented Mar 20, 2020

lberki commented Mar 20, 2020 •

edited

Loading

lberki Mar 20, 2020

cheister Mar 20, 2020

lberki Mar 23, 2020

lberki Mar 20, 2020

cheister Mar 20, 2020

lberki Mar 20, 2020

cheister Mar 20, 2020

lberki Mar 20, 2020

cheister Mar 20, 2020

lberki Mar 23, 2020

Yannic Mar 23, 2020

cheister Mar 23, 2020

cheister Mar 27, 2020

lberki Mar 20, 2020

cheister Mar 20, 2020

lberki Mar 20, 2020 •

edited

Loading

Yannic Mar 20, 2020

cheister Mar 20, 2020

Yannic Mar 20, 2020

lberki Mar 23, 2020

lberki Mar 20, 2020 •

edited

Loading

lberki Mar 20, 2020

cheister Mar 20, 2020

cheister commented Mar 25, 2020

lberki commented Apr 1, 2020

lberki commented Apr 1, 2020

lberki left a comment

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

Conversation

cheister commented Mar 15, 2020

cheister commented Mar 20, 2020

lberki commented Mar 20, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lberki Mar 20, 2020 • edited Loading

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

lberki Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheister commented Mar 25, 2020

lberki commented Apr 1, 2020

lberki commented Apr 1, 2020

lberki left a comment

Choose a reason for hiding this comment

lberki commented Mar 20, 2020 •

edited

Loading

lberki Mar 20, 2020 •

edited

Loading

lberki Mar 20, 2020 •

edited

Loading