Refactor scala_library.go with --plugins option (#263)

pcj · web-flow · commit 9bea22f9fe0b · 2022-07-26T18:35:09.000-06:00
* {proto,grpc}_scala_library: add --partition_services flag

* Checkpoint WIP

* Refactor scala_library.go with --plugins option

* Cleanup

* Fix golden example integration test

* fix unit test

* Fix more label changes

* Add back file filter

* Match plugin name rather than plugin implementation name

* add distinct configiration for grpc vs. non-grpc

* Fix resolution of file imports
diff --git a/docs/scala.md b/docs/scala.md
@@ -5,15 +5,13 @@ permalink: examples/scala
 parent: Examples
 ---
 
-
 # scala example
 
 `bazel test //example/golden:scala_test`
 
-
 ## `BUILD.bazel` (after gazelle)
 
-~~~python
+```python
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@build_stack_rules_proto//rules/scala:grpc_scala_library.bzl", "grpc_scala_library")
 load("@build_stack_rules_proto//rules:proto_compile.bzl", "proto_compile")
@@ -54,7 +52,7 @@ grpc_scala_library(
         "@maven_scala//:io_grpc_grpc_api",
         "@maven_scala//:io_grpc_grpc_protobuf",
         "@maven_scala//:io_grpc_grpc_stub",
-        "@scalaapis//scalapb:scalapb_scala_library",
+        "@scalaapis//scalapb:scalapb_proto_scala_library",
     ],
 )
 
@@ -71,18 +69,16 @@ proto_compile(
     ],
     proto = "syntax_proto",
 )
-~~~
-
+```
 
 ## `BUILD.bazel` (before gazelle)
 
-~~~python
-~~~
-
+```python
+```
 
 ## `WORKSPACE`
 
-~~~python
+```python
 # ----------------------------------------------------
 # scala
 # ----------------------------------------------------
@@ -159,5 +155,4 @@ proto_repository(
     type = "zip",
     urls = ["https://codeload.github.com/scalapb/ScalaPB/zip/a4e0e02c0f5b160877d5f97f6902dbec4c633afe"],
 )
-~~~
-
+```
diff --git a/example/golden/testdata/scala/BUILD.in b/example/golden/testdata/scala/BUILD.in
@@ -1,3 +1,5 @@
 exports_files(["config.yaml"])
 
 # gazelle:proto_language scala enabled true
+# gazelle:proto_rule proto_scala_library option --plugins=protoc-gen-scala
+# gazelle:proto_rule grpc_scala_library option --plugins=protoc-gen-akka-grpc
diff --git a/example/golden/testdata/scala/BUILD.out b/example/golden/testdata/scala/BUILD.out
@@ -1,3 +1,5 @@
 exports_files(["config.yaml"])
 
 # gazelle:proto_language scala enabled true
+# gazelle:proto_rule proto_scala_library option --plugins=protoc-gen-scala
+# gazelle:proto_rule grpc_scala_library option --plugins=protoc-gen-akka-grpc
diff --git a/example/golden/testdata/scala/config.yaml b/example/golden/testdata/scala/config.yaml
@@ -1,39 +1,46 @@
 plugins:
   - name: protoc-gen-scala
     implementation: scalapb:scalapb:protoc-gen-scala
+  - name: protoc-gen-scala-grpc
+    implementation: scalapb:scalapb:protoc-gen-scala
+    label: "@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala-grpc"
     options:
       - grpc
-    deps:
-      - "@com_google_protobuf//:protobuf_java"
-      - "@maven_scala//:com_thesamet_scalapb_lenses_2_12"
-      - "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12"
-      - "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_grpc_2_12"
-      - "@maven_scala//:io_grpc_grpc_api"
-      - "@maven_scala//:io_grpc_grpc_stub"
-      - "@maven_scala//:io_grpc_grpc_protobuf"
   - name: protoc-gen-akka-grpc
     implementation: akka:akka-grpc:protoc-gen-akka-grpc
-    deps:
-      - "@maven_akka//:com_lightbend_akka_grpc_akka_grpc_runtime_2_12"
-      - "@maven_akka//:com_typesafe_akka_akka_actor_2_12"
-      - "@maven_akka//:com_typesafe_akka_akka_http_core_2_12"
-      - "@maven_akka//:com_typesafe_akka_akka_stream_2_12"
 rules:
   - name: proto_compile
     implementation: stackb:rules_proto:proto_compile
   - name: proto_scala_library
     implementation: stackb:rules_proto:proto_scala_library
     visibility:
       - //visibility:public
+    deps:
+      - "@com_google_protobuf//:protobuf_java"
+      - "@maven_scala//:com_thesamet_scalapb_lenses_2_12"
+      - "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12"
   - name: grpc_scala_library
     implementation: stackb:rules_proto:grpc_scala_library
     visibility:
       - //visibility:public
+    deps:
+      - "@com_google_protobuf//:protobuf_java"
+      - "@maven_akka//:com_lightbend_akka_grpc_akka_grpc_runtime_2_12"
+      - "@maven_akka//:com_typesafe_akka_akka_actor_2_12"
+      - "@maven_akka//:com_typesafe_akka_akka_http_core_2_12"
+      - "@maven_akka//:com_typesafe_akka_akka_stream_2_12"
+      - "@maven_scala//:com_thesamet_scalapb_lenses_2_12"
+      - "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12"
+      - "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_grpc_2_12"
+      - "@maven_scala//:io_grpc_grpc_api"
+      - "@maven_scala//:io_grpc_grpc_protobuf"
+      - "@maven_scala//:io_grpc_grpc_stub"
 languages:
   - name: scala
     plugins:
-      - protoc-gen-akka-grpc
       - protoc-gen-scala
+      - protoc-gen-scala-grpc
+      - protoc-gen-akka-grpc
     rules:
       - proto_compile
       - proto_scala_library
diff --git a/example/golden/testdata/scala/imports.csv b/example/golden/testdata/scala/imports.csv
@@ -1,3 +1,3 @@
 proto,proto,scalapb/scalapb.proto,@scalaapis//scalapb:scalapb_proto
-protobuf,scala,scalapb/scalapb.proto,@scalaapis//scalapb:scalapb_scala_library
+protobuf,scala,scalapb/scalapb.proto,@scalaapis//scalapb:scalapb_proto_scala_library
 protobuf,scala,lib.WithSnapshot,//lib:scala
diff --git a/example/golden/testdata/scala/lib/BUILD.in b/example/golden/testdata/scala/lib/BUILD.in
@@ -5,7 +5,7 @@ scala_library(
     srcs = ["WithSnapshot.scala"],
     visibility = ["//visibility:public"],
     deps = [
-        "//proto:proto_scala_library",
+        "//proto:proto_proto_scala_library",
         "@maven_scala//:com_thesamet_scalapb_lenses_2_12",
         "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12",
     ],
diff --git a/example/golden/testdata/scala/lib/BUILD.out b/example/golden/testdata/scala/lib/BUILD.out
@@ -5,7 +5,7 @@ scala_library(
     srcs = ["WithSnapshot.scala"],
     visibility = ["//visibility:public"],
     deps = [
-        "//proto:proto_scala_library",
+        "//proto:proto_proto_scala_library",
         "@maven_scala//:com_thesamet_scalapb_lenses_2_12",
         "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12",
     ],
diff --git a/example/golden/testdata/scala/proto/BUILD.out b/example/golden/testdata/scala/proto/BUILD.out
@@ -10,22 +10,25 @@ proto_library(
 
 proto_compile(
     name = "proto_scala_compile",
-    outputs = ["proto_scala.srcjar"],
-    plugins = ["@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala"],
+    options = {"@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala-grpc": ["grpc"]},
+    outputs = [
+        "proto_scala.srcjar",
+        "proto_scala_grpc.srcjar",
+    ],
+    plugins = [
+        "@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala",
+        "@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala-grpc",
+    ],
     proto = "proto_proto",
 )
 
 proto_scala_library(
-    name = "proto_scala_library",
+    name = "proto_proto_scala_library",
     srcs = ["proto_scala.srcjar"],
     visibility = ["//visibility:public"],
     deps = [
         "@com_google_protobuf//:protobuf_java",
         "@maven_scala//:com_thesamet_scalapb_lenses_2_12",
         "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12",
-        "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_grpc_2_12",
-        "@maven_scala//:io_grpc_grpc_api",
-        "@maven_scala//:io_grpc_grpc_protobuf",
-        "@maven_scala//:io_grpc_grpc_stub",
     ],
 )
diff --git a/example/golden/testdata/scala/syntax/BUILD.out b/example/golden/testdata/scala/syntax/BUILD.out
@@ -1,5 +1,6 @@
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@build_stack_rules_proto//rules/scala:grpc_scala_library.bzl", "grpc_scala_library")
+load("@build_stack_rules_proto//rules/scala:proto_scala_library.bzl", "proto_scala_library")
 load("@build_stack_rules_proto//rules:proto_compile.bzl", "proto_compile")
 
 proto_library(
@@ -18,15 +19,12 @@ proto_library(
 )
 
 grpc_scala_library(
-    name = "syntax_scala_library",
-    srcs = [
-        "syntax_akka_grpc.srcjar",
-        "syntax_scala.srcjar",
-    ],
+    name = "syntax_grpc_scala_library",
+    srcs = ["syntax_akka_grpc.srcjar"],
     visibility = ["//visibility:public"],
     deps = [
-        "//lib:scala",
-        "//proto:proto_scala_library",
+        ":syntax_proto_scala_library",
+        "//proto:proto_proto_scala_library",
         "@com_google_protobuf//:protobuf_java",
         "@maven_akka//:com_lightbend_akka_grpc_akka_grpc_runtime_2_12",
         "@maven_akka//:com_typesafe_akka_akka_actor_2_12",
@@ -38,20 +36,36 @@ grpc_scala_library(
         "@maven_scala//:io_grpc_grpc_api",
         "@maven_scala//:io_grpc_grpc_protobuf",
         "@maven_scala//:io_grpc_grpc_stub",
-        "@scalaapis//scalapb:scalapb_scala_library",
+        "@scalaapis//scalapb:scalapb_proto_scala_library",
     ],
 )
 
 proto_compile(
     name = "syntax_scala_compile",
-    options = {"@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala": ["grpc"]},
+    options = {"@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala-grpc": ["grpc"]},
     outputs = [
         "syntax_akka_grpc.srcjar",
         "syntax_scala.srcjar",
+        "syntax_scala_grpc.srcjar",
     ],
     plugins = [
         "@build_stack_rules_proto//plugin/akka/akka-grpc:protoc-gen-akka-grpc",
         "@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala",
+        "@build_stack_rules_proto//plugin/scalapb/scalapb:protoc-gen-scala-grpc",
     ],
     proto = "syntax_proto",
 )
+
+proto_scala_library(
+    name = "syntax_proto_scala_library",
+    srcs = ["syntax_scala.srcjar"],
+    visibility = ["//visibility:public"],
+    deps = [
+        "//lib:scala",
+        "//proto:proto_proto_scala_library",
+        "@com_google_protobuf//:protobuf_java",
+        "@maven_scala//:com_thesamet_scalapb_lenses_2_12",
+        "@maven_scala//:com_thesamet_scalapb_scalapb_runtime_2_12",
+        "@scalaapis//scalapb:scalapb_proto_scala_library",
+    ],
+)
diff --git a/pkg/plugin/scalapb/scalapb/protoc_gen_scala.go b/pkg/plugin/scalapb/scalapb/protoc_gen_scala.go
@@ -23,18 +23,16 @@ func (p *ProtocGenScalaPlugin) Name() string {
 
 // Configure implements part of the Plugin interface.
 func (p *ProtocGenScalaPlugin) Configure(ctx *protoc.PluginContext) *protoc.PluginConfiguration {
-	srcjar := ctx.ProtoLibrary.BaseName() + "_scala.srcjar"
-	if ctx.Rel != "" {
-		srcjar = path.Join(ctx.Rel, srcjar)
-	}
 	options := ctx.PluginConfig.GetOptions()
+	hasGrpc := containsOption(options, "grpc")
 
-	// if the plugin has 'grpc' statically configured, but the proto_library
-	// does not contain services, remove it.
-	if filtered, ok := removeAll(options, "grpc"); ok {
-		if !protoc.HasServices(ctx.ProtoLibrary.Files()...) {
-			options = filtered
-		}
+	srcjar := ctx.ProtoLibrary.BaseName() + "_scala"
+	if hasGrpc {
+		srcjar += "_grpc"
+	}
+	srcjar += ".srcjar"
+	if ctx.Rel != "" {
+		srcjar = path.Join(ctx.Rel, srcjar)
 	}
 
 	return &protoc.PluginConfiguration{
@@ -44,17 +42,13 @@ func (p *ProtocGenScalaPlugin) Configure(ctx *protoc.PluginContext) *protoc.Plug
 	}
 }
 
-// removeAll returns a copy of the slice will all elements matching 'key'
+// containsOption returns a copy of the slice will all elements matching 'key'
 // removed.  If at least item was removed, return true.
-func removeAll(src []string, key string) ([]string, bool) {
-	dst := make([]string, 0)
-	found := false
+func containsOption(src []string, key string) bool {
 	for _, item := range src {
 		if item == key {
-			found = true
-			continue
+			return true
 		}
-		dst = append(dst, item)
 	}
-	return dst, found
+	return false
 }
diff --git a/pkg/protoc/depsresolver.go b/pkg/protoc/depsresolver.go
@@ -190,13 +190,20 @@ func StripRel(rel string, filename string) string {
 func ProtoLibraryImportSpecsForKind(kind string, libs ...ProtoLibrary) []resolve.ImportSpec {
 	specs := make([]resolve.ImportSpec, 0)
 	for _, lib := range libs {
-		files := lib.Files()
-		for _, file := range files {
-			imp := path.Join(file.Dir, file.Basename)
-			spec := resolve.ImportSpec{Lang: kind, Imp: imp}
-			specs = append(specs, spec)
-		}
+		specs = append(specs, ProtoFilesImportSpecsForKind(kind, lib.Files())...)
 	}
 
 	return specs
 }
+
+// ProtoLibraryImportSpecsForKind generates an ImportSpec for each file in the
+// set of given proto_library.
+func ProtoFilesImportSpecsForKind(kind string, files []*File) []resolve.ImportSpec {
+	specs := make([]resolve.ImportSpec, 0)
+	for _, file := range files {
+		imp := path.Join(file.Dir, file.Basename)
+		spec := resolve.ImportSpec{Lang: kind, Imp: imp}
+		specs = append(specs, spec)
+	}
+	return specs
+}
diff --git a/pkg/protoc/language_rule_config.go b/pkg/protoc/language_rule_config.go
@@ -19,7 +19,7 @@ type LanguageRuleConfig struct {
 	Attrs map[string]map[string]bool
 	// Options is a generic key -> value string mapping.  Various rule
 	// implementations are free to document/interpret options in an
-	// implementation-dependenct manner.
+	// implementation-dependent manner.  TODO: deprecate this and rename to 'Flags'
 	Options map[string]bool
 	// Resolves is a mapping from resolve mapping spec to rewrite.  Negative
 	// intent is represented by the empty rewrite value.
diff --git a/pkg/rule/rules_scala/README.md b/pkg/rule/rules_scala/README.md
@@ -28,7 +28,7 @@ gazelle:proto_rule proto_scala_library deps @maven//:com_google_protobuf_protobu
 gazelle:proto_rule proto_scala_library deps @maven//:com_thesamet_scalapb_lenses_2_12
 gazelle:proto_rule proto_scala_library deps @maven//:com_thesamet_scalapb_scalapb_runtime_2_12
 gazelle:proto_rule proto_scala_library options --noresolve=scalapb/scalapb.proto
-gazelle:proto_rule proto_scala_library options --nooutput=package_scala.srcjar
+gazelle:proto_rule proto_scala_library options --exclude=package_scala.srcjar
 gazelle:proto_rule proto_scala_library visibility //visibility:public
 ```
 
@@ -74,4 +74,4 @@ However, in this case we do not want to emit a `proto_scala_library`. Why?
 Because the `package_scala.srcjar` in this case will be empty, as the scalapbc
 plugin does not emit and corresponding JVM code for this degenerate case.
 
-Therefore, the `--nooutput` means "suppress this output from the output list".
+Therefore, the `--exclude` means "suppress this output from the output list".
diff --git a/pkg/rule/rules_scala/scala_library.go b/pkg/rule/rules_scala/scala_library.go
diff --git a/pkg/rule/rules_scala/scala_library_test.go b/pkg/rule/rules_scala/scala_library_test.go
diff --git a/plugin/scalapb/scalapb/BUILD.bazel b/plugin/scalapb/scalapb/BUILD.bazel
diff --git a/rules/private/proto_repository_tools_srcs.bzl b/rules/private/proto_repository_tools_srcs.bzl
