Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate code file for protos with no definitions #27

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

rnro
Copy link
Collaborator

@rnro rnro commented Jan 6, 2025

This change is broken out of #26

Motivation

To prepare for use in a SwiftPM build plugin which requires deterministic output files and to match the behavior of proto-gen-swift we should generate a source file even if no definitions are found.

Modifications:

  • We no longer return early in the case of no definitions being found.
  • Add empty proto file test
  • Add a preamble to foo-messages.proto
  • Don't unconditionally add a dependency on GRPCProtobuf if there are no services
  • Add the new test case code generation to dev/protos/generate.sh
  • Depend on grpc-swift main to pick up empty file generation changes.

Result:

We will generate a Swift source file even if no definitions are found.

Motivation

To prepare for use in a SwiftPM build plugin which requires
deterministic output files and to match the behavior of
`proto-gen-swift` we should generate a source file even if no
definitions are found.

Modifications:

We no longer return early in the case of no definitions being found.

Result:

We will generate a Swift source file even if no definitions are found.
@rnro rnro requested a review from glbrntt January 6, 2025 10:09
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.

Can you add a test for this?

Obviously it'd make more sense to get grpc/grpc-swift#2151 merged first so we don't need to update the test afterwards.

You can modify dev/protos/generate.sh so that a descriptor set is generated for foo-messages.proto, once you have that it should be straightforward to add one by following other examples in GRPCProtobufCodeGenTests.

@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jan 6, 2025
* Add empty proto file test
* Add a preamble to `foo-messages.proto`
* Don't unconditionally add a dependency on `GRPCProtobuf` if there are
  no services
* Add the new test case code generation to `dev/protos/generate.sh`
* depend on `grpc-swift` `main` to pick up empty file generation
  changes.
@rnro rnro force-pushed the generate_empty_files branch from f3164b2 to 5731604 Compare January 7, 2025 13:42
@rnro rnro requested a review from glbrntt January 7, 2025 13:43
@@ -31,7 +31,7 @@ let products: [Product] = [
let dependencies: [Package.Dependency] = [
.package(
url: "https://github.com/grpc/grpc-swift.git",
exact: "2.0.0-beta.2"
branch: "main"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@glbrntt note this change. I'm happy to take a different approach than this if you'd rather.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, this is good. We only switched to exact for the tagged releases, main is fine between releases.

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.

One edge case bug but looks good otherwise

@@ -31,7 +31,7 @@ let products: [Product] = [
let dependencies: [Package.Dependency] = [
.package(
url: "https://github.com/grpc/grpc-swift.git",
exact: "2.0.0-beta.2"
branch: "main"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, this is good. We only switched to exact for the tagged releases, main is fine between releases.

Comment on lines 103 to 105
if file.services.count > 0 {
codeDependencies.append(Dependency(module: "GRPCProtobuf", accessLevel: .internal))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't quite right: we'll still generate some imports in some cases unnecessarily (i.e. if the file defines a message which depends on a type which is bundled with SwiftProtobuf). We should return the empty array if there are no services.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good point

@rnro rnro requested a review from glbrntt January 7, 2025 14:54
@glbrntt glbrntt merged commit be41136 into grpc:main Jan 7, 2025
21 checks passed
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.

2 participants