Copy the protos we need into a temp folder #848
Conversation
|
fixes #843 |
|
Thanks for tackling! looks like legit test failures. |
| 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} |
There was a problem hiding this comment.
this looks like debug. Can we remove?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
oh yeah... I overlooked that. nit: can we do foreach(System.out.println) (use parens rather than {} when passing a method as a function argument)?
| }.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)}"} |
There was a problem hiding this comment.
nit, you don't need .toString inside the interpolation. Also, I think just $tmpDir/${stripToVirtual(e)} should work. Why do we need to convert to File?
| 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(',') |
There was a problem hiding this comment.
want to add a require(arr.length == 2, s"expected exactly two comma separated items in ${arr.toList}") just to guard against anything funny changing later?
| 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) => |
There was a problem hiding this comment.
nit: can we make this line not quite so long? Maybe newline after the ) concatenating the two lists?
| (Paths.get(protoPath), relativePath) | ||
| } else { | ||
| (Paths.get(protoPath), Paths.get(protoPath)) | ||
| } | ||
| }.toList |
| @@ -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("") | |||
There was a problem hiding this comment.
::: List("") I think is a bit more efficient on lists than ++ but maybe not...
| 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} |
There was a problem hiding this comment.
oh yeah... I overlooked that. nit: can we do foreach(System.out.println) (use parens rather than {} when passing a method as a function argument)?
There was a problem hiding this comment.
Thanks for tackling this!
I'm approving this but to be honest I feel I don't fully understand the change and I feel there is a lot of noise in the current implementation.
@lberki @Yannic I wonder if maybe we need to do some collaboration here since it seems to me we can either improve Bazel by identifying (and later removing) proto related boilerplate we needed to do here, improve rules_scala by dropping old/unneeded/mistaken workarounds we have, or even improve both. WDYT?
|
I agree that writing these rules isn't as easy as it could be. We have a design doc in review that proposes some helper functions to improve this situation (cmp. p 3-5), however they assume that the generator is a protoc plugin. From what I've seen, |
|
Specifically for the issue #843 and our own build problems I might have a smaller fix that actually avoids copying altogether. I will send a PR for consideration. |
|
See: #850 |
|
Closing in favor of #850 |
Description
We have errors around colliding files. The copying is putting things into trees that aren't part of the sandbox per build.
Eventually i believe the virtual_imports work will be a nicer solution to this, but from what i can tell part of it is still behind a flag. I wasn't quite sure exactly how to implement it all using that today. So for now instead of copying some stuff around and maybe colliding. Just copy everything into a temp location.
Also we weren't getting any error messages from protoc when run for scalapb so i made some changes here to redirect the output to a file and reply it back so we now get the error messages
Motivation
Errors/problems in the copying approach