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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b768b78
Fix paths in proto_path and avoid copying
ignasl Sep 23, 2019
1c146e0
Prepare mapping in advance
ignasl Sep 26, 2019
7e30a41
Condensed all transformations into one method
ignasl Sep 27, 2019
ba3f9cf
Added tests
ignasl Sep 27, 2019
4b03fde
Buildifier corrections
ignasl Sep 30, 2019
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
23 changes: 21 additions & 2 deletions src/scala/scripts/PBGenerateRequest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,25 @@ 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)])(orig: String): String = includedProto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we store the first map through the list:

def fixTransitiveProtoPath(includedProto: List[(Path, Path)]): String => String = {
  val relPath = includedProto.map { case (root, full) => (root.toString, root.relativize(full).toString) }

  { orig: String =>
    relPath.find { case (_, rel) => rel.startsWith(orig) } match {
       case Some((root, _)) => s"$root/$orig"
       case None => orig
    }
  } 
}

This is a bit longer, but it builds the list and converts to strings once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Optimized.

.map { case (root, full) => (root, root.relativize(full)) }
.find { case (_, rel) => rel.toString.startsWith(orig) }
.map { case (root, _) => root.toString + "/" + orig }
.getOrElse(orig)

def from(args: java.util.List[String]): PBGenerateRequest = {
val jarOutput = args.get(0)
val protoFiles = args.get(4).split(':')
Expand All @@ -27,13 +46,13 @@ 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(".")
}).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
View file
Open in desktop
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