FileAlreadyExistsException during proto compilation

[sholokhov]

Hello!

I've tried to use latest rules_scala version (8092d5f6165a8d9c4797d5f089c1ba4eee3326b1) and faced an interesting problem: during proto compilation of independent rules bazel if both are depending on some common protobuf (like timestamp.proto), bazel crashes with FileAlreadyExistsException. If you just rerun the previous command, everything will pass successfully. It looks like there is some kind of conflict (or just forgot to check that file already exists) during creation of _virtual_imports:

sholokhov$ bazel build //shared-libs/protocols:service_events_scala_proto 
INFO: Invocation ID: 216a834e-0e37-4dac-8306-3e6f0f61386a
INFO: Analyzed target //shared-libs/protocols:service_events_scala_proto (96 packages loaded, 1058 targets configured).
INFO: Found 1 target...
INFO: From Linking external/com_google_protobuf/libprotobuf_lite.a [for host]:
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf_lite/io_win32.o has no symbols
INFO: From Linking external/com_google_protobuf/libprotobuf.a [for host]:
/Library/Developer/CommandLineTools/usr/bin/libtool: file: bazel-out/host/bin/external/com_google_protobuf/_objs/protobuf/error_listener.o has no symbols
INFO: From Building external/com_google_protobuf/libprotobuf_java.jar (122 source files, 1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From scala @io_bazel_rules_scala//src/scala/scripts:scalapb_generator_lib [for host]:
warning: there was one deprecation warning
warning: there were two deprecation warnings (since 2.11.0)
warning: there were three deprecation warnings in total; re-run with -deprecation for details
three warnings found
ERROR: /code/shared-libs/protocols/BUILD.bazel:34:1: creating scalapb files //shared-libs/protocols:iam_events_proto failed (Exit 1)
java.nio.file.FileAlreadyExistsException: external/com_google_protobuf/google/protobuf/timestamp.proto
        at java.base/sun.nio.fs.UnixCopyFile.copy(UnixCopyFile.java:573)
        at java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:254)
        at java.base/java.nio.file.Files.copy(Files.java:1294)
        at scripts.ScalaPBGenerator.$anonfun$setupIncludedProto$1(ScalaPBGenerator.scala:42)
        at scala.collection.immutable.List.foreach(List.scala:392)
        at scripts.ScalaPBGenerator.setupIncludedProto(ScalaPBGenerator.scala:37)
        at scripts.ScalaPBGenerator.processRequest(ScalaPBGenerator.scala:53)
        at io.bazel.rulesscala.worker.GenericWorker.runPersistentWorker(GenericWorker.java:45)
        at io.bazel.rulesscala.worker.GenericWorker.run(GenericWorker.java:111)
        at scripts.ScalaPBWorker$.main(ScalaPBGenerator.scala:26)
        at scripts.ScalaPBWorker.main(ScalaPBGenerator.scala)
Target //shared-libs/protocols:service_events_scala_proto failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 56.293s, Critical Path: 32.41s
INFO: 206 processes: 1 remote cache hit, 193 darwin-sandbox, 12 worker.
FAILED: Build did NOT complete successfully

# just rerun previous command

sholokhov$ bazel build //shared-libs/protocols:service_events_scala_proto 
INFO: Invocation ID: 57ca90aa-87b8-445c-b170-98689f1aa259
INFO: Analyzed target //shared-libs/protocols:service_events_scala_proto (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //shared-libs/protocols:service_events_scala_proto up-to-date:
  bazel-bin/shared-libs/protocols/billing_events_proto_scalapb.jar
  bazel-bin/shared-libs/protocols/iam_events_proto_scalapb.jar
INFO: Elapsed time: 12.149s, Critical Path: 11.99s
INFO: 8 processes: 8 worker.
INFO: Build completed successfully, 9 total actions

Bazel 0.27.0.
Just in case, dependency graph:

[sholokhov]

I have a quick fix for that problem, but I'm not 100% sure if it's a valid fix, because it basically fights against symptoms, not the root cause. But it works for me though:

Index: src/scala/scripts/ScalaPBGenerator.scala
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/scala/scripts/ScalaPBGenerator.scala	(revision 8092d5f6165a8d9c4797d5f089c1ba4eee3326b1)
+++ src/scala/scripts/ScalaPBGenerator.scala	(date 1562061165000)
@@ -1,7 +1,7 @@
 package scripts
 
 import java.io.PrintStream
-import java.nio.file.Path
+import java.nio.file.{Path, FileAlreadyExistsException}
 
 import io.bazel.rulesscala.io_utils.DeleteRecursively
 import io.bazel.rulesscala.jar.JarCreator
@@ -12,6 +12,7 @@
 import java.nio.file.{Files, Paths}
 import scalapb.{ScalaPBC, ScalaPbCodeGenerator, ScalaPbcException}
 import java.net.URLClassLoader
+import scala.util.{Try, Failure}
 
 object ScalaPBWorker extends GenericWorker(new ScalaPBGenerator) {
 
@@ -39,7 +40,13 @@
       val relativePath = root.relativize(fullPath)
 
       relativePath.toFile.getParentFile.mkdirs
-      Files.copy(fullPath, relativePath)
+      Try(Files.copy(fullPath, relativePath)) match {
+        case Failure(err) if err.isInstanceOf[FileAlreadyExistsException] =>
+          Console.println(s"File already exists, skipping: ${err.getMessage}")
+        case Failure(err) =>
+          sys.error(err.getMessage)
+        case _ => ()
+      }
     }
   }
   def deleteDir(path: Path): Unit =

[johnynek]

Do you possibly have duplicate classes or protos in different targets somehow?

[sholokhov]

No, only timestamp and wrappers which are shared between two independent protos:

proto_library(
    name = "A",
    srcs = [ "event/A.proto" ],
    deps = [
        "@com_google_protobuf//:wrappers_proto",
        "@com_google_protobuf//:timestamp_proto",
    ],
)

proto_library(
    name = "B",
    srcs = [ "event/B.proto" ],
    deps = [
        "@com_google_protobuf//:timestamp_proto",
        "@com_google_protobuf//:wrappers_proto",
    ],
)

proto_library(
    name = "service_events_proto",
    visibility = [ "//visibility:public" ],
    srcs = [ "event/ServiceEvent.proto" ],
    deps = [
        ":A",
        ":B",
    ],
)

scalapb_proto_library(
    name = "service_events_scala_proto",
    visibility = ["//visibility:public"],
    deps = [ ":service_events_proto" ],
)

[sholokhov]

Sorry for pushing you guys, but is there any thoughts or updates on it?

Now we have to use forked version of this rules with quick fix for the problem, but I would prefer to depend on official rules rather than supporting forked version.

[johnynek]

Could you make a PR with a test that fails? That would be very helpful if we are going to fix it. Thank you for your help!

Cc @ianoc

[ianoc]

I tried quickly to repo this and couldn't, so would love a PR showing the issue

[ptarjan]

After upgrading from bazel 0.26 to 0.27 this issue started to happen very frequently on our repo

[sholokhov]

I've prepared small example which reproduces this issue: https://github.com/sholokhov/bazel-rules-scala-issue

Just try to build everything from scratch (bazel build //...) and it fails with FileAlreadyExistsException.

[ianoc]

Thanks for the repo case @sholokhov, its quite interesting -- so this was something related to a bazel change in 0.27, i tried 0.27 and it fails, pretty handily. But 0.26 (clean expunge + build) in 2 cycle attempts didn't fail. (Not very scientific, overall, but rebuilding proto takes a few mins so its not so much fun)

Question here will be seeing if the approach in the code is actually still safe now or if the virtual imports in 0.28(edit -- actually looks like this change is on master but not in 0.28) maybe make this no longer required at all. bazelbuild/bazel#7157

[ianoc]

It sounds like maybe actions here:

1. try see if the change on bazel master would make this go away
   a) if so we should put a fix in until that release comes out, but that might be testing to see if target file exists, if it does asserting the contents are identical? , also removing any files added when we are done
2. If it doesn't, maybe look closer to see if this is safe going forward. (concern is really if the changes around symlinks could mean its possible to write to the external shared folder and not some scratch space?)

[ptarjan]

@ianoc yay! Thanks for the breakdown. Is that something you're going to work on?

[ianoc]

At some point i will, have a lot on this week and @sholokhov has a workaround that sounds like it'll survive for another few days anyway.

[ptarjan]

@ianoc ok, thank you! I'll switch our repo to point to his fork so I can get our bazel upgrade in then. Does that sound reasonable?

[ianoc]

@ptarjan thats really an internal stripe discussion, but i don't believe generally speaking we'd want our main repos pointing to forks on the internet.

[johnynek]

so, @sholokhov posted a work around:

#779 (comment)

@ptarjan can you make a PR with that diff and we can merge as a workaround and keep this issue open to root cause it?