Fix paths in --proto_path and avoid copying proto files (#850)

* Fix paths in proto_path and avoid copying

* Prepare mapping in advance

* Condensed all transformations into one method

* Added tests

* Buildifier corrections

Author: ignasl

src/scala/scripts/PBGenerateRequest.scala

@@ -6,6 +6,30 @@ case class PBGenerateRequest(jarOutput: String, scalaPBOutput: Path, scalaPBArgs
 
 object PBGenerateRequest {
 
+  // This little function fixes a problem, where external/com_google_protobuf is not found. The com_google_protobuf
+  // is special in a way that it also brings-in protoc and also google well-known proto files. This, possibly,
+  // confuses Bazel and external/com_google_protobuf is not made available for target builds. Actual causes are unknown
+  // and this fixTransitiveProtoPath fixes this problem in the following way:
+  //  (1) We have a list of all required .proto files; this is a tuple list (root -> full path), for example:
+  //        bazel-out/k8-fastbuild/bin -> bazel-out/k8-fastbuild/bin/external/com_google_protobuf/google/protobuf/source_context.proto
+  //  (2) Convert the full path to relative from the root:
+  //        bazel-out/k8-fastbuild/bin -> external/com_google_protobuf/google/protobuf/source_context.proto
+  //  (3) From all the included protos we find the first one that is located within dir we are processing -- relative
+  //      path starts with the dir we are processing
+  //  (4) If found -- the include folder is "orphan" and is not anchored in either host or target. To fix we prepend
+  //      root. If not found, return original. This works as long as "external/com_google_protobuf" is available in
+  //      target root.
+  def fixTransitiveProtoPath(includedProto: List[(Path, Path)]): String => String = {
+    val includedRelProto = includedProto.map { case (root, full) => (root.toString, root.relativize(full).toString) }
+
+    { orig =>
+      includedRelProto.find { case (_, rel) => rel.startsWith(orig) } match {
+        case Some((root, _)) => s"$root/$orig"
+        case None => orig
+      }
+    }
+  }
+
   def from(args: java.util.List[String]): PBGenerateRequest = {
     val jarOutput = args.get(0)
     val protoFiles = args.get(4).split(':')
@@ -23,17 +47,18 @@ object PBGenerateRequest {
       case s if s.charAt(0) == '-' => Some(s.tail) //drop padding character
       case other => sys.error(s"expected a padding character of - (dash), but found: $other")
     }
-    val transitiveProtoPaths = (args.get(3) match {
+
+    val transitiveProtoPaths: List[String] = (args.get(3) match {
       case "-" => Nil
       case s if s.charAt(0) == '-' => s.tail.split(':').toList //drop padding character
       case other => sys.error(s"expected a padding character of - (dash), but found: $other")
-    }) ++ List(".")
+    }).map(fixTransitiveProtoPath(includedProto)) ++ List(".")
 
     val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp"))
     val scalaPBOutput = Files.createTempDirectory(tmp, "bazelscalapb")
     val flagPrefix = flagOpt.fold("")(_ + ":")
 
-      val namedGenerators = args.get(6).drop(1).split(',').filter(_.nonEmpty).map { e =>
+    val namedGenerators = args.get(6).drop(1).split(',').filter(_.nonEmpty).map { e =>
       val kv = e.split('=')
       (kv(0), kv(1))
     }

src/scala/scripts/ScalaPBGenerator.scala

@@ -34,20 +34,6 @@ object ScalaPBWorker extends GenericWorker(new ScalaPBGenerator) {
 }
 
 class ScalaPBGenerator extends Processor {
-  def setupIncludedProto(includedProto: List[(Path, Path)]): Unit = {
-    includedProto.foreach { case (root, fullPath) =>
-      require(fullPath.toFile.exists, s"Path $fullPath does not exist, which it should as a dependency of this rule")
-      val relativePath = root.relativize(fullPath)
-
-      relativePath.toFile.getParentFile.mkdirs
-      Try(Files.copy(fullPath, relativePath)) match {
-        case Failure(err: FileAlreadyExistsException) =>
-          Console.println(s"File already exists, skipping: ${err.getMessage}")
-        case Failure(err) => throw err
-        case _ => ()
-      }
-    }
-  }
   def deleteDir(path: Path): Unit =
     try DeleteRecursively.run(path)
     catch {
@@ -56,8 +42,6 @@ class ScalaPBGenerator extends Processor {
 
   def processRequest(args: java.util.List[String]) {
     val extractRequestResult = PBGenerateRequest.from(args)
-    setupIncludedProto(extractRequestResult.includedProto)
-
     val extraClassesClassLoader = new URLClassLoader(extractRequestResult.extraJars.map { e =>
       val f = e.toFile
       require(f.exists, s"Expected file for classpath loading $f to exist")

test/src/main/scala/scalarules/test/scripts/BUILD

@@ -0,0 +1,10 @@
+load("//scala:scala.bzl", "scala_library", "scala_specs2_junit_test")
+load("//scala:scala_import.bzl", "scala_import")
+
+scala_specs2_junit_test(
+    name = "pb_generate_request_test",
+    size = "small",
+    srcs = ["PBGenerateRequestTest.scala"],
+    suffixes = ["Test"],
+    deps = ["//src/scala/scripts:scala_proto_request_extractor"],
+)

test/src/main/scala/scalarules/test/scripts/PBGenerateRequestTest.scala

@@ -0,0 +1,21 @@
+package scalarules.test.scripts
+
+import java.nio.file.Paths
+import scripts.PBGenerateRequest
+import org.specs2.mutable.SpecWithJUnit
+
+class PBGenerateRequestTest extends SpecWithJUnit {
+  "fixTransitiveProtoPath should fix path when included proto is available, ignore otherwise" >> {
+    val includedProtos = List(Paths.get("a/b/c") -> Paths.get("a/b/c/d/e/f.proto"))
+    Seq("d/e", "x/y/z").map(PBGenerateRequest.fixTransitiveProtoPath(includedProtos)) must
+      beEqualTo(Seq("a/b/c/d/e", "x/y/z"))
+  }
+
+  "actual case observed in builds" >> {
+    val includedProtos = List(
+      Paths.get("bazel-out/k8-fastbuild/bin") ->
+        Paths.get("bazel-out/k8-fastbuild/bin/external/com_google_protobuf/google/protobuf/source_context.proto"))
+    Seq("external/com_google_protobuf").map(PBGenerateRequest.fixTransitiveProtoPath(includedProtos)) must
+      beEqualTo(Seq("bazel-out/k8-fastbuild/bin/external/com_google_protobuf"))
+  }
+}