Switch PluginFrontend to sockets on macOS by bell-db · Pull Request #367 · scalapb/protoc-bridge

bell-db · 2024-08-09T18:37:25Z

What changes are proposed in this pull request?

This PR is stacked on top of #371. Check the last commit for changes.

Fixes #366 by switching from named pipes to sockets on macOS.

Linux continues to use named pipes due to nc availability concerns. See code comment for details.

Windows version is already using sockets so also did some refactoring.

A side benefit is that sockets are actually much faster on macOS (no difference on Linux):

Request 10000 bytes, Response 10000 bytes, Repeat 1000 times
Named pipes on macOS = 33.405s
Sockets on macOS = 15.311s
Named pipes on Linux = 4.223s
Sockets on Linux = 4.219s

How is this tested?

Passes the stress test in #365.

bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala

thesamet · 2024-08-19T03:05:20Z

bridge/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala

        val response = PluginFrontend.runWithInputStream(plugin, fsin, env)
        fsin.close()

+        // Note that the output pipe must be opened after the input pipe is closed.


It's unclear. By "stuck in input pipe" did you mean "stuck reading from the input pipe"? Maybe add more details or remove?

Clarified:

// Note that the output pipe must be opened after the input pipe is consumed. // Otherwise, there might be a deadlock that // - The shell script is stuck writing to the input pipe (which has a full buffer), // and doesn't open the write end of the output pipe. // - This thread is stuck waiting for the write end of the output pipe to be opened.

This was a confusing bug when I tried to do val fsout = newOutputStream; try { fsin ... } finally { fsout.close() }, so feel worth noting here.

thesamet · 2024-08-24T19:46:46Z

Can you run scalafmt? Looks good otherwise.

bell-db · 2024-08-26T17:10:01Z

@thesamet Yeah formatting are fixed now.

thesamet · 2024-08-27T18:32:36Z

Thanks, Bell!

Added a couple more repo dependencies to targets under //test. However, `bazel test //test:all` is a bust for now, because the protoc-bridge mechanism hangs on macOS: - scalapb/protoc-bridge#366 - scalapb/protoc-bridge#367 - scalapb/protoc-bridge#379 - scalapb/protoc-bridge#380 Though scalapb/protoc-bridge#366 suggests that it only hangs occasionally, it's hanging 100% of the time for me on macOS 15.0 on a M3 Max.

This version contains this update to `protocbridge.frontend.PluginFrontend.runWithInputStream()`: - https://github.com/scalapb/protoc-bridge/blob/c574d50eaee5b800fd54493fe25c4e0eed3b9def/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 This change originally came from: - https://github.com/scalapb/protoc-bridge/blob/d0d56f635d13f7efaa2755ed0d2d66bdef18b588/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 - scalapb/protoc-bridge#367 This should close scalapb/ScalaPB#1771 and obviate the need for the `try`/`catch` blocks added in bazel-contrib#1630 and bazel-contrib#1637, and the `ScalaPBCodeGenerator` implementation added in bazel-contrib#1648. The update requires special handling in `scripts/create_repository.py` to ensure that only Scala 3.3 and up receive the `protoc-bridge_3` version. Scala 3.1 and 3.2 still receive the `protoc-bridge_2.13` version. However, the `protoc-bridge_3` version currently fails with the following (setting `RULES_SCALA_TEST_ONLY="test_scala_version 3.3.4"` fails the same way): ```txt $ RULES_SCALA_TEST_ONLY="test_scala_version 3.6.2" \ ./test_thirdparty_version.sh ERROR: third_party/test/proto/BUILD.bazel:4:14: ProtoScalaPBRule third_party/test/proto/proto_jvm_extra_protobuf_generator_scalapb.srcjar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) --scala_out: java.lang.NoClassDefFoundError: Could not initialize class scalapb.ScalaPbCodeGenerator$ at scripts.ScalaPbCodeGenerator$.process(ScalaPbCodeGeneratorWrapper.scala:8) at protocgen.CodeGenApp.run(CodeGenApp.scala:48) at protocgen.CodeGenApp.run$(CodeGenApp.scala:41) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocgen.CodeGenApp.run(CodeGenApp.scala:33) at protocgen.CodeGenApp.run$(CodeGenApp.scala:32) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:48) at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:113) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$1(SocketBasedPluginFrontend.scala:31) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:37) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1$$anon$2.block(ExecutionContextImpl.scala:60) at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3118) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1.blockOn(ExecutionContextImpl.scala:71) at scala.concurrent.package$.blocking(package.scala:124) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1(SocketBasedPluginFrontend.scala:37) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:38) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:687) at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) java.lang.RuntimeException: Exit with code 1 at scala.sys.package$.error(package.scala:27) at scripts.ScalaPBWorker$.work(ScalaPBWorker.scala:44) at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:96) at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49) at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39) at scripts.ScalaPBWorker.main(ScalaPBWorker.scala) ERROR: third_party/test/proto/BUILD.bazel:4:14 Building source jar third_party/test/proto/proto_scalapb-src.jar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) ```

This required updates to `scripts/create_repository.py` to accommodate comparisons between `_2.13` and `_3` versions of ScalaPB artifacts. The `_3` versions of the most recent ScalaPB artifacts are only available for Scala 3.3 and later, and `compilerplugin_3` currently still depends on `protoc-gen_2.13`. The `MavenCoordinates` dataclass in `scripts/create_repository.py` has new `unversioned_artifact`, `scala_version`, and `artifact_name` members. Updates to `MavenCoordinates.is_newer_than()` now compare objects with matching `artifact_name` values, and take the `scala_version` into account. These changes then precipitated: - Replacing the `artifact_name()` call with `artifact_name` accesses. - Replacing `ArtifactLabelMaker._remove_scala_version_suffix` with `MavenCoordinates.unversioned_artifact`. - Extracting `__compare_versions` from `is_newer_than` to reuse the same code for comparing Scala versions and artifact versions. (Properly reversed the use of `lhs` and `rhs` in the process.) This change also removes an unnecessary `try`/`catch` block from `scripts.ScalaPbCodeGenerator.process()` for Scala 2.12 and above. The block is still required for the Scala 2.11 implementation of `scripts.ScalaPbCodeGenerator.run()` and in `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run()`. Finally, this change adds notes in a few places to indicate where support remains for Scala 2.11. This supporting code can ultimately be removed if we ever decide to drop Scala 2.11 support. --- Tested by setting the `protobuf` dependencies to a versions I know will break `ScalaPB` 0.11.17: - `abseil-cpp`: 20220623.1 => 20240722.0 - `protobuf`: v21.7 => v26.1 I temporarily removed the `try`/`catch` block for `Throwable` from `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run`. I then ran `bazel shutdown` to stop persistent `ProtoScalaPBRule` workers. After that, building with Bazel 7.4.1 (since Bazel 6.5.0 can't build `abseil-cpp` 20240722.0 without setting C++14 compiler flags) crashed as expected, instead of hanging. Undoing the `abseil-cpp` and `protobuf` changes and building then produced a working build. ```txt $ USE_BAZEL_VERSION=7.4.1 bazel build //third_party/test/proto:scala [ ...snip... ] ERROR: third_party/test/proto/BUILD.bazel:4:14: ProtoScalaPBRule third_party/test/proto/proto_jvm_extra_protobuf_generator_scalapb.srcjar failed: (Exit 1): scalapb_worker failed: error executing ProtoScalaPBRule command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) --jvm_extra_protobuf_generator_out: java.lang.VerifyError: Cannot inherit from final class at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1022) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800) at java.base/jdk.internal.loader.BuiltinClassLoader$4.run(BuiltinClassLoader.java:711) at java.base/jdk.internal.loader.BuiltinClassLoader$4.run(BuiltinClassLoader.java:706) at java.base/java.security.AccessController.doPrivileged(Native Method) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:719) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:527) at scalapb.options.Scalapb.<clinit>(Scalapb.java:24835) at scalapb.options.compiler.Scalapb$.registerAllExtensions(Scalapb.scala:8) at scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator$.run(ExtraProtobufGenerator.scala:52) at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:48) at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:113) at protocbridge.frontend.SocketBasedPluginFrontend.$anonfun$prepare$2(SocketBasedPluginFrontend.scala:31) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1$$anon$2.block(ExecutionContextImpl.scala:75) at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3118) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1.blockOn(ExecutionContextImpl.scala:87) at scala.concurrent.package$.blocking(package.scala:146) at protocbridge.frontend.SocketBasedPluginFrontend.$anonfun$prepare$1(SocketBasedPluginFrontend.scala:23) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:659) at scala.util.Success.$anonfun$map$1(Try.scala:255) at scala.util.Success.map(Try.scala:213) at scala.concurrent.Future.$anonfun$map$1(Future.scala:292) at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:42) at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:74) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) java.lang.RuntimeException: Exit with code 1 at scala.sys.package$.error(package.scala:30) at scripts.ScalaPBWorker$.work(ScalaPBWorker.scala:44) at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:96) at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49) at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39) at scripts.ScalaPBWorker.main(ScalaPBWorker.scala) Target //third_party/test/proto:scala failed to build ERROR: third_party/test/proto/BUILD.bazel:4:14 scala @//third_party/test/proto:proto failed: (Exit 1): scalapb_worker failed: error executing ProtoScalaPBRule command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) ``` --- The initial motivation for this change was to try to eliminate the need for the custom `scripts.ScalaPbCodeGenerator` implementation by upgrading to `protoc-bridge` 0.9.8. This version contains a change to `catch` any `Throwable` objects raised by a generator implementation in `protocbridge.frontend.PluginFrontend.runWithInputStream()`. - https://github.com/scalapb/protoc-bridge/blob/c574d50eaee5b800fd54493fe25c4e0eed3b9def/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 This change originally came from: - https://github.com/scalapb/protoc-bridge/blob/d0d56f635d13f7efaa2755ed0d2d66bdef18b588/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 - scalapb/protoc-bridge#367 I closed scalapb/ScalaPB#1771 as a result, and reexamined the `try`/`catch` blocks added in bazel-contrib#1630 and bazel-contrib#1637, and the `scripts.ScalaPBCodeGenerator` implementations added in bazel-contrib#1648. However, the last `protoc-bridge` version to support Scala 2.11 is 0.7.14. Scala 2.11 won't be able to use `protoc-bridge` 0.9.8, so we still need to `catch` any `Throwable`s ourselves. This includes the `catch` blocks from the Scala 2.11 `scripts.ScalaPbCodeGenerator` and `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run`. --- In `protoc-bridge` 0.9.7 and 0.9.8, `protocbridge.ProtocCodeGenerator` declares a `run()` method with no implementation (and no `try`/`catch` block). - https://github.com/scalapb/protoc-bridge/blob/v0.9.7/bridge/src/main/scala/protocbridge/ProtocCodeGenerator.scala#L5 In `protoc-gen` 0.9.7 and 0.9.8, `protogen.CodeGenApp` implements `protocbridge.ProtocCodeGenerator`, and `CodeGenApp.run()` wraps a call to `CodeGenApp.process()` inside a `try`/`catch` block. - https://github.com/scalapb/protoc-bridge/blob/v0.9.7/protoc-gen/src/main/scala/protocgen/CodeGenApp.scala#L41-L56 `scalapb.ScalaPbCodeGenerator` from `compilerplugin` 0.11.17 extends `CodeGenApp` and implements `process()`. For this version of `ScalaPbCodeGenerator`, available in Scala 2.12 and later, there's no need for a `try`/`catch` wrapper on our end. - https://github.com/scalapb/ScalaPB/blob/v0.11.17/compiler-plugin/src/main/scala/scalapb/ScalaPbCodeGenerator.scala#L11 However, the last available `compilerplugin` version for Scala 2.11 is 0.9.8. `scalapb.ScalaPbCodeGenerator` from that version implements `protocbridge.ProtocCodeGenerator` and has a `try`/`catch` in its `run()` method. However, the `Scalapb.registerAllExtensions(registry)` call on line 14 lies outside this block, producing the crash described in bazel-contrib#1648 (commit 23ae356). This is why we need the Scala 2.11 implementation of `scripts.ScalaPbCodeGenerator`. - https://github.com/scalapb/ScalaPB/blob/v0.9.8/compiler-plugin/src/main/scala/scalapb/ScalaPbCodeGenerator.scala#L14 `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator`, also extends `ProtocCodeGenerator` directly, instead of implementing `CodeGenApp` (so it also builds with Scala 2.11). This is why `ExtraProtobufGenerator.run()` hangs when we remove the `try`/`catch` block and run with `protoc-bridge` 0.9.7 and `protobuf` > v25.5, even with later Scala versions. --- Since `scalapb.ScalaPbCodeGenerator` from `compilerplugin` 0.11.17 implements `CodeGenApp`, `scripts.ScalaPbCodeGenerator` for Scala 2.12 should be unnecessary. The only reason we need it is to maintain the same interface as the Scala 2.11 implementation. --- Before the `MavenCoordinates` changes, `scripts/create_repository.py` would ScalaPB flip ScalaPB artifacts between their `_2.13` and `_3` versions on subsequent runs (e.g., `protoc-bridge_2.13` vs. `protoc-bridge_3`). See the comments added within `MavenCoordinates.new()` for details. --- Building Scala 3.3 and later versions with `compilerplugin_3` and `protoc-bridge_3` artifacts (instead of `protoc-bridge_2.13` artifacts) produces the following build failure: ```txt $ RULES_SCALA_TEST_ONLY="test_scala_version 3.6.2" \ ./test_thirdparty_version.sh [ ...snip... ] ERROR: third_party/test/proto/BUILD.bazel:4:14: ProtoScalaPBRule third_party/test/proto/proto_jvm_extra_protobuf_generator_scalapb.srcjar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) --scala_out: java.lang.NoClassDefFoundError: Could not initialize class scalapb.ScalaPbCodeGenerator$ at scripts.ScalaPbCodeGenerator$.process(ScalaPbCodeGeneratorWrapper.scala:8) at protocgen.CodeGenApp.run(CodeGenApp.scala:48) at protocgen.CodeGenApp.run$(CodeGenApp.scala:41) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocgen.CodeGenApp.run(CodeGenApp.scala:33) at protocgen.CodeGenApp.run$(CodeGenApp.scala:32) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:48) at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:113) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$1(SocketBasedPluginFrontend.scala:31) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:37) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1$$anon$2.block(ExecutionContextImpl.scala:60) at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3118) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1.blockOn(ExecutionContextImpl.scala:71) at scala.concurrent.package$.blocking(package.scala:124) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1(SocketBasedPluginFrontend.scala:37) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:38) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:687) at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) java.lang.RuntimeException: Exit with code 1 at scala.sys.package$.error(package.scala:27) at scripts.ScalaPBWorker$.work(ScalaPBWorker.scala:44) at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:96) at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49) at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39) at scripts.ScalaPBWorker.main(ScalaPBWorker.scala) ERROR: third_party/test/proto/BUILD.bazel:4:14 Building source jar third_party/test/proto/proto_scalapb-src.jar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) ```

This required updates to `scripts/create_repository.py` to accommodate comparisons between `_2.13` and `_3` versions of ScalaPB artifacts. The `_3` versions of the most recent ScalaPB artifacts are only available for Scala 3.3 and later, and `compilerplugin_3` currently still depends on `protoc-gen_2.13`. The `MavenCoordinates` dataclass in `scripts/create_repository.py` has new `unversioned_artifact`, `scala_version`, and `artifact_name` members. Updates to `MavenCoordinates.is_newer_than()` now compare objects with matching `artifact_name` values, and take the `scala_version` into account. These changes then precipitated: - Replacing the `artifact_name()` call with `artifact_name` accesses. - Replacing `ArtifactLabelMaker._remove_scala_version_suffix` with `MavenCoordinates.unversioned_artifact`. - Extracting `__compare_versions` from `is_newer_than` to reuse the same code for comparing Scala versions and artifact versions. (Properly reversed the use of `lhs` and `rhs` in the process.) This change also removes an unnecessary `try`/`catch` block from `scripts.ScalaPbCodeGenerator.process()` for Scala 2.12 and above. The block is still required for the Scala 2.11 implementation of `scripts.ScalaPbCodeGenerator.run()` and in `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run()`. Finally, this change adds notes in a few places to indicate where support remains for Scala 2.11. This supporting code can ultimately be removed if we ever decide to drop Scala 2.11 support. --- Tested by setting the `protobuf` dependencies to a versions I know will break `ScalaPB` 0.11.17: - `abseil-cpp`: 20220623.1 => 20240722.0 - `protobuf`: v21.7 => v26.1 I temporarily removed the `try`/`catch` block for `Throwable` from `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run`. I then ran `bazel shutdown` to stop persistent `ProtoScalaPBRule` workers. After that, building with Bazel 7.4.1 (since Bazel 6.5.0 can't build `abseil-cpp` 20240722.0 without setting C++14 compiler flags) crashed as expected, instead of hanging. Undoing the `abseil-cpp` and `protobuf` changes and building then produced a working build. ```txt $ USE_BAZEL_VERSION=7.4.1 bazel build //third_party/test/proto:scala [ ...snip... ] ERROR: third_party/test/proto/BUILD.bazel:4:14: ProtoScalaPBRule third_party/test/proto/proto_jvm_extra_protobuf_generator_scalapb.srcjar failed: (Exit 1): scalapb_worker failed: error executing ProtoScalaPBRule command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) --jvm_extra_protobuf_generator_out: java.lang.VerifyError: Cannot inherit from final class at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1022) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800) at java.base/jdk.internal.loader.BuiltinClassLoader$4.run(BuiltinClassLoader.java:711) at java.base/jdk.internal.loader.BuiltinClassLoader$4.run(BuiltinClassLoader.java:706) at java.base/java.security.AccessController.doPrivileged(Native Method) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:719) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:527) at scalapb.options.Scalapb.<clinit>(Scalapb.java:24835) at scalapb.options.compiler.Scalapb$.registerAllExtensions(Scalapb.scala:8) at scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator$.run(ExtraProtobufGenerator.scala:52) at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:48) at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:113) at protocbridge.frontend.SocketBasedPluginFrontend.$anonfun$prepare$2(SocketBasedPluginFrontend.scala:31) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1$$anon$2.block(ExecutionContextImpl.scala:75) at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3118) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1.blockOn(ExecutionContextImpl.scala:87) at scala.concurrent.package$.blocking(package.scala:146) at protocbridge.frontend.SocketBasedPluginFrontend.$anonfun$prepare$1(SocketBasedPluginFrontend.scala:23) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:659) at scala.util.Success.$anonfun$map$1(Try.scala:255) at scala.util.Success.map(Try.scala:213) at scala.concurrent.Future.$anonfun$map$1(Future.scala:292) at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:42) at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:74) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) java.lang.RuntimeException: Exit with code 1 at scala.sys.package$.error(package.scala:30) at scripts.ScalaPBWorker$.work(ScalaPBWorker.scala:44) at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:96) at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49) at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39) at scripts.ScalaPBWorker.main(ScalaPBWorker.scala) Target //third_party/test/proto:scala failed to build ERROR: third_party/test/proto/BUILD.bazel:4:14 scala @//third_party/test/proto:proto failed: (Exit 1): scalapb_worker failed: error executing ProtoScalaPBRule command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) ``` --- The initial motivation for this change was to try to eliminate the need for the custom `scripts.ScalaPbCodeGenerator` implementation by upgrading to `protoc-bridge` 0.9.8. This version contains a change to `catch` any `Throwable` objects raised by a generator implementation in `protocbridge.frontend.PluginFrontend.runWithInputStream()`. - https://github.com/scalapb/protoc-bridge/blob/c574d50eaee5b800fd54493fe25c4e0eed3b9def/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 This change originally came from: - https://github.com/scalapb/protoc-bridge/blob/d0d56f635d13f7efaa2755ed0d2d66bdef18b588/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala#L107-L122 - scalapb/protoc-bridge#367 I closed scalapb/ScalaPB#1771 as a result, and reexamined the `try`/`catch` blocks added in #1630 and #1637, and the `scripts.ScalaPBCodeGenerator` implementations added in #1648. However, the last `protoc-bridge` version to support Scala 2.11 is 0.7.14. Scala 2.11 won't be able to use `protoc-bridge` 0.9.8, so we still need to `catch` any `Throwable`s ourselves. This includes the `catch` blocks from the Scala 2.11 `scripts.ScalaPbCodeGenerator` and `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator.run`. --- In `protoc-bridge` 0.9.7 and 0.9.8, `protocbridge.ProtocCodeGenerator` declares a `run()` method with no implementation (and no `try`/`catch` block). - https://github.com/scalapb/protoc-bridge/blob/v0.9.7/bridge/src/main/scala/protocbridge/ProtocCodeGenerator.scala#L5 In `protoc-gen` 0.9.7 and 0.9.8, `protogen.CodeGenApp` implements `protocbridge.ProtocCodeGenerator`, and `CodeGenApp.run()` wraps a call to `CodeGenApp.process()` inside a `try`/`catch` block. - https://github.com/scalapb/protoc-bridge/blob/v0.9.7/protoc-gen/src/main/scala/protocgen/CodeGenApp.scala#L41-L56 `scalapb.ScalaPbCodeGenerator` from `compilerplugin` 0.11.17 extends `CodeGenApp` and implements `process()`. For this version of `ScalaPbCodeGenerator`, available in Scala 2.12 and later, there's no need for a `try`/`catch` wrapper on our end. - https://github.com/scalapb/ScalaPB/blob/v0.11.17/compiler-plugin/src/main/scala/scalapb/ScalaPbCodeGenerator.scala#L11 However, the last available `compilerplugin` version for Scala 2.11 is 0.9.8. `scalapb.ScalaPbCodeGenerator` from that version implements `protocbridge.ProtocCodeGenerator` and has a `try`/`catch` in its `run()` method. However, the `Scalapb.registerAllExtensions(registry)` call on line 14 lies outside this block, producing the crash described in #1648 (commit 23ae356). This is why we need the Scala 2.11 implementation of `scripts.ScalaPbCodeGenerator`. - https://github.com/scalapb/ScalaPB/blob/v0.9.8/compiler-plugin/src/main/scala/scalapb/ScalaPbCodeGenerator.scala#L14 `scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator`, also extends `ProtocCodeGenerator` directly, instead of implementing `CodeGenApp` (so it also builds with Scala 2.11). This is why `ExtraProtobufGenerator.run()` hangs when we remove the `try`/`catch` block and run with `protoc-bridge` 0.9.7 and `protobuf` > v25.5, even with later Scala versions. --- Since `scalapb.ScalaPbCodeGenerator` from `compilerplugin` 0.11.17 implements `CodeGenApp`, `scripts.ScalaPbCodeGenerator` for Scala 2.12 should be unnecessary. The only reason we need it is to maintain the same interface as the Scala 2.11 implementation. --- Before the `MavenCoordinates` changes, `scripts/create_repository.py` would ScalaPB flip ScalaPB artifacts between their `_2.13` and `_3` versions on subsequent runs (e.g., `protoc-bridge_2.13` vs. `protoc-bridge_3`). See the comments added within `MavenCoordinates.new()` for details. --- Building Scala 3.3 and later versions with `compilerplugin_3` and `protoc-bridge_3` artifacts (instead of `protoc-bridge_2.13` artifacts) produces the following build failure: ```txt $ RULES_SCALA_TEST_ONLY="test_scala_version 3.6.2" \ ./test_thirdparty_version.sh [ ...snip... ] ERROR: third_party/test/proto/BUILD.bazel:4:14: ProtoScalaPBRule third_party/test/proto/proto_jvm_extra_protobuf_generator_scalapb.srcjar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) --scala_out: java.lang.NoClassDefFoundError: Could not initialize class scalapb.ScalaPbCodeGenerator$ at scripts.ScalaPbCodeGenerator$.process(ScalaPbCodeGeneratorWrapper.scala:8) at protocgen.CodeGenApp.run(CodeGenApp.scala:48) at protocgen.CodeGenApp.run$(CodeGenApp.scala:41) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocgen.CodeGenApp.run(CodeGenApp.scala:33) at protocgen.CodeGenApp.run$(CodeGenApp.scala:32) at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper.scala:5) at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:48) at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:113) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$1(SocketBasedPluginFrontend.scala:31) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:37) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1$$anon$2.block(ExecutionContextImpl.scala:60) at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3118) at scala.concurrent.impl.ExecutionContextImpl$DefaultThreadFactory$$anon$1.blockOn(ExecutionContextImpl.scala:71) at scala.concurrent.package$.blocking(package.scala:124) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$1(SocketBasedPluginFrontend.scala:37) at protocbridge.frontend.SocketBasedPluginFrontend.prepare$$anonfun$adapted$1(SocketBasedPluginFrontend.scala:38) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:687) at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) java.lang.RuntimeException: Exit with code 1 at scala.sys.package$.error(package.scala:27) at scripts.ScalaPBWorker$.work(ScalaPBWorker.scala:44) at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:96) at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49) at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39) at scripts.ScalaPBWorker.main(ScalaPBWorker.scala) ERROR: third_party/test/proto/BUILD.bazel:4:14 Building source jar third_party/test/proto/proto_scalapb-src.jar failed: (Exit 1): scalapb_worker failed: error executing command (from target //third_party/test/proto:proto) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/src/scala/scripts/scalapb_worker ... (remaining 2 arguments skipped) ```

bell-db force-pushed the bell-db/v0.9.7-socket branch from 1ed4511 to 5a14de2 Compare August 15, 2024 16:30

nadavsamet-db reviewed Aug 15, 2024

View reviewed changes

bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala Outdated Show resolved Hide resolved

bell-db force-pushed the bell-db/v0.9.7-socket branch 3 times, most recently from c0a82ee to 23fb12c Compare August 17, 2024 00:07

bell-db mentioned this pull request Aug 17, 2024

Handle all failures in runWithInputStream #371

Closed

bell-db changed the title ~~[WIP] Switch PosixPluginFrontend to sockets~~ Switch PosixPluginFrontend to sockets on macOS Aug 17, 2024

bell-db changed the title ~~Switch PosixPluginFrontend to sockets on macOS~~ Switch PluginFrontend to sockets on macOS Aug 17, 2024

bell-db force-pushed the bell-db/v0.9.7-socket branch from 23fb12c to 2949802 Compare August 17, 2024 00:53

bell-db marked this pull request as ready for review August 17, 2024 00:54

thesamet reviewed Aug 19, 2024

View reviewed changes

bell-db force-pushed the bell-db/v0.9.7-socket branch from 2949802 to c9456b9 Compare August 20, 2024 18:12

bell-db requested review from nadavsamet-db and thesamet August 21, 2024 16:28

bell-db force-pushed the bell-db/v0.9.7-socket branch 2 times, most recently from 1a723ee to db49de0 Compare August 21, 2024 17:24

bell-db added 3 commits August 26, 2024 09:58

Add PosixPluginFrontendSpec

7953a64

Handle all failures in Future

c576aed

Switch PluginFrontend to sockets on macOS

0ba6fcf

bell-db force-pushed the bell-db/v0.9.7-socket branch from db49de0 to 0ba6fcf Compare August 26, 2024 17:09

thesamet merged commit c574d50 into scalapb:master Aug 27, 2024

This was referenced Sep 17, 2024

[TEST][NO-MERGE] Stress test sockets bell-db/protoc-bridge#2

Draft

Sockets aren't 100% reliable on macOS #379

Open

[TEST][NO-MERGE] Stress test sockets #381

Draft

This was referenced Nov 22, 2024

protoc-bridge builds hang when scalapb.ScalaPbCodeGenerator doesn't catch Throwable scalapb/ScalaPB#1771

Closed

Support Bzlmod and add rules_scala to bazel-central-registry bazel-contrib/rules_scala#1482

Closed

mbland mentioned this pull request Dec 20, 2024

Protobuf compatibility findings and suggestions bazel-contrib/rules_scala#1647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch PluginFrontend to sockets on macOS#367

Switch PluginFrontend to sockets on macOS#367
thesamet merged 3 commits intoscalapb:masterfrom
bell-db:bell-db/v0.9.7-socket

bell-db commented Aug 9, 2024 •

edited

Loading

Uh oh!

Uh oh!

thesamet Aug 19, 2024 •

edited

Loading

Uh oh!

bell-db Aug 20, 2024 •

edited

Loading

Uh oh!

thesamet commented Aug 24, 2024

Uh oh!

bell-db commented Aug 26, 2024

Uh oh!

thesamet commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bell-db commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How is this tested?

Uh oh!

Uh oh!

thesamet Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bell-db Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesamet commented Aug 24, 2024

Uh oh!

bell-db commented Aug 26, 2024

Uh oh!

thesamet commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bell-db commented Aug 9, 2024 •

edited

Loading

thesamet Aug 19, 2024 •

edited

Loading

bell-db Aug 20, 2024 •

edited

Loading