Skip to content

Conversation

@jagobagascon
Copy link
Contributor

@jagobagascon jagobagascon commented May 14, 2020

When generating a file using the option ProtoPathModuleMappings the module of the generated proto is being added to the header of the file.

swift mappings file:

mapping {
    module_name: "A"
    proto_file_path: "a.proto"
}
mapping {
    module_name: "B"
    proto_file_path: "b.proto"
}

a.grpc.swift (generated from a.proto that depends on b.proto)

...
import A // this is not needed
import B
...

@jagobagascon jagobagascon changed the title do not add own imports in generated code do not add own module import in generated code May 14, 2020
// mappings to import the service protos as well as and any other proto modules that the file
// imports.
let moduleMappings = options.protoToModuleMappings
if let serviceProtoModuleName = moduleMappings.moduleName(forFile: file) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @allevato who added this originally (#347)

My understanding of the comment above is that if we have a foo.proto containing:

service FooService { ... }

message FooMessage { ... }

Then Bazel would produce two Swift modules, say FooServices and FooMessages; and this line is to import the FooMessages module.

If my understanding of this is correct this seems like a misuse of module mappings since the output generated from a single proto is split across multiple modules. Using it in this way also leads to an unnecessary warning if you want to use module mappings in the intended way. Can this be better expressed in the Bazel rules as an additional import instead of module mappings?

Copy link
Contributor

@allevato allevato May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct about the original motivation; the Bazel rule swift_proto_library generates just the code for the messages, the rule swift_grpc_library generates just the code for the services, and they're in separate modules so the latter needs to import the former.

The ExtraModuleImports flag was added in #402 (after my original change) to support getting the right imports for client stubs, so that wasn't an option when we first spun up Bazel support. But looking at the rule implementation again (it's been a while), I think it should be a straightforward change to use ExtraModuleImports in the same way here, so I don't foresee a problem with this change; we'll just need to update the rules when we update to this version.

cc @mpetrov for awareness as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks very much.

As a heads up: this is change is for protoc-gen-grpc-swift (gRPC Swift 1.x); protoc-gen-swiftgrpc (gRPC Swift 0.x) won't be affected.

@glbrntt glbrntt self-requested a review May 14, 2020 10:45
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jagobagascon!

@glbrntt glbrntt merged commit db6ae11 into grpc:master May 14, 2020
@jagobagascon jagobagascon deleted the fix/do-not-add-own-module-on-generated-code branch May 15, 2020 08:22
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants