Enhancement Request: Add support for multiple instances of the --swift_opt=ProtoPathModuleMappings=PATH flag

[sergiocampama]

Background

The --swift_opt=ProtoPathModuleMappings=PATH flag allows you to specify a file containing the mapping of proto file to Swift module that includes it, so that the module dependency graph can be represented in the generated proto sources.

One pattern we've found ourselves doing is having a multi layer dependency graph of Swift packages where each package contains protos that get generated into a module, but are also available for usage by downstream dependents. The way we do this is by adding -I=.build/checkouts/DEPENDENCY/Protos include paths in the proto generation step (sample Makefile).

When there's a need for a single mapping file, this works great, but as you add more and more dependencies, we've needed to resort to concatenating all of the mappings files before passing it in the flag.

Enhancement Request

Ideally, the generator should accept multiple instances of the ProtoPathModuleMappings option, so that clients can avoid the external concatenation command.

Created this issue to discuss this particular idea, but also to discuss the general problem of transitive dependencies and generator and see if there's been any thought in this area (i.e. outside of bazel)

[thomasvl]

What's creating your mapping files? i.e. - if something were already auto creating them, then it would seen like merging/concatenating is along the same lines, no? But yea, supporting it for that one seems doable, we already have validation for collisions/etc.

Your transitive dependencies comment is a little general, so I'm not sure what you're asking there. I think what you might be asking about isn't at all specific to SwiftProtobuf and is more about usage of protoc since it needs the imported descriptors to build the graph and then generate. You can avoid using -I directive by using protoc's support for passing desciptor sets in. i.e. - each thing can make a descriptor set of their descriptor and expose that along with the module mappings to the next thing in the chain. The the raw proto files don't have to be exposed.

[thomasvl]

ps - if we do support multiple ProtoPathModuleMappings usages, to fully use it, it might need support in other things also (gRPC).

[sergiocampama]

1. We're not automatically creating them, it's a manual edit file. Do you have examples for the generation of the modulemap files?
2. Hadn't considered the descriptor set route, will give it a thought to see whether we can leverage that. Perhaps having them generated at source generation time and checked in to the repo to be reused. But the modulemap concatenation issue is still present in that use case (given there's no dependency management for protoc in general)
3. gRPC support: not a concern at this time since we don't currently need it, but we might in the medium/long term

[thomasvl]

1. We're not automatically creating them, it's a manual edit file. Do you have examples for the generation of the modulemap files?

Only thing I'm aware of is in rules_swift. The TextFormat in this case should be pretty simple, you can likely do it in a little bit of bash to also glob out the files.

2. Hadn't considered the descriptor set route, will give it a thought to see whether we can leverage that. Perhaps having them generated at source generation time and checked in to the repo to be reused. But the modulemap concatenation issue is still present in that use case (given there's no dependency management for protoc in general)

protoc does have:

  --dependency_out=FILE       Write a dependency output file in the format
                              expected by make. This writes the transitive
                              set of input file paths to FILE

3. gRPC support: not a concern at this time since we don't currently need it, but we might in the medium/long term

Yea, just meant in general, if we do look at providing it, to make it fully useful, we'd have to make sure others take on the work since someone who needs it here might also need in there.