From 13446a541ca83e413fd67b21fe9c77776272882c Mon Sep 17 00:00:00 2001 From: Ian O'Connell Date: Wed, 18 Sep 2019 11:39:36 -0600 Subject: [PATCH 1/4] Copy the protos we need into a temp folder --- src/scala/scripts/PBGenerateRequest.scala | 21 +++++++-------- src/scala/scripts/ScalaPBGenerator.scala | 31 ++++++++++++++++------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/scala/scripts/PBGenerateRequest.scala b/src/scala/scripts/PBGenerateRequest.scala index a35d03c31..4c7efeffd 100644 --- a/src/scala/scripts/PBGenerateRequest.scala +++ b/src/scala/scripts/PBGenerateRequest.scala @@ -6,17 +6,17 @@ case class PBGenerateRequest(jarOutput: String, scalaPBOutput: Path, scalaPBArgs object PBGenerateRequest { - def from(args: java.util.List[String]): PBGenerateRequest = { + def from(tmpDir: Path)(args: java.util.List[String]): PBGenerateRequest = { val jarOutput = args.get(0) val protoFiles = args.get(4).split(':') + val protoFilesToBuild = protoFiles.map { e => s"${tmpDir.toFile.toString}/$e"} + val includedProto = args.get(1).drop(1).split(':').distinct.map { e => val p = e.split(',') // If its an empty string then it means we are local to the current repo for the key, no op - (Some(p(0)).filter(_.nonEmpty), p(1)) - }.collect { - // if the to compile files contains this absolute path then we are compiling it and shoudln't try move it around(duplicate files.) - case (Some(k), v) if !protoFiles.contains(v) => (Paths.get(k), Paths.get(v)) - }.toList + val absolutePath = Some(p(0)).filter(_.nonEmpty).getOrElse(p(1)) + (Paths.get(absolutePath), Paths.get(p(1))) + }.toList ++ protoFiles.map { p => (Paths.get(p), Paths.get(p))} val flagOpt = args.get(2) match { case "-" => None @@ -27,7 +27,7 @@ object PBGenerateRequest { 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(".") + }) ++ List("") val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp")) val scalaPBOutput = Files.createTempDirectory(tmp, "bazelscalapb") @@ -43,7 +43,8 @@ object PBGenerateRequest { }.toList - val scalaPBArgs = outputSettings ::: (padWithProtoPathPrefix(transitiveProtoPaths) ++ protoFiles) + val scalaPBArgs = outputSettings ::: (padWithProtoPathPrefix(tmpDir)(transitiveProtoPaths) ++ protoFilesToBuild) + val protoc = Paths.get(args.get(5)) val extraJars = args.get(7).drop(1).split(':').filter(_.nonEmpty).distinct.map {e => Paths.get(e)}.toList @@ -51,7 +52,7 @@ object PBGenerateRequest { new PBGenerateRequest(jarOutput, scalaPBOutput, scalaPBArgs, includedProto, protoc, namedGenerators, extraJars) } - private def padWithProtoPathPrefix(transitiveProtoPathFlags: List[String]) = - transitiveProtoPathFlags.map("--proto_path="+_) + private def padWithProtoPathPrefix(tmpDir: Path)(transitiveProtoPathFlags: List[String]) = + transitiveProtoPathFlags.map(s"--proto_path=${tmpDir.toFile.toString}/"+_).map(_.stripSuffix(".")) } diff --git a/src/scala/scripts/ScalaPBGenerator.scala b/src/scala/scripts/ScalaPBGenerator.scala index 5fdaa8937..71cef146b 100644 --- a/src/scala/scripts/ScalaPBGenerator.scala +++ b/src/scala/scripts/ScalaPBGenerator.scala @@ -34,15 +34,16 @@ object ScalaPBWorker extends GenericWorker(new ScalaPBGenerator) { } class ScalaPBGenerator extends Processor { - def setupIncludedProto(includedProto: List[(Path, Path)]): Unit = { + def setupIncludedProto(tmpRoot: Path, 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) + val relativePath = if(root == fullPath) fullPath else root.relativize(fullPath) + val targetPath = tmpRoot.resolve(relativePath) - relativePath.toFile.getParentFile.mkdirs - Try(Files.copy(fullPath, relativePath)) match { + targetPath.toFile.getParentFile.mkdirs + Try(Files.copy(fullPath, targetPath)) match { case Failure(err: FileAlreadyExistsException) => - Console.println(s"File already exists, skipping: ${err.getMessage}") + Console.println(s"File already exists, skipping copy from $fullPath to $targetPath: ${err.getMessage}") case Failure(err) => throw err case _ => () } @@ -55,8 +56,10 @@ class ScalaPBGenerator extends Processor { } def processRequest(args: java.util.List[String]) { - val extractRequestResult = PBGenerateRequest.from(args) - setupIncludedProto(extractRequestResult.includedProto) + val tmpRoot = Files.createTempDirectory("proto_builder") + + val extractRequestResult = PBGenerateRequest.from(tmpRoot)(args) + setupIncludedProto(tmpRoot, extractRequestResult.includedProto) val extraClassesClassLoader = new URLClassLoader(extractRequestResult.extraJars.map { e => val f = e.toFile @@ -93,6 +96,16 @@ class ScalaPBGenerator extends Processor { } } - protected def exec(protoc: Path): Seq[String] => Int = (args: Seq[String]) => - new ProcessBuilder(protoc.toString +: args: _*).inheritIO().start().waitFor() + protected def exec(protoc: Path): Seq[String] => Int = (args: Seq[String]) => { + val tmpFile = Files.createTempFile("stdout", "log") + try { + val ret = new ProcessBuilder(protoc.toString +: args: _*).redirectErrorStream(true).redirectOutput(ProcessBuilder.Redirect.to(tmpFile.toFile)).start().waitFor() + Try { + scala.io.Source.fromFile(tmpFile.toFile).getLines.foreach{System.out.println} + } + ret + } finally { + tmpFile.toFile.delete + } + } } From b5091472dcaa2ac76dacfb73383b8a81989d7cd7 Mon Sep 17 00:00:00 2001 From: Ian O'Connell Date: Wed, 18 Sep 2019 11:46:25 -0600 Subject: [PATCH 2/4] Ensure we cleanup --- src/scala/scripts/ScalaPBGenerator.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/src/scala/scripts/ScalaPBGenerator.scala b/src/scala/scripts/ScalaPBGenerator.scala index 71cef146b..a67ff516a 100644 --- a/src/scala/scripts/ScalaPBGenerator.scala +++ b/src/scala/scripts/ScalaPBGenerator.scala @@ -92,6 +92,7 @@ class ScalaPBGenerator extends Processor { } JarCreator.buildJar(Array(extractRequestResult.jarOutput, extractRequestResult.scalaPBOutput.toString)) } finally { + deleteDir(tmpRoot) deleteDir(extractRequestResult.scalaPBOutput) } } From 359bd614a133845124d4ab99c5168438adb2b1bb Mon Sep 17 00:00:00 2001 From: Ian O'Connell Date: Wed, 18 Sep 2019 16:01:42 -0600 Subject: [PATCH 3/4] Refactor this code with clearer meanings --- src/scala/scripts/PBGenerateRequest.scala | 44 ++++++++++++++++++----- src/scala/scripts/ScalaPBGenerator.scala | 11 +++--- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/scala/scripts/PBGenerateRequest.scala b/src/scala/scripts/PBGenerateRequest.scala index 4c7efeffd..3bdf488a1 100644 --- a/src/scala/scripts/PBGenerateRequest.scala +++ b/src/scala/scripts/PBGenerateRequest.scala @@ -6,17 +6,45 @@ case class PBGenerateRequest(jarOutput: String, scalaPBOutput: Path, scalaPBArgs object PBGenerateRequest { + // If there is a virtual import in the path, then + // the path under there is pretty friendly already to protoc + def stripToVirtual(e: String): String = { + val v = "/_virtual_imports/" + val idx = e.indexOf(v) + if(idx >= 0) { + e.drop(idx + v.size) + } else e + } + def from(tmpDir: Path)(args: java.util.List[String]): PBGenerateRequest = { val jarOutput = args.get(0) val protoFiles = args.get(4).split(':') - val protoFilesToBuild = protoFiles.map { e => s"${tmpDir.toFile.toString}/$e"} - - val includedProto = args.get(1).drop(1).split(':').distinct.map { e => - val p = e.split(',') - // If its an empty string then it means we are local to the current repo for the key, no op - val absolutePath = Some(p(0)).filter(_.nonEmpty).getOrElse(p(1)) - (Paths.get(absolutePath), Paths.get(p(1))) - }.toList ++ protoFiles.map { p => (Paths.get(p), Paths.get(p))} + val protoFilesToBuild = protoFiles.map { e => s"${tmpDir.toFile.toString}/${stripToVirtual(e)}"} + + val includedProtoSplit: List[(String, String)] = args.get(1).drop(1).split(':').map { e => + val arr = e.split(',') + (arr(0), arr(1)) + }.toList + val includedProto: List[(Path, Path)] = (includedProtoSplit ++ protoFiles.toList.map { e => ("", e)}).distinct.map { case (repoPath, protoPath) => + // repoPath shoudl refer to the external repo root. If there is _virtual_imports this will + // be in here too. + // + // If virtual imports are present we are going to prefer that for calculating our target + // path. If not we will use relative to the external repo base. + // if not that, just use thhe path we have. + + if(protoPath.contains("_virtual_imports")) { + (Paths.get(protoPath), Paths.get(stripToVirtual(protoPath))) + } else if (repoPath.nonEmpty) { + // We have a repo specified + // if the repo path and the file path are the same, no-op + // otherwise get a relative path. + val relativePath: Path = if(repoPath == protoPath) Paths.get(protoPath) else Paths.get(repoPath).relativize(Paths.get(protoPath)) + (Paths.get(protoPath), relativePath) + } else { + (Paths.get(protoPath), Paths.get(protoPath)) + } + }.toList val flagOpt = args.get(2) match { case "-" => None diff --git a/src/scala/scripts/ScalaPBGenerator.scala b/src/scala/scripts/ScalaPBGenerator.scala index a67ff516a..ffa749c38 100644 --- a/src/scala/scripts/ScalaPBGenerator.scala +++ b/src/scala/scripts/ScalaPBGenerator.scala @@ -35,15 +35,14 @@ object ScalaPBWorker extends GenericWorker(new ScalaPBGenerator) { class ScalaPBGenerator extends Processor { def setupIncludedProto(tmpRoot: Path, 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 = if(root == fullPath) fullPath else root.relativize(fullPath) - val targetPath = tmpRoot.resolve(relativePath) + includedProto.foreach { case (srcPath, relativeTargetPath) => + require(srcPath.toFile.exists, s"Path $srcPath does not exist, which it should as a dependency of this rule") + val targetPath = tmpRoot.resolve(relativeTargetPath) targetPath.toFile.getParentFile.mkdirs - Try(Files.copy(fullPath, targetPath)) match { + Try(Files.copy(srcPath, targetPath)) match { case Failure(err: FileAlreadyExistsException) => - Console.println(s"File already exists, skipping copy from $fullPath to $targetPath: ${err.getMessage}") + Console.println(s"File already exists, skipping copy from $srcPath to $targetPath: ${err.getMessage}") case Failure(err) => throw err case _ => () } From cf5fb631c388006493afa9a9bdadcef6e915a045 Mon Sep 17 00:00:00 2001 From: Ian O'Connell Date: Thu, 19 Sep 2019 09:58:12 -0600 Subject: [PATCH 4/4] Part of last commit missing --- src/scala/scripts/PBGenerateRequest.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scala/scripts/PBGenerateRequest.scala b/src/scala/scripts/PBGenerateRequest.scala index 3bdf488a1..2b1a9660a 100644 --- a/src/scala/scripts/PBGenerateRequest.scala +++ b/src/scala/scripts/PBGenerateRequest.scala @@ -81,6 +81,6 @@ object PBGenerateRequest { } private def padWithProtoPathPrefix(tmpDir: Path)(transitiveProtoPathFlags: List[String]) = - transitiveProtoPathFlags.map(s"--proto_path=${tmpDir.toFile.toString}/"+_).map(_.stripSuffix(".")) + transitiveProtoPathFlags.map(stripToVirtual).map(s"--proto_path=${tmpDir.toFile.toString}/"+_).map(_.stripSuffix(".")) }