Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions src/scala/scripts/PBGenerateRequest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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(':')
Expand All @@ -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))
}
Expand Down
16 changes: 0 additions & 16 deletions src/scala/scripts/ScalaPBGenerator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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")
Expand Down
10 changes: 10 additions & 0 deletions test/src/main/scala/scalarules/test/scripts/BUILD
Original file line number Diff line number Diff line change
@@ -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"],
)
Original file line number Diff line number Diff line change
@@ -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"))
}
}