Support --include_source_info

[Monnoroch]

Original stackb repository supports configuring it for all rules. See java example.

This is a big issue for plugins that want to generate code preserving documentation.

[Monnoroch]

Unfortunately it's not as easy as just adding the argument:

--include_source_info only makes sense when combined with --descriptor_set_out.

[aaliddell]

These rules consume the descriptors generated by the builtin (or sourced from bazelbuild/rules_proto) proto_library rule. So, as far as I can tell, those rules are the ones that dictate whether the plugins see a descriptor with source info included and there does not appear to be an option to enable this presently.

I'm marking this issue as blocked, as I don't think there's much this repo can do without upstream support first. In theory we could regenerate descriptors ourselves from the info in the ProtoInfo provider, but then we'd be replicating all of the behaviour/work of proto_library, which I'd rather leave to them.

Do you have examples of plugins that fail with the existing behaviour?

[Monnoroch]

stackb/rules_proto also consumes native proto_library rule outputs, yet it does support source info. I'm actually trying to migrate from stackb/rules_proto to rules_proto_grpc and this is a blocker for me. In my company we have a few proto plugins that preserve documentation using source info. Perhaps you could take a look at how the original repo handles descriptors in a way that allows it?

[Monnoroch]

I believe this is the key. The original repo would not use descriptors and would pass source files directly with --descriptor_set_out --include_source_info to protoc. Since rules_proto_grpc doens't use source files and passes descriptors instead, the source info has been lost by the time you call protoc in the aspect.

Perhaps you could introduce a per-plugin config that uses source files instead of input descriptors? It would be slower, but more feature rich. It can be done by using ProtoInfo.transitive_sources instead of transitive_descriptor_sets.

[aaliddell]

Perhaps you could introduce a per-plugin config that uses source files instead of input descriptors? It would be slower, but more feature rich. It can be done by using ProtoInfo.transitive_sources instead of transitive_descriptor_sets.

That may be reasonable, although transitive_sources should probably be direct_sources, as otherwise you'd end up calling the plugin with everything in the transitive tree of proto files, which the aspect will already be handling for you.

[Monnoroch]

Right, it could get transitive descriptors from dependencies and only recompile direct sources.

Thank you for looking into this!

[Yannic]

This could be solved by bazelbuild/bazel#9337, which is about including source info in the descriptor-sets by default.

[aaliddell]

Ah excellent, I had been meaning to check if there's an issue open regarding this. Thanks for the link.

In which case, I'd rather not have to support two paths within the aspect for descriptor vs direct compilation, if this is the only use case highlighted thus far for the latter. However, I should be able to get a branch working for this as an intermediary solution, but I won't plan on merging those changes to master.

[Yannic]

Shouldn't it be sufficient to add build --protocopt=--include_source_info [1] to //.bazelrc to include the source info in the descriptors and "fix" this?

[1] https://docs.bazel.build/versions/master/command-line-reference.html#flag--protocopt

[Monnoroch]

The flag would work, but when you have a lot of protos it would slow down many plugins that don't need the source infos. This is actually why I was advocating for having the two paths -- it would be nice to support both. Global variables are not great for that. I see two ways right now:

1. Configure the proto_plugin with requires_source_info = True and have two paths in the aspect
2. Configure two proto toolchains with different --protocopt flags and somehow use different toolchains for different plugins. This one is just an idea, I'm not sure how would one make it work though

[aaliddell]

It'd be useful to have some concrete numbers on the impact of enabling source info on large codebases, as mentioned in the above issue comments. Finding a repo with stacks of proto files would be a good benchmark, perhaps https://github.com/googleapis/api-common-protos?

As I see it, we have four 'modes' that could be supported:

1. Descriptors with no source info (what we have now)
2. Descriptors with source info
3. No descriptors, all proto files passed to protoc
4. Descriptors for deps, proto files for direct sources

Modes 1 and 2 are supported today, with mode 2 requiring the protocopt flag presently but possibly being the default

One of the benefits of using descriptors is that you pay all the IO and parsing cost of reading the proto files once. On the other hand, the 'direct' modes (3 & 4) require reading every proto file for each *_proto_compile. So, if the compilation is IO or proto-parsing limited, it may actually be more beneficial to just take the hit on including the source info (globally or somehow per-proto_library).

[Monnoroch]

Possibly. Though when you add source info to the descriptor set, it basically becomes as large as the source file, so there are no IO savings, just the parsing time saving.

In any case, --protocopt should work for my current use case, this is all just thoughts about scalability to large repos.

[aaliddell]

Sorry, that wasn't too clear: I meant that the IO latency of opening, reading, parsing and closing multiple small files may be slower than the same for one big file.

[Monnoroch]

Actually the style guide says you should generally have one proto file proto_library, so the many small files vs one big file doesn't work, if I understand it correctly. There's going to be many descriptor files anyway. Unless they get merged somehow and I missed it in the aspect code?

[Monnoroch]

Just want to add that the proposed build --protocopt=--include_source_info solution makes the aspect spam with those:

--include_source_info only makes sense when combined with --descriptor_set_out.

[aaliddell]

Since the move to non-transitive mode in 3.0.0, it's now a lot easier to support this. Therefore, in the next release I'm adding a plugin option to pass the files and import paths directly to the protoc invocation (effectively mode 4 above). This allows plugins to directly see the proto files they are working on, rather than indirectly via descriptors.