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

Document use of the build plugin #2176

Merged
merged 10 commits into from
Jan 24, 2025

Conversation

rnro
Copy link
Collaborator

@rnro rnro commented Jan 23, 2025

Motivation:

To ease use of the grpc-swift-protobuf Swift Package Manager build plugin.

Modifications:

Documentation added.

Result:

Greater understanding?

Motivation:

To ease use of the `grpc-swift-protobuf` Swift Package Manager build
plugin.

Modifications:

Documentation added.

Result:

Greater understanding?
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.

The structure and content of this is more or less what we want. I think we need to tweak a few things before we can merge it though.

Comment on lines 17 to 23
The protoc plugin can be used from the command line directly, passed to `protoc`, or
you can make use of a convenience which adds the stub generation to the Swift build graph.
The automatic gRPC Swift stub generation makes use of a [Swift Package Manager build plugin](
https://github.com/swiftlang/swift-package-manager/blob/main/Documentation/Plugins.md) to use the
`.proto` files as inputs to the build graph, input them into `protoc` using `protoc-gen-grpc-swift`
and `protoc-gen-swift` as needed, and make the resulting gRPC Swift stubs available to code
against without committing them as source. The build plugin may be invoked either from the command line or from Xcode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should pull this further up (so that it's the second paragraph) and make it more concise. We can then expand in each section. I'd like it so that the overview is like a landing page; you're giving the reader a brief introduction and then giving them a choice (manual generation with protoc vs. automated with build plugin.)

To that end it might be worth swapping the two around and having the build plugin first (progressive disclosure?).

Comment on lines 99 to 101
to be available at compile time. Also because of a limitation of Swift Package Manager build plugins, the plugin
will only be invoked when applied to the source contained in a leaf package, so it is not useful for generating code for
library authors.
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 actually true. The reason you can't use this build plugin in non-leaf packages is that it forces an implicit build time dependency on protoc which might not always be available.

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, thanks for pointing that out! I'll have to refresh my knowledge of the docs.

Comment on lines 98 to 99
Because it generates the gRPC Swift stubs as part of the build it has the requirement that `protoc` must be guaranteed
to be available at compile time. Also because of a limitation of Swift Package Manager build plugins, the plugin
Copy link
Collaborator

Choose a reason for hiding this comment

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

"must" and "guaranteed" are both requirements, let's get rid of one

Suggested change
Because it generates the gRPC Swift stubs as part of the build it has the requirement that `protoc` must be guaranteed
to be available at compile time. Also because of a limitation of Swift Package Manager build plugins, the plugin
Because it generates the gRPC Swift stubs as part of the build it has the requirement that `protoc` must be available at compile time. Also because of a limitation of Swift Package Manager build plugins, the plugin

Comment on lines 103 to 104
The build plugin will detect `.proto` files in the source tree and perform one invocation of `protoc` for each file
(caching results and performing the generation as necessary).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to write in the active voice, e.g. "The build plugin detects .proto files in the source tree and invokes protoc once for each file."

Comment on lines 107 to 109
Swift Package Manager build plugins must be adopted on a per-target basis, you can do this by modifying your
package manifest (`Package.swift` file). You will need to declare the `grpc-swift-protobuf` package as a package
dependency and then add the plugin to any desired targets.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: active voice

"You must adopt Swift Package Manager build plugins on a per-target basis by modifying your package manifest (Package.swift file). To do this, declare the grpc-swift-protobuf package as a dependency and add the plugin to your desired targets.

"messages": true,
},
"generatedSource": {
"accessLevelOnImports": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot to mention that this change never actually got made, the config is still using useAccessLevelOnImports. I can fix that up though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines 161 to 163
| `generate.servers` | `true`, `false` | `True` | Generate server stubs |
| `generate.clients` | `true`, `false` | `True` | Generate client stubs |
| `generate.messages` | `true`, `false` | `True` | Generate message stubs |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `generate.servers` | `true`, `false` | `True` | Generate server stubs |
| `generate.clients` | `true`, `false` | `True` | Generate client stubs |
| `generate.messages` | `true`, `false` | `True` | Generate message stubs |
| `generate.servers` | `true`, `false` | `true` | Generate server stubs |
| `generate.clients` | `true`, `false` | `true` | Generate client stubs |
| `generate.messages` | `true`, `false` | `true` | Generate message stubs |

| `generate.clients` | `true`, `false` | `True` | Generate client stubs |
| `generate.messages` | `true`, `false` | `True` | Generate message stubs |
| `generatedSource.accessLevelOnImports` | `true`, `false` | `false` | Whether imports should have explicit access levels |
| `generatedSource.accessLevel` | `public`, `package`, `internal` | `internal` | Access level for generated stubs |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we're describing JSON, these should be strings

Suggested change
| `generatedSource.accessLevel` | `public`, `package`, `internal` | `internal` | Access level for generated stubs |
| `generatedSource.accessLevel` | `"public"`, `"package"`, `"internal"` | `"internal"` | Access level for generated stubs |

| `generate.messages` | `true`, `false` | `True` | Generate message stubs |
| `generatedSource.accessLevelOnImports` | `true`, `false` | `false` | Whether imports should have explicit access levels |
| `generatedSource.accessLevel` | `public`, `package`, `internal` | `internal` | Access level for generated stubs |
| `protoc.executablePath` | N/A | N/A (attempted discovery) | Path to the `protoc` executable |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because it's JSON the default is null, but that has meaning which we should describe

Suggested change
| `protoc.executablePath` | N/A | N/A (attempted discovery) | Path to the `protoc` executable |
| `protoc.executablePath` | N/A | null
| Path to the `protoc` executable. Autodetected if the `null`. |

| `generatedSource.accessLevelOnImports` | `true`, `false` | `false` | Whether imports should have explicit access levels |
| `generatedSource.accessLevel` | `public`, `package`, `internal` | `internal` | Access level for generated stubs |
| `protoc.executablePath` | N/A | N/A (attempted discovery) | Path to the `protoc` executable |
| `protoc.importPaths` | N/A | Directory containing the config file | Access level for generated stubs |
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description is wrong here. Also like above we put the default as null and explain that the directory of the config file is used if null.

@glbrntt glbrntt added the semver/none No version bump required. label Jan 24, 2025
@rnro rnro force-pushed the grpc-swift-protobuf_plugin_docs branch from 1356950 to 9e70d5c Compare January 24, 2025 11:28
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.

A couple of formatting nits aside this looks great!

Comment on lines 11 to 12
The [`grpc-swift-protobuf`](https://github.com/grpc/grpc-swift-protobuf) package provides
`protoc-gen-grpc-swift`, a program which is a plugin for the Protocol Buffers compiler, `protoc`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be in the first paragraph in ### Using protoc section

Comment on lines 19 to 21

> `protoc-gen-grpc-swift` only generates gRPC stubs, it doesn't generate messages. You must use
> `protoc-gen-swift` to generate messages in addition to gRPC Stubs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be in the second paragraph in ### Using protoc section


You must provide a configuration file in the directory which encloses all .proto files (in the same directory or a parent).
Configuration files, written in JSON, tell the build plugin about the options used for protoc invocations.
You must name the file grpc-swift-proto-generator-config.json and structure it in the following format:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You must name the file grpc-swift-proto-generator-config.json and structure it in the following format:
You must name the file `grpc-swift-proto-generator-config.json` and structure it in the following format:

@rnro rnro requested a review from glbrntt January 24, 2025 13:33
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.

Looks great, thanks @rnro!

@glbrntt glbrntt enabled auto-merge (squash) January 24, 2025 13:44
@glbrntt glbrntt merged commit 5f21fc9 into grpc:main Jan 24, 2025
27 of 29 checks passed
@rnro rnro deleted the grpc-swift-protobuf_plugin_docs branch January 24, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants