-
-
Notifications
You must be signed in to change notification settings - Fork 287
Copy the protos we need into a temp folder #848
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,16 +6,44 @@ case class PBGenerateRequest(jarOutput: String, scalaPBOutput: Path, scalaPBArgs | |
|
|
||
| object PBGenerateRequest { | ||
|
|
||
| def from(args: java.util.List[String]): 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 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)) | ||
| 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(',') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. want to add a |
||
| (arr(0), arr(1)) | ||
| }.toList | ||
| val includedProto: List[(Path, Path)] = (includedProtoSplit ++ protoFiles.toList.map { e => ("", e)}).distinct.map { case (repoPath, protoPath) => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we make this line not quite so long? Maybe newline after the |
||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't this already a list? |
||
|
|
||
| val flagOpt = args.get(2) match { | ||
|
|
@@ -27,7 +55,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("") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp")) | ||
| val scalaPBOutput = Files.createTempDirectory(tmp, "bazelscalapb") | ||
|
|
@@ -43,15 +71,16 @@ 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 | ||
|
|
||
| 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(stripToVirtual).map(s"--proto_path=${tmpDir.toFile.toString}/"+_).map(_.stripSuffix(".")) | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,15 +34,15 @@ 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) | ||
| def setupIncludedProto(tmpRoot: Path, includedProto: List[(Path, Path)]): Unit = { | ||
| 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) | ||
|
|
||
| relativePath.toFile.getParentFile.mkdirs | ||
| Try(Files.copy(fullPath, relativePath)) match { | ||
| targetPath.toFile.getParentFile.mkdirs | ||
| Try(Files.copy(srcPath, targetPath)) match { | ||
| case Failure(err: FileAlreadyExistsException) => | ||
| Console.println(s"File already exists, skipping: ${err.getMessage}") | ||
| Console.println(s"File already exists, skipping copy from $srcPath to $targetPath: ${err.getMessage}") | ||
| case Failure(err) => throw err | ||
| case _ => () | ||
| } | ||
|
|
@@ -55,8 +55,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 | ||
|
|
@@ -89,10 +91,21 @@ class ScalaPBGenerator extends Processor { | |
| } | ||
| JarCreator.buildJar(Array(extractRequestResult.jarOutput, extractRequestResult.scalaPBOutput.toString)) | ||
| } finally { | ||
| deleteDir(tmpRoot) | ||
| deleteDir(extractRequestResult.scalaPBOutput) | ||
| } | ||
| } | ||
|
|
||
| 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} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks like debug. Can we remove?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how we get the stdout/stderr from protoc into the bazel output stream. So can't remove. (prior to this PR any failure in protoc would result in just seeing a message that it exited with code 1 and no further info)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah... I overlooked that. nit: can we do |
||
| } | ||
| ret | ||
| } finally { | ||
| tmpFile.toFile.delete | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, you don't need
.toStringinside the interpolation. Also, I think just$tmpDir/${stripToVirtual(e)}should work. Why do we need to convert to File?