protoc output handling & review comments

Author: rnro

Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

@@ -37,6 +37,8 @@ struct CommandConfig {
     verbose: false,
     dryRun: false
   )
+
+  static let parameterGroupSeparator = "--"
 }
 
 extension CommandConfig {
@@ -118,11 +120,8 @@ extension CommandConfig {
         }
 
       case .accessLevelOnImports:
-        if let value = argExtractor.extractSingleOption(named: flag.rawValue) {
-          guard let accessLevelOnImports = Bool(value) else {
-            throw CommandPluginError.invalidArgumentValue(name: flag.rawValue, value: value)
-          }
-          config.common.accessLevelOnImports = accessLevelOnImports
+        if argExtractor.extractFlag(named: flag.rawValue) > 0 {
+          config.common.accessLevelOnImports = true
         }
 
       case .importPath:
@@ -193,17 +192,17 @@ extension OptionsAndFlags {
   func usageDescription() -> String {
     switch self {
     case .servers:
-      return "Indicate that server code is to be generated. Generated by default."
+      return "Generate server code. Generated by default."
     case .noServers:
-      return "Indicate that server code is not to be generated. Generated by default."
+      return "Do not generate server code. Generated by default."
     case .clients:
-      return "Indicate that client code is to be generated. Generated by default."
+      return "Generate client code. Generated by default."
     case .noClients:
-      return "Indicate that client code is not to be generated. Generated by default."
+      return "Do not generate client code. Generated by default."
     case .messages:
-      return "Indicate that message code is to be generated. Generated by default."
+      return "Generate message code. Generated by default."
     case .noMessages:
-      return "Indicate that message code is not to be generated. Generated by default."
+      return "Do not generate message code. Generated by default."
     case .fileNaming:
       return
         "The naming scheme for output files [fullPath/pathToUnderscores/dropPath]. Defaults to fullPath."
@@ -213,7 +212,8 @@ extension OptionsAndFlags {
     case .accessLevelOnImports:
       return "Whether imports should have explicit access levels. Defaults to false."
     case .importPath:
-      return "The directory in which to search for imports."
+      return
+        "The directory in which to search for imports. May be specified multiple times. If none are specified the current working directory is used."
     case .protocPath:
       return "The path to the protoc binary."
     case .dryRun:
@@ -235,7 +235,9 @@ extension OptionsAndFlags {
       printMessage = Stderr.print
     }
 
-    printMessage("Usage: swift package generate-grpc-code-from-protos [flags] [--] [input files]")
+    printMessage(
+      "Usage: swift package generate-grpc-code-from-protos [flags] [\(CommandConfig.parameterGroupSeparator)] [input files]"
+    )
     printMessage("")
     printMessage("Flags:")
     printMessage("")

Plugins/GRPCProtobufGeneratorCommand/CommandPluginError.swift

@@ -15,38 +15,51 @@
  */
 
 enum CommandPluginError: Error {
-  case missingArgumentValue
   case invalidArgumentValue(name: String, value: String)
   case missingInputFile
   case unknownOption(String)
   case unknownAccessLevel(String)
   case unknownFileNamingStrategy(String)
   case conflictingFlags(String, String)
-  case generationFailure
+  case generationFailure(
+    errorDescription: String,
+    executable: String?,
+    arguments: [String]?,
+    stdErr: String?
+  )
   case tooManyParameterSeparators
 }
 
 extension CommandPluginError: CustomStringConvertible {
   var description: String {
     switch self {
-    case .missingArgumentValue:
-      "Provided option does not have a value."
     case .invalidArgumentValue(let name, let value):
-      "Invalid value '\(value)', for '\(name)'."
+      return "Invalid value '\(value)', for '\(name)'."
     case .missingInputFile:
-      "No input file(s) specified."
-    case .unknownOption(let value):
-      "Provided option is unknown: \(value)."
+      return "No input file(s) specified."
+    case .unknownOption(let name):
+      return "Provided option is unknown: \(name)."
     case .unknownAccessLevel(let value):
-      "Provided access level is unknown: \(value)."
+      return "Provided access level is unknown: \(value)."
     case .unknownFileNamingStrategy(let value):
-      "Provided file naming strategy is unknown: \(value)."
+      return "Provided file naming strategy is unknown: \(value)."
     case .conflictingFlags(let flag1, let flag2):
-      "Provided flags conflict: '\(flag1)' and '\(flag2)'."
-    case .generationFailure:
-      "Code generation failed."
+      return "Provided flags conflict: '\(flag1)' and '\(flag2)'."
+    case .generationFailure(let errorDescription, let executable, let arguments, let stdErr):
+      var message = "Code generation failed with: \(errorDescription)."
+      if let executable {
+        message += "\n\tExecutable: \(executable)"
+      }
+      if let arguments {
+        message += "\n\tArguments: \(arguments.joined(separator: " "))"
+      }
+      if let stdErr {
+        message += "\n\tprotoc error output:"
+        message += "\n\t\(stdErr)"
+      }
+      return message
     case .tooManyParameterSeparators:
-      "Unexpected parameter structure, too many '--' separators."
+      return "Unexpected parameter structure, too many '--' separators."
     }
   }
 }

Plugins/GRPCProtobufGeneratorCommand/Plugin.swift

@@ -52,28 +52,26 @@ struct GRPCProtobufGeneratorCommandPlugin {
     tool: (String) throws -> PluginContext.Tool,
     pluginWorkDirectoryURL: URL
   ) throws {
-    let groups = arguments.split(separator: "--")
     let flagsAndOptions: [String]
     let inputFiles: [String]
-    switch groups.count {
-    case 0:
-      OptionsAndFlags.printHelp(requested: false)
-      return
-
-    case 1:
-      inputFiles = Array(groups[0])
-      flagsAndOptions = []
 
-      var argExtractor = ArgumentExtractor(inputFiles)
+    let separatorCount = arguments.filter { $0 == CommandConfig.parameterGroupSeparator }.count
+    switch separatorCount {
+    case 0:
+      var argExtractor = ArgumentExtractor(arguments)
       // check if help requested
       if argExtractor.extractFlag(named: OptionsAndFlags.help.rawValue) > 0 {
         OptionsAndFlags.printHelp(requested: true)
         return
       }
 
-    case 2:
-      flagsAndOptions = Array(groups[0])
-      inputFiles = Array(groups[1])
+      inputFiles = arguments
+      flagsAndOptions = []
+
+    case 1:
+      let splitIndex = arguments.firstIndex(of: CommandConfig.parameterGroupSeparator)!
+      flagsAndOptions = Array(arguments[..<splitIndex])
+      inputFiles = Array(arguments[(splitIndex).advanced(by: 1)...])
 
     default:
       throw CommandPluginError.tooManyParameterSeparators
@@ -126,21 +124,15 @@ struct GRPCProtobufGeneratorCommandPlugin {
         outputDirectory: outputDirectory
       )
 
-      if commandConfig.verbose || commandConfig.dryRun {
-        printProtocInvocation(protocPath, arguments)
-      }
+      try executeProtocInvocation(
+        executableURL: protocPath,
+        arguments: arguments,
+        verbose: commandConfig.verbose,
+        dryRun: commandConfig.dryRun
+      )
+
       if !commandConfig.dryRun {
-        let process = try Process.run(protocPath, arguments: arguments)
-        process.waitUntilExit()
-
-        if process.terminationReason == .exit, process.terminationStatus == 0 {
-          if commandConfig.verbose {
-            Stderr.print("Generated gRPC Swift files for \(inputFiles.joined(separator: ", ")).")
-          }
-        } else {
-          let problem = "\(process.terminationReason):\(process.terminationStatus)"
-          throw CommandPluginError.generationFailure
-        }
+        Stderr.print("Generated gRPC Swift files for \(inputFiles.joined(separator: ", ")).")
       }
     }
 
@@ -155,31 +147,102 @@ struct GRPCProtobufGeneratorCommandPlugin {
         outputDirectory: outputDirectory
       )
 
-      if commandConfig.verbose || commandConfig.dryRun {
-        printProtocInvocation(protocPath, arguments)
-      }
+      let completionStatus = try executeProtocInvocation(
+        executableURL: protocPath,
+        arguments: arguments,
+        verbose: commandConfig.verbose,
+        dryRun: commandConfig.dryRun
+      )
+
       if !commandConfig.dryRun {
-        let process = try Process.run(protocPath, arguments: arguments)
-        process.waitUntilExit()
-
-        if process.terminationReason == .exit, process.terminationStatus == 0 {
-          Stderr.print(
-            "Generated protobuf message Swift files for \(inputFiles.joined(separator: ", "))."
-          )
-        } else {
-          let problem = "\(process.terminationReason):\(process.terminationStatus)"
-          throw CommandPluginError.generationFailure
-        }
+        Stderr.print(
+          "Generated protobuf message Swift files for \(inputFiles.joined(separator: ", "))."
+        )
       }
     }
   }
 }
 
-/// Print a single invocation of `protoc`
+/// Execute a single invocation of `protoc`, printing output and if in verbose mode the invocation
 /// - Parameters:
 ///   - executableURL: The path to the `protoc` executable.
 ///   - arguments: The arguments to be passed to `protoc`.
-func printProtocInvocation(_ executableURL: URL, _ arguments: [String]) {
-  Stderr.print("\(executableURL.absoluteStringNoScheme) \\")
-  Stderr.print("  \(arguments.joined(separator: " \\\n  "))")
+///   - verbose: Whether or not to print verbose output
+///   - dryRun: If this invocation is a dry-run, i.e. will not actually be executed
+
+func executeProtocInvocation(
+  executableURL: URL,
+  arguments: [String],
+  verbose: Bool,
+  dryRun: Bool
+) throws {
+  if verbose {
+    Stderr.print("\(executableURL.absoluteStringNoScheme) \\")
+    Stderr.print("  \(arguments.joined(separator: " \\\n  "))")
+  }
+
+  if dryRun {
+    return
+  }
+
+  let process = Process()
+  process.executableURL = executableURL
+  process.arguments = arguments
+
+  let outputPipe = Pipe()
+  let errorPipe = Pipe()
+  process.standardOutput = outputPipe
+  process.standardError = errorPipe
+
+  do {
+    try process.run()
+  } catch {
+    try printProtocOutput(outputPipe, verbose: verbose)
+    let stdErr: String?
+    if let errorData = try errorPipe.fileHandleForReading.readToEnd() {
+      stdErr = String(decoding: errorData, as: UTF8.self)
+    } else {
+      stdErr = nil
+    }
+    throw CommandPluginError.generationFailure(
+      errorDescription: "\(error)",
+      executable: executableURL.absoluteStringNoScheme,
+      arguments: arguments,
+      stdErr: stdErr
+    )
+  }
+  process.waitUntilExit()
+
+  try printProtocOutput(outputPipe, verbose: verbose)
+
+  guard process.terminationReason == .exit && process.terminationStatus == 0 else {
+    let stdErr: String?
+    if let errorData = try errorPipe.fileHandleForReading.readToEnd() {
+      stdErr = String(decoding: errorData, as: UTF8.self)
+    } else {
+      stdErr = nil
+    }
+    let problem = "\(process.terminationReason):\(process.terminationStatus)"
+    throw CommandPluginError.generationFailure(
+      errorDescription: problem,
+      executable: executableURL.absoluteStringNoScheme,
+      arguments: arguments,
+      stdErr: stdErr
+    )
+  }
+
+  return
+}
+
+func printProtocOutput(_ stdOut: Pipe, verbose: Bool) throws {
+  let prefix = "\t"
+
+  if verbose, let outputData = try stdOut.fileHandleForReading.readToEnd() {
+    let output = String(decoding: outputData, as: UTF8.self)
+    let lines = output.split { $0.isNewline }
+    print("protoc output:")
+    for line in lines {
+      print("\(prefix)\(line)")
+    }
+  }
 }