Code generation build plugin #28

rnro · 2025-01-08T13:53:57Z

Overview

New build plugin to generate gRPC services and protobuf messages

The SwiftPM build plugin will locate protobuf files in the Sources directory (with the extension .proto) and attempt to invoke both the protoc-gen-swift and protoc-gen-grpc-swift protoc plugins on them to automatically generate Swift source. Behavior can be modified by specifying one or more configuration files (named grpc-swift-config.json).

Configuration

The configuration file is JSON which may contain the following entries.

/// The configuration of the plugin.
struct ConfigurationFile: Codable {
  enum Visibility: String, Codable {            /// The visibility of the generated files.
    case `internal`                             /// The generated files should have `internal` access level.
    case `public`                               /// The generated files should have `public` access level.
    case `package`                              /// The generated files should have `package` access level.
  }

  var visibility: Visibility?                   /// The visibility of the generated files.
  var server: Bool?                             /// Whether server code is generated.
  var client: Bool?                             /// Whether client code is generated.
  var message: Bool?                            /// Whether message code is generated.
  var protoPathModuleMappings: String?          /// Path to module map .asciipb file.
  var useAccessLevelOnImports: Bool?            /// Whether imports should have explicit access levels.

  /// Specify the directory in which to search for
  /// imports. May be specified multiple times;
  /// directories will be searched in order.
  /// The target source directory is always appended
  /// to the import paths.
  var importPaths: [String]?

  var protocPath: String?                       /// The path to the `protoc` binary. If this is not set, SPM will try to find the tool itself.
}

For a given protobuf definition file the tool will search for configuration files in the same and all parent directories and will use the file lowest in the hierarchy.
Most configuration if not specified will use the protoc-plugin's own defaults.

Adoption

Users must add the plugin to any target which wishes to make use of it

  targets: [
    .executableTarget(
      name: "plugin-adopter",
      dependencies: [
        // ...
      ],
      plugins: [
        .plugin(name: "GRPCGeneratorPlugin", package: "grpc-swift-protobuf")
      ]
    )
  ]

This PR is split out of #26

Motivation: To make code generation more convenient for adopters. Modifications: * New build plugin to generate gRPC services and protobuf messages Result: * Users will be able to make use of the build plugin.

glbrntt · 2025-01-08T14:18:02Z

Package.swift

+    name: "GRPCGeneratorPlugin",
+    targets: ["GRPCGeneratorPlugin"]


Having plugin in the name is weird. I think we also want to nod to it being the generator for protobuf, so can we call it GRPCProtobufGenerator?

glbrntt · 2025-01-08T14:18:42Z

Package.swift

+    name: "GRPCGeneratorPlugin",
+    capability: .buildTool(),
+    dependencies: [
+      "protoc-gen-grpc-swift",


nit, for consistency:

Suggested change

"protoc-gen-grpc-swift",

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

Plugins/GRPCGeneratorPlugin/ConfigurationFile.swift

glbrntt · 2025-01-08T14:21:08Z

Plugins/GRPCGeneratorPlugin/ConfigurationFile.swift

+ */
+
+/// The configuration of the plugin.
+struct ConfigurationFile: Codable {


nit for consistency: gRPC uses "config" instead of "configuration".

"File" is also out of place here; we should nod to its use for generation here instead. "GeneratorConfig"? "PluginConfig"? "Config"?

So "File" is in the name because it corresponds specifically to the format of the configuration file on disk which is why it is Codable, there is an upcoming structure which knows about configuration which is passed as flags and the common configuration representation is already in this PR. I'm not sure how that fits into your existing naming schemes.

It being on disk is incidental though. The name should indicate what it's config for not where the config comes from.

If plugins were (much) more flexible you could imagine getting the config from e.g. a config service. Using the name ConfigFile for that type would be weird!

glbrntt · 2025-01-08T14:21:28Z

Plugins/GRPCGeneratorPlugin/ConfigurationFile.swift

+  //  /// Whether reflection data is generated.
+  //  var reflectionData: Bool?


We can get rid of this as it's commented out

We can if you'd prefer - I didn't know how imminent the use of reflection data would be

glbrntt · 2025-01-08T15:04:35Z

Plugins/GRPCGeneratorPlugin/Plugin.swift

+  )
+
+  return Command.buildCommand(
+    displayName: "Generating protobuf Swift files for \(inputFile.relativePath)",


Suggested change

displayName: "Generating protobuf Swift files for \(inputFile.relativePath)",

displayName: "Generating Swift Protobuf files for \(inputFile.relativePath)",

glbrntt · 2025-01-08T15:05:53Z

Plugins/PluginsShared/PluginUtils.swift

+  // TODO: Don't currently support implementation only imports
+  //  // Add the implementation only imports flag if it was set
+  //  if let implementationOnlyImports = config.implementationOnlyImports {
+  //      protocArgs.append("--swift_opt=ImplementationOnlyImports=\(implementationOnlyImports)")
+  //  }


Let's remove this

glbrntt · 2025-01-08T15:09:21Z

Plugins/PluginsShared/PluginUtils.swift

+
+  if let importPaths = config.importPaths {
+    for path in importPaths {
+      protocArgs.append("-I")


-I is a synonym for --proto_path which we also set below, why do we have both and have two source of values for them? Also, the protoc-gen-swift args only have --proto_path it seems. Somethin' ain't right here!

The two sources of values is because one is user-specified and one is a default one which we add based on the path to the proto file we're currently processing. I've made them both use --proto_path and fixed up the missing source.

glbrntt · 2025-01-08T15:09:29Z

Plugins/PluginsShared/PluginUtils.swift

+  // TODO: Don't currently support reflection data
+  //  if let generateReflectionData = config.reflectionData {
+  //    protocArgs.append("--grpc-swift_opt=ReflectionData=\(generateReflectionData)")
+  //  }


Let's remove this

glbrntt · 2025-01-08T15:11:51Z

Plugins/PluginsShared/PluginUtils.swift

+///   - outputDirectory: The directory in which generated source files are created.
+///   - outputExtension: The file extension to be appended to generated files in-place of `.proto`.
+/// - Returns: The expected output file path.
+func deriveOutputFilePath(


IIRC the output file path is only needed by the build plugin which can only use the .fullPath file naming. If so we can simplify this, right?

glbrntt · 2025-01-14T15:16:57Z

Plugins/GRPCProtobufGenerator/BuildPluginConfig.swift

+  let defaultVisibility: GenerationConfig.Visibility = .internal
+  let defaultServer = true
+  let defaultClient = true
+  let defaultMessage = true
+  let defaultUseAccessLevelOnImports = false
+  let defaultImportPaths: [String] = []


We don't want to carry around the defaults on each instance, we should just have a static instance:

static let defaults = Self(...)

glbrntt · 2025-01-14T15:18:12Z

Plugins/GRPCProtobufGenerator/BuildPluginConfig.swift

+}
+
+extension GenerationConfig {
+  init(configurationFile: BuildPluginConfig, configurationFilePath: URL, outputPath: URL) {


nit: s/configuration/config

glbrntt · 2025-01-14T15:18:55Z

Plugins/GRPCProtobufGenerator/BuildPluginConfig.swift

+    // hard-code full-path to avoid collisions since this goes into a temporary directory anyway
+    self.fileNaming = .fullPath
+    self.useAccessLevelOnImports = configurationFile.useAccessLevelOnImports
+    self.importPaths = []


We set this below

glbrntt · 2025-01-14T15:27:37Z

Plugins/GRPCProtobufGenerator/BuildPluginConfig.swift

+    // Generate absolute paths for the imports relative to the config file in which they are specified
+    self.importPaths = configurationFile.importPaths.map { relativePath in
+      configurationFilePath.deletingLastPathComponent().relativePath + "/" + relativePath


This doesn't tie together: the comment says we're generating absolute paths relative to the config file, but the implementation uses the relative path of the config file?

Plugins/GRPCProtobufGenerator/Plugin.swift

glbrntt · 2025-01-14T16:19:07Z

Plugins/PluginsShared/GenerationConfig.swift

+  /// Whether client code is generated.
+  var client: Bool
+  /// Whether message code is generated.
+  var message: Bool


nit: wondering whether this should be messages? WDYT?

glbrntt · 2025-01-14T16:21:13Z

Plugins/PluginsShared/PluginError.swift

+    case .incompatibleTarget(let string):
+      "Build plugin applied to incompatible target."
+    case .noConfigurationFilesFound:
+      "No configuration files found."


Can you expand on this to say that there needs to be a file called "blah" somewhere?

glbrntt · 2025-01-14T16:22:55Z

Plugins/PluginsShared/PluginUtils.swift

+/// - Returns: The constructed arguments to be passed to `protoc` when invoking the `proto-gen-swift` `protoc` plugin.
+func constructProtocGenSwiftArguments(
+  config: GenerationConfig,
+  using fileNaming: GenerationConfig.FileNaming?,


using in the context of constructing args feels way outta place. Let's just use fileNaming here

glbrntt · 2025-01-14T16:23:49Z

Plugins/PluginsShared/PluginUtils.swift

+    protocArgs.append("--proto_path")
+    protocArgs.append("\(path)")


Does this work? The docs say -IPATH, --proto_path=PATH so it looks like you need an = here.. 🤷‍♂️

Empirically, yes it does seem to work.

glbrntt · 2025-01-14T16:24:36Z

Plugins/PluginsShared/PluginUtils.swift

+  protocArgs.append("--swift_opt=Visibility=\(config.visibility.rawValue)")
+  protocArgs.append("--swift_opt=FileNaming=\(config.fileNaming.rawValue)")
+  protocArgs.append("--swift_opt=UseAccessLevelOnImports=\(config.useAccessLevelOnImports)")
+  protocArgs.append(contentsOf: protoDirectoryPaths.map { "--proto_path=\($0.relativePath)" })


I don't think we should set this and use the config paths, we should do one or the other.

I'm not sure I agree, one is optional and comes from user configuration and one is determined by the plugin and is required.

The value provided by the plugin is a sensible default. Explicitly setting paths in the config should replace that default value.

Using both is quite surprising behaviour IMO.

Including the directory which contains the source file is I think more than just a sensible default, it's required by protoc. If you don't specify it you get this:

File does not reside within any path specified using --proto_path (or -I). You must specify a --proto_path which encompasses this file. Note that the proto_path must be an exact prefix of the .proto file names -- protoc is too dumb to figure out when two paths (e.g. absolute and relative) are equivalent (it's harder than you think).

I think it is helpful for us to add this to avoid users having to enumerate every directory containing source files.

Moving this from being a parameter to explicitly being derived at this level from the input proto files might make sense to clarify what's happening.

I think we're talking about different things. I agree that we should use a sensible value if the user doesn't express an opinion about which paths to use.

What I'm arguing is that if the user expresses an opinion then the plugin shouldn't have an opinion: it should use the paths specified by the user and nothing else.

For what it's worth protoc doesn't require --proto_path/-I to be set; it uses the current working directory. We obviously can't rely on that for the plugin so we do need a default value.

I'm not sure that the directory of the source file is the best choice though: the directory of the config file is likely to be a better choice as it will include the input proto file and likely any protos depended on by the input proto file.

Co-authored-by: George Barnett <[email protected]>

Code generation build plugin

a5010cb

Motivation: To make code generation more convenient for adopters. Modifications: * New build plugin to generate gRPC services and protobuf messages Result: * Users will be able to make use of the build plugin.

rnro added the 🆕 semver/minor Adds new public API. label Jan 8, 2025

rnro requested review from glbrntt January 8, 2025 13:56

glbrntt requested changes Jan 8, 2025

View reviewed changes

rnro force-pushed the build_plugin branch from 84627d2 to 5deb609 Compare January 14, 2025 14:50

review comments

af7cfe0

rnro force-pushed the build_plugin branch from 5deb609 to af7cfe0 Compare January 14, 2025 15:03

glbrntt requested changes Jan 14, 2025

View reviewed changes

rnro and others added 4 commits January 14, 2025 16:28

add config file as a registered input file

256b3e9

Update Plugins/GRPCProtobufGenerator/Plugin.swift

1749e56

Co-authored-by: George Barnett <[email protected]>

Update Plugins/GRPCProtobufGenerator/Plugin.swift

87197c6

Co-authored-by: George Barnett <[email protected]>

review comments, rethink config & imports

585e856

rnro force-pushed the build_plugin branch from 13ddcf3 to 585e856 Compare January 17, 2025 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code generation build plugin #28

Code generation build plugin #28

rnro commented Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

rnro Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

rnro Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

rnro Jan 14, 2025

glbrntt Jan 8, 2025

glbrntt Jan 8, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

glbrntt Jan 14, 2025

rnro Jan 14, 2025

glbrntt Jan 14, 2025

rnro Jan 14, 2025

glbrntt Jan 15, 2025

rnro Jan 15, 2025

glbrntt Jan 15, 2025

		name: "GRPCGeneratorPlugin",
		targets: ["GRPCGeneratorPlugin"]

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

		// /// Whether reflection data is generated.
		// var reflectionData: Bool?

	displayName: "Generating protobuf Swift files for \(inputFile.relativePath)",
	displayName: "Generating Swift Protobuf files for \(inputFile.relativePath)",

		protocArgs.append("--proto_path")
		protocArgs.append("\(path)")

Code generation build plugin #28

Are you sure you want to change the base?

Code generation build plugin #28

Conversation

rnro commented Jan 8, 2025

Overview

Configuration

Adoption

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

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