allow passing the same PluginGenerator several times

scalapb · Jan 26, 2021 · c8799d8 · c8799d8
1 parent eb1c738
commit c8799d8
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 60 deletions.
diff --git a/bridge/src/main/scala/protocbridge/ProtocBridge.scala b/bridge/src/main/scala/protocbridge/ProtocBridge.scala
@@ -36,33 +36,46 @@ object ProtocBridge {
       classLoader: Artifact => ClassLoader
   ): ExitCode = {
 
-    // The same JvmGenerator might be passed several times, but requires separate frontends
-    val targetsSuffixed = targets.zipWithIndex.map {
-      case (t @ Target(gen: JvmGenerator, _, _), i) =>
-        t.copy(generator = gen.copy(name = s"${gen.name}_$i"))
-      case (t @ Target(gen: SandboxedJvmGenerator, _, _), i) =>
-        val codeGen =
-          gen.resolver(classLoader(gen.artifact))
-        t.copy(generator =
-          JvmGenerator(name = codeGen.name + s"_$i", gen = codeGen)
-        )
-      case (t, _) => t
-    }
+    // The same JvmGenerator or PluginGenerator might be passed several times with different options
+    val targetsSuffixed =
+      targets
+        .groupBy(_.generator.name)
+        .values
+        .flatMap { targets =>
+          // don't add suffix if we have only one generator for that name
+          if (targets.length == 1) targets
+          else {
+            targets.zipWithIndex.map {
+              case (t @ Target(gen: JvmGenerator, _, _), i) =>
+                t.copy(generator = gen.copy(name = s"${gen.name}_$i"))
+              case (t @ Target(gen: SandboxedJvmGenerator, _, _), i) =>
+                val codeGen =
+                  gen.resolver(classLoader(gen.artifact))
+                t.copy(generator =
+                  JvmGenerator(name = codeGen.name + s"_$i", gen = codeGen)
+                )
+              case (t @ Target(gen: PluginGenerator, _, _), i) =>
+                t.copy(generator = gen.copy(name = s"${gen.name}_$i"))
+              case (t, _) => t
+            }
+          }
+        }
+        .toSeq
 
     val namedGenerators: Seq[(String, ProtocCodeGenerator)] =
       targetsSuffixed.collect { case Target(gen: JvmGenerator, _, _) =>
         (gen.name, gen.gen)
       }
 
-    val cmdLine: Seq[String] = pluginArgs(targets) ++ targetsSuffixed.flatMap {
-      p =>
+    val cmdLine: Seq[String] =
+      pluginArgs(targetsSuffixed) ++ (targetsSuffixed).flatMap { p =>
         val maybeOptions =
           if (p.options.isEmpty) Nil
           else {
             s"--${p.generator.name}_opt=${p.options.mkString(",")}" :: Nil
           }
         s"--${p.generator.name}_out=${p.outputPath.getAbsolutePath}" :: maybeOptions
-    } ++ params
+      } ++ params
 
     runWithGenerators(protoc, namedGenerators, cmdLine, pluginFrontend)
   }
@@ -96,28 +109,10 @@ object ProtocBridge {
     PluginFrontend.newInstance
   )
 
-  private def pluginArgs(targets: Seq[Target]): Seq[String] = {
-    val pluginsAndPaths: Seq[(String, String)] = targets.collect {
-      case Target(PluginGenerator(pluginName, _, Some(pluginPath)), _, _) =>
-        (pluginName, pluginPath)
-    }.distinct
-
-    val pluginsWithDifferentPaths =
-      pluginsAndPaths.groupBy(_._1).values.collect {
-        case (pluginName, _) :: rest if rest.nonEmpty => pluginName
-      }
-
-    if (pluginsWithDifferentPaths.nonEmpty) {
-      throw new RuntimeException(
-        "Different paths found for the plugin: " + pluginsWithDifferentPaths
-          .mkString(",")
-      )
-    }
-
-    pluginsAndPaths.map { case (name, path) =>
+  private def pluginArgs(targets: Seq[Target]): Seq[String] =
+    targets.collect { case Target(PluginGenerator(name, _, Some(path)), _, _) =>
       s"--plugin=protoc-gen-$name=$path"
     }
-  }
 
   def runWithGenerators[ExitCode](
       protoc: ProtocRunner[ExitCode],

diff --git a/bridge/src/test/scala/protocbridge/ProtocBridgeSpec.scala b/bridge/src/test/scala/protocbridge/ProtocBridgeSpec.scala
@@ -90,36 +90,25 @@ class ProtocBridgeSpec extends AnyFlatSpec with Matchers {
     run(
       Seq(
         Target(gens.plugin("foo", "/path/to/plugin"), TmpPath),
-        Target(gens.plugin("foo", "/path/to/plugin"), TmpPath1),
+        Target(gens.plugin("foo", "/otherpath/to/plugin"), TmpPath1),
         Target(gens.plugin("foo"), TmpPath2),
         Target(gens.plugin("bar"), TmpPath)
       )
     ) must be(
       Seq(
-        "--plugin=protoc-gen-foo=/path/to/plugin",
-        s"--foo_out=$TmpPath",
-        s"--foo_out=$TmpPath1",
-        s"--foo_out=$TmpPath2",
+        "--plugin=protoc-gen-foo_0=/path/to/plugin",
+        "--plugin=protoc-gen-foo_1=/otherpath/to/plugin",
+        s"--foo_0_out=$TmpPath",
+        s"--foo_1_out=$TmpPath1",
+        s"--foo_2_out=$TmpPath2",
         s"--bar_out=$TmpPath"
       )
     )
   }
 
-  it should "not allow ambigious paths for plugins" in {
-    intercept[RuntimeException] {
-      run(
-        Seq(
-          Target(gens.plugin("foo", "/path/to/plugin"), TmpPath1),
-          Target(gens.plugin("foo", "/other/path/to/plugin"), TmpPath1)
-        )
-      )
-    }.getMessage() must be("Different paths found for the plugin: foo")
-  }
-
-  val DefineFlag = "--plugin=protoc-gen-jvm_(.*?)=null".r
-  val UseFlag = s"--jvm_(.*?)_out=${Pattern.quote(TmpPath.toString)}".r
-  val UseFlagParams =
-    s"--jvm_(.*?)_opt=x,y".r
+  val DefineFlag = "--plugin=protoc-gen-(.*?)=null".r
+  val UseFlag = s"--(.*?)_out=${Pattern.quote(TmpPath.toString)}".r
+  val UseFlagParams = s"--(.*?)_opt=x,y".r
 
   it should "allow using FooBarGen" in {
     run(Seq(Target(FoobarGen, TmpPath))) match {
@@ -142,13 +131,13 @@ class ProtocBridgeSpec extends AnyFlatSpec with Matchers {
     ) must be(
       Seq(
         "--plugin=protoc-gen-fff_0=null",
-        "--plugin=protoc-gen-jvm_1=null",
-        "--plugin=protoc-gen-fff_2=null",
+        "--plugin=protoc-gen-fff_1=null",
+        "--plugin=protoc-gen-jvm=null",
         s"--fff_0_out=$TmpPath",
         s"--fff_0_opt=x,y",
-        s"--jvm_1_out=$TmpPath1",
-        s"--fff_2_out=$TmpPath2",
-        s"--fff_2_opt=foo,bar"
+        s"--fff_1_out=$TmpPath2",
+        s"--fff_1_opt=foo,bar",
+        s"--jvm_out=$TmpPath1"
       )
     )
   }