Code generation command plugin #40

rnro · 2025-01-30T07:30:34Z

Motivation:

To make it simpler to generate gRPC stubs with protoc-gen-grpc-swift and protoc-gen-swift.

Modifications:

Add a new command plugin
Refactor some errors

The command plugin can be invoked from the CLI as:

swift package generate-grpc-code-from-protos --import-path /path/to/Protos -- /path/to/Protos/HelloWorld.proto

The plugin has flexible configuration:

❯ swift package generate-grpc-code-from-protos --help
Usage: swift package generate-grpc-code-from-protos [flags] [--] [input files]

Flags:

  --servers                   Indicate that server code is to be generated. Generated by default.
  --no-servers                Indicate that server code is not to be generated. Generated by default.
  --clients                   Indicate that client code is to be generated. Generated by default.
  --no-clients                Indicate that client code is not to be generated. Generated by default.
  --messages                  Indicate that message code is to be generated. Generated by default.
  --no-messages               Indicate that message code is not to be generated. Generated by default.
  --file-naming               The naming scheme for output files [fullPath/pathToUnderscores/dropPath]. Defaults to fullPath.
  --access-level              The access level of the generated source [internal/public/package]. Defaults to internal.
  --access-level-on-imports   Whether imports should have explicit access levels. Defaults to false.
  --import-path               The directory in which to search for imports.
  --protoc-path               The path to the protoc binary.
  --output-path               The path into which the generated source files are created.
  --verbose                   Emit verbose output.
  --dry-run                   Print but do not execute the protoc commands.
  --help                      Print this help.

When executing, the command prints the protoc invocations it uses for ease of debugging. The --dry-run flag can be supplied for this purpose or so that they may be extracted and used separately e.g. in a script.
If no protoc path is supplied then Swift Package Manager will attempt to locate it.
If no output directory is supplied then generated files are placed a Swift Package Manager build directory.

Result:

More convenient code generation

This PR is split out of #26

Motivation: To make it simpler to generate gRPC stubs with `protoc-gen-grpc-swift` and `protoc-gen-swift`. Modifications: * Add a new command plugin `swift package generate-grpc-code-from-protos/path/to/Protos/HelloWorld.proto --import-path /path/to/Protos` * Refactor some errors Result: More convenient code generation

glbrntt

This looks good so far! Most of the comments are around the UX.

glbrntt · 2025-02-03T15:06:51Z

Package.swift

+      ]
+    ),
+    dependencies: [
+      "protoc-gen-grpc-swift",


Suggested change

"protoc-gen-grpc-swift",

.target(name: "protoc-gen-grpc-swift"),

glbrntt · 2025-02-03T15:07:23Z

Package.swift

+
+  // Code generator SwiftPM command
+  .plugin(
+    name: "GRPCGeneratorCommand",


We should include "Protobuf" in the name. I find "Command" a weird suffix but that stems more from me finding the "command plugin" naming weird as well.

Suggested change

name: "GRPCGeneratorCommand",

name: "GRPCProtobufGeneratorCommand",

glbrntt · 2025-02-03T15:10:18Z

Plugins/GRPCGeneratorCommand/CommandConfig.swift

+struct CommandConfig {
+  var common: GenerationConfig
+
+  var dryRun: Bool


Love this, great idea!

glbrntt · 2025-02-03T15:12:46Z

Plugins/GRPCGeneratorCommand/CommandConfig.swift

+          case "package":
+            config.common.accessLevel = .`package`
+          default:
+            Diagnostics.error("Unknown accessLevel \(value)")


We should parrot out the name of the argument as passed by the user ("--access-level") rather than the property name.

glbrntt · 2025-02-03T15:15:20Z

Plugins/GRPCGeneratorCommand/CommandConfig.swift

+      switch flag {
+      case .accessLevel:
+        let accessLevel = argExtractor.extractOption(named: flag.rawValue)
+        if let value = accessLevel.first {


If there's more than one value we should either warn or throw an error.

(This feedback applies to all of these options.)

glbrntt · 2025-02-03T15:50:16Z

Plugins/GRPCGeneratorCommand/Plugin.swift

+    let inputFileURLs = inputFiles.map { URL(fileURLWithPath: $0) }
+
+    // MARK: protoc-gen-grpc-swift
+    if config.clients != false || config.servers != false {


These should be non-optional now so can be more idiomatic:

Suggested change

if config.clients != false || config.servers != false {

if config.clients || config.servers {

glbrntt · 2025-02-03T15:52:20Z

Plugins/GRPCGeneratorCommand/Plugin.swift

+    }
+
+    // MARK: protoc-gen-swift
+    if config.messages != false {


Suggested change

if config.messages != false {

if config.messages {

glbrntt · 2025-02-03T15:55:00Z

Plugins/GRPCGeneratorCommand/Plugin.swift

+///   - arguments: The arguments to be passed to `protoc`.
+func printProtocInvocation(_ executableURL: URL, _ arguments: [String]) {
+  print("protoc invocation:")
+  print("  \(executableURL.absoluteStringNoScheme) \\")


Suggested change

print(" \(executableURL.absoluteStringNoScheme) \\")

print("\(executableURL.absoluteStringNoScheme) \\")

glbrntt · 2025-02-03T15:59:25Z

Plugins/GRPCGeneratorCommand/Plugin.swift

+///   - executableURL: The path to the `protoc` executable.
+///   - arguments: The arguments to be passed to `protoc`.
+func printProtocInvocation(_ executableURL: URL, _ arguments: [String]) {
+  print("protoc invocation:")


Not sure this is necessary; protoc ... will make it obvious.

glbrntt · 2025-02-03T16:01:11Z

Plugins/GRPCGeneratorCommand/Plugin.swift

+func printProtocInvocation(_ executableURL: URL, _ arguments: [String]) {
+  print("protoc invocation:")
+  print("  \(executableURL.absoluteStringNoScheme) \\")
+  for argument in arguments[..<arguments.count.advanced(by: -1)] {


Can't you just do print(arguments.joined(separator: " \\\n "))?

glbrntt · 2025-02-25T11:06:31Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+  static func helpRequested(
+    argumentExtractor: inout ArgumentExtractor
+  ) -> Bool {
+    let help = argumentExtractor.extractFlag(named: OptionsAndFlags.help.rawValue)
+    return help != 0
+  }


This function is tiny and only used one time so I think the indirection hurts it.

You'd be better off just inlining it:

if argumentExtractor.extractFlag(named: OptionsAndFlags.help.rawValue) > 0 { // ... }

glbrntt · 2025-02-25T11:12:28Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+        let servers = argExtractor.extractFlag(named: OptionsAndFlags.servers.rawValue)
+        let noServers = argExtractor.extractFlag(named: OptionsAndFlags.noServers.rawValue)
+        if noServers > servers {
+          config.common.servers = false
+        }


I think if the user specifies both flags then they've made a mistake and we should emit a diagnostic and fail. It should be okay to specify the same flag more than once though.

Same comment for clients and messages.

glbrntt · 2025-02-25T11:18:47Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+        }
+
+      case .servers, .noServers:
+        if flag == .noServers { continue }  // only process this once


This threw me for a second because I expected to be looping over the arguments and checking for known flags/options as opposed to looping over the known flags/options and extracting them (as that requires scanning the args multiple times) although this is how the arg extractor wants you to do it.

Very minor but I think it'd be more obvious if they were treated as separate cases:

case .noServers: // Handled by `.servers` continue

glbrntt · 2025-02-25T11:23:24Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+        let accessLevelOnImports = argExtractor.extractOption(named: flag.rawValue)
+        if let value = extractSingleValue(flag, values: accessLevelOnImports) {
+          guard let accessLevelOnImports = Bool(value) else {
+            throw CommandPluginError.invalidArgumentValue(name: flag.rawValue, value: value)


When do we use a diagnostic vs an error?

The idea was to try to be permissive and just limp on but that got a bit muddied so I've reversed that now and I just throw an error if there's an issue.

Great, I think that's the better approach.

glbrntt · 2025-02-25T11:24:38Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+
+func extractSingleValue(_ flag: OptionsAndFlags, values: [String]) -> String? {
+  if values.count > 1 {
+    Stderr.print(


Is there a warning diagnostic?

There is, I've used it now :)

glbrntt · 2025-02-25T11:25:24Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+  }
+}
+
+func extractSingleValue(_ flag: OptionsAndFlags, values: [String]) -> String? {


This seems like an appropriate extension to have on arg extractor

glbrntt · 2025-02-25T11:30:02Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+    if argExtractor.remainingArguments.isEmpty {
+      throw CommandPluginError.missingInputFile
+    }
+
+    for argument in argExtractor.remainingArguments {
+      if argument.hasPrefix("--") {
+        throw CommandPluginError.unknownOption(argument)
+      }
+    }
+
+    return (config, argExtractor.remainingArguments)


This setup is such that we allow inputs to be interspersed with options:

--clients foo.proto -verbose --servers bar.proto --dry-run

That's a bit unexpected for a CLI and you can end up with a mistyped flag/option as an input (e.g. -verbose). I think the easiest thing to do is have args after a "--" and then split the args on that, everything before goes to the extractor, everything after is an input (everything is input if there's no "--"):

--clients -verbose --servers --dry-run -- foo.proto bar.proto

glbrntt · 2025-02-25T14:31:48Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+  case accessLevelOnImports = "access-level-on-imports"
+  case importPath = "import-path"
+  case protocPath = "protoc-path"
+  case output


Should this be output-path? (We have import path and protoc path 🤷‍♂️)

glbrntt · 2025-02-25T14:33:22Z

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

+
+@main
+struct GRPCProtobufGeneratorCommandPlugin: CommandPlugin {
+  /// Perform command, the entry-point when using a Package manifest.


when using a package manifest? Not sure I understand this one.

Ah, I forgot to implement the path which applies to Xcode projects XcodeCommandPlugin.

That wasn't what I had in mind but I now understand what the comment means. I don't think it's accurate though: you can have a package manifest and still use an Xcode project. I think the comment really means the entry point when invoked from the command line via swift package something something?

Is that what it means? How do you invoke a command plugin via Xcode? Or rather how do you invoke the Xcode flavor?

Yes, I believe so. IIRC you right click on a package in the project navigator and any commands are in there.

You don't get a choice of flavour: you can only invoke the Xcode flavour using that UI.

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

glbrntt · 2025-03-03T15:00:10Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+        let accessLevelOnImports = argExtractor.extractOption(named: flag.rawValue)
+        if let value = extractSingleValue(flag, values: accessLevelOnImports) {
+          guard let accessLevelOnImports = Bool(value) else {
+            throw CommandPluginError.invalidArgumentValue(name: flag.rawValue, value: value)


Great, I think that's the better approach.

glbrntt · 2025-03-03T15:12:30Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+  func usageDescription() -> String {
+    switch self {
+    case .servers:
+      return "Indicate that server code is to be generated. Generated by default."


nit: brevity is important, if you can say it as clearly in fewer words then you should. I think "Generate server code" says the same as "Indicate that server code is to be generated"

glbrntt · 2025-03-03T15:13:45Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+    case .accessLevelOnImports:
+      return "Whether imports should have explicit access levels. Defaults to false."
+    case .importPath:
+      return "The directory in which to search for imports."


Worth noting that it can be specified multiple times and IIUC the current working directory is used if not specified?

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

glbrntt · 2025-03-03T15:37:41Z

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

+            Stderr.print("Generated gRPC Swift files for \(inputFiles.joined(separator: ", ")).")
+          }
+        } else {
+          let problem = "\(process.terminationReason):\(process.terminationStatus)"


This is presumably meant to be part of the error?

It is, yeah, I've done some restructuring of the protoc output handling in general which has included this line

glbrntt · 2025-03-03T15:38:14Z

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

+            "Generated protobuf message Swift files for \(inputFiles.joined(separator: ", "))."
+          )
+        } else {
+          let problem = "\(process.terminationReason):\(process.terminationStatus)"


Same here, is this meant to be part of the error?

glbrntt · 2025-03-03T15:42:20Z

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

+
+@main
+struct GRPCProtobufGeneratorCommandPlugin: CommandPlugin {
+  /// Perform command, the entry-point when using a Package manifest.


That wasn't what I had in mind but I now understand what the comment means. I don't think it's accurate though: you can have a package manifest and still use an Xcode project. I think the comment really means the entry point when invoked from the command line via swift package something something?

glbrntt · 2025-03-03T15:42:51Z

Plugins/GRPCProtobufGeneratorCommand/CommandPluginError.swift

+  var description: String {
+    switch self {
+    case .missingArgumentValue:
+      "Provided option does not have a value."


Which option?

It turns out this was left over from an earlier era. I have removed it now.

glbrntt · 2025-03-03T15:46:55Z

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

+        }
+
+      case .accessLevelOnImports:
+        if let value = argExtractor.extractSingleOption(named: flag.rawValue) {


Totally missed this last time (sorry): this is a boolean option ... which is otherwise known as a flag. Let's just make it a flag (no need for the --no version as the default isn't enabled).

Co-authored-by: George Barnett <gbarnett@apple.com>

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

glbrntt · 2025-03-04T15:22:46Z

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

+
+  try printProtocOutput(outputPipe, verbose: verbose)
+
+  guard process.terminationReason == .exit && process.terminationStatus == 0 else {


Sorry, this is a massive pet-peeve of mine: guard is great for early termination so that the rest of your function doesn't have an extra level of nesting.

Here though, there isn't really an early exit: there's a bunch of code in the else early-exit path and none in the guard path.

I would much rather we either invert this and exit early because the process exited cleanly i.e. if process.terminationReason == .exit && process.terminationStatus == 0 { return } as this then removes the nesting from the large error-handling block.

👍 I agree with you - I think this snuck in when I outlined a method and didn’t refactor the guard

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

glbrntt · 2025-03-04T15:30:59Z

Plugins/GRPCProtobufGeneratorCommand/CommandPluginError.swift

+    executable: String?,
+    arguments: [String]?,


Aren't these always non-nil?

They are, yeah, I'll make them non-optionals.

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

Co-authored-by: George Barnett <gbarnett@apple.com>

glbrntt

LGTM, thanks @rnro!

### Motivation: To make it simpler to generate gRPC stubs with `protoc-gen-grpc-swift` and `protoc-gen-swift`. ### Modifications: * Add a new command plugin * Refactor some errors The command plugin can be invoked from the CLI as: ``` swift package generate-grpc-code-from-protos --import-path /path/to/Protos -- /path/to/Protos/HelloWorld.proto ``` The plugin has flexible configuration: ``` ❯ swift package generate-grpc-code-from-protos --help Usage: swift package generate-grpc-code-from-protos [flags] [--] [input files] Flags: --servers Indicate that server code is to be generated. Generated by default. --no-servers Indicate that server code is not to be generated. Generated by default. --clients Indicate that client code is to be generated. Generated by default. --no-clients Indicate that client code is not to be generated. Generated by default. --messages Indicate that message code is to be generated. Generated by default. --no-messages Indicate that message code is not to be generated. Generated by default. --file-naming The naming scheme for output files [fullPath/pathToUnderscores/dropPath]. Defaults to fullPath. --access-level The access level of the generated source [internal/public/package]. Defaults to internal. --access-level-on-imports Whether imports should have explicit access levels. Defaults to false. --import-path The directory in which to search for imports. --protoc-path The path to the protoc binary. --output-path The path into which the generated source files are created. --verbose Emit verbose output. --dry-run Print but do not execute the protoc commands. --help Print this help. ``` * When executing, the command prints the `protoc` invocations it uses for ease of debugging. The `--dry-run` flag can be supplied for this purpose or so that they may be extracted and used separately e.g. in a script. * If no `protoc` path is supplied then Swift Package Manager will attempt to locate it. * If no `output` directory is supplied then generated files are placed a Swift Package Manager build directory. ### Result: More convenient code generation This PR is split out of grpc#26 --------- Co-authored-by: George Barnett <gbarnett@apple.com>

rnro force-pushed the command_plugin branch from 2a2f966 to 6d58cd4 Compare January 30, 2025 08:17

rnro added the 🔨 semver/patch label Jan 30, 2025

rnro requested a review from glbrntt January 30, 2025 09:04

glbrntt requested changes Feb 3, 2025

View reviewed changes

rnro added 2 commits February 17, 2025 15:35

review comments

Loading
Loading status checks…

e025438

Merge branch 'main' into command_plugin

Loading
Loading status checks…

68a2eb2

rnro requested a review from glbrntt February 17, 2025 15:39

rnro added 4 commits February 17, 2025 15:57

formatter

Loading
Loading status checks…

6c42243

simplify OptionsAndFlags

Loading
Loading status checks…

5819616

Merge branch 'main' into command_plugin

Loading
Loading status checks…

efb4100

fix merge

Loading
Loading status checks…

9536932

rnro force-pushed the command_plugin branch from fc59cb2 to 9536932 Compare February 25, 2025 11:14

glbrntt requested changes Feb 25, 2025

View reviewed changes

review comments

Loading
Loading status checks…

bdb7235

rnro requested a review from glbrntt March 3, 2025 11:48

formatting

Loading
Loading status checks…

96da879

glbrntt requested changes Mar 3, 2025

View reviewed changes

rnro and others added 5 commits March 3, 2025 16:08

Update Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

Loading
Loading status checks…

66261e7

Co-authored-by: George Barnett <gbarnett@apple.com>

Update Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

Loading
Loading status checks…

3e8afb4

Co-authored-by: George Barnett <gbarnett@apple.com>

protoc output handling & review comments

Loading
Loading status checks…

d8dd2b6

Merge branch 'main' into command_plugin

Loading
Loading status checks…

d95d00a

remove entry point comments

Loading
Loading status checks…

f088e0a

rnro requested a review from glbrntt March 4, 2025 15:16

glbrntt requested changes Mar 4, 2025

View reviewed changes

rnro and others added 5 commits March 4, 2025 15:41

Update Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

Loading
Loading status checks…

d657e93

Co-authored-by: George Barnett <gbarnett@apple.com>

Update Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

Loading
Loading status checks…

4dd7da2

Co-authored-by: George Barnett <gbarnett@apple.com>

Update Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

Loading
Loading status checks…

b87b3e5

Co-authored-by: George Barnett <gbarnett@apple.com>

review comments

Loading
Loading status checks…

1da570e

Merge branch 'main' into command_plugin

Loading
Loading status checks…

16d19d8

rnro requested a review from glbrntt March 5, 2025 10:15

glbrntt approved these changes Mar 5, 2025

View reviewed changes

glbrntt added 🆕 semver/minor and removed 🔨 semver/patch labels Mar 5, 2025

glbrntt enabled auto-merge (squash) March 5, 2025 10:35

glbrntt merged commit 8726dda into grpc:main Mar 5, 2025
27 checks passed

rnro deleted the command_plugin branch March 5, 2025 11:45

	"protoc-gen-grpc-swift",
	.target(name: "protoc-gen-grpc-swift"),

	name: "GRPCGeneratorCommand",
	name: "GRPCProtobufGeneratorCommand",

	if config.clients != false \|\| config.servers != false {
	if config.clients \|\| config.servers {

	print(" \(executableURL.absoluteStringNoScheme) \\")
	print("\(executableURL.absoluteStringNoScheme) \\")


		try printProtocOutput(outputPipe, verbose: verbose)

		guard process.terminationReason == .exit && process.terminationStatus == 0 else {

Code generation command plugin #40

Code generation command plugin #40

Conversation

rnro commented Jan 30, 2025 • edited Loading

Motivation:

Modifications:

Result:

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

rnro commented Jan 30, 2025 •

edited

Loading

rnro Mar 3, 2025 •

edited

Loading

rnro Mar 3, 2025 •

edited

Loading