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.

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

Bump ScalaPB, protoc-bridge, gRPC, and Guava deps, and add ProtobufAdapters and ScalaPBCodeGenerator wrappers #1648

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Nov 12, 2024

Description

Adds ScalaPB 0.11.17, protoc-bridge 0.9.8, gRPC 1.68.1, and Guava 33.3.1-jre as root artifacts in scripts/create_repository.py and ProtobufAdapters for cross Scala version compatibility. Adds ScalaPBCodeGenerator wrappers to ensure all ScalaPB workers catch all exceptions to avoid build hangs when updating the Protobuf library.

This change does not bump the Protobuf library version, which can happen in another PR and is up for discussion in #1647. As always, I'm happy to break this apart, at least into one "version bump and compatibility updates" PR and one "wrap ScalaPBCodeGenerator" PR, if so desired.

There are lots of details in the individual commit messages.

Motivation

These changes are spawned from #1482 and #1647, whereby I investigated what it would take to update the Protobuf version to accommodate future Bzlmod users that depend on newer Protobuf versions.

These changes were essential to the Protobuf experiment, yet aren't necessarily essential to the Bzlmod work, but they're generally beneficial to include for their own sake. All builds and tests are still compatible with all supported Scala versions, on Bazel 6 and Bazel 7.

This is also a good demonstration of how to add new root artifact dependencies to scripts/create_repository.py, and how easy it is to run the script and get correct updates across all supported Scala versions.

Regarding the ScalaPBCodeGenerator wrappers, I encountered more hanging builds in my bzlmod-update-abseil-cpp-and-protobuf working branch that the changes from #1637 didn't catch. I backported Wrap ScalaPbCodeGenerator, expose Scala 2.11 error from that branch into this one.

See that commit's message and #1647 for the gory details on why the ScalaPB libs are so sensitive to the Protobuf version and why we have to use wrappers to avoid hanging the build.

Adds these versions to `scripts/create_repository.py`.

There are breakages still that future commits will address, but the
following are fixed in this one.

Removes the unnecessary `true` parameter from `.getPlaintext(true)` in
`test/TestServer.scala` to fix this error after the gRPC bump:

```txt
ERROR: .../test/BUILD:676:14:
  scala @//test:lib_with_scala_proto_dep failed:
  (Exit 1): scalac failed: error executing command
  (from target //test:lib_with_scala_proto_dep)
  bazel-bin/src/java/io/bazel/rulesscala/scalac/scalac
    @bazel-out/darwin_arm64-fastbuild/bin/test/lib_with_scala_proto_dep.jar-0.params

test/TestServer.scala:70: error: no arguments allowed for nullary method usePlaintext: ()?0
    val channel = ManagedChannelBuilder.forAddress(host, port).usePlaintext(true).build
                                                                            ^
```

We exclude `com.google.guava:listenablefuture` because trying to
download it breaks the build with an HTTP 404 (everything builds fine
without it anyway):

```txt
INFO: repository @io_bazel_rules_scala_listenablefuture' used the
  following cache hits instead of downloading the corresponding file.
  * Hash '...' for https://repo.maven.apache.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar

WARNING: Download from
  https://repo.maven.apache.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar
  failed: class java.io.FileNotFoundException GET returned 404 Not Found

WARNING: Download from
  https://maven-central.storage-download.googleapis.com/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar
  failed: class java.io.FileNotFoundException GET returned 404 Not Found

WARNING: Download from
  https://mirror.bazel.build/repo1.maven.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar
  failed: class java.io.FileNotFoundException GET returned 404 Not Found

WARNING: Download from
https://jcenter.bintray.com/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar
  failed: class java.io.FileNotFoundException GET returned 404 Not Found

ERROR: An error occurred during the fetch of repository 'io_bazel_rules_scala_listenablefuture':
  Traceback (most recent call last):
    File ".../scala/scala_maven_import_external.bzl",
    line 130, column 32, in _jvm_import_external
      repository_ctx.download(srcurls, srcpath, srcsha, auth = _get_auth(repository_ctx, srcurls))

Error in download: java.io.IOException: Error downloading
  [ ...4 URLs from WARNINGs above... ]
to .../external/io_bazel_rules_scala_listenablefuture/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-src.jar:
GET returned 404 Not Found

ERROR: /Users/mbland/src/bazelbuild/rules_scala/WORKSPACE:75:25:
  fetching jvm_import_external rule
  //external:io_bazel_rules_scala_listenablefuture:
  [ ...Traceback, java.io.IOException, and HTTP 404 from above... ]

ERROR: .../external/io_bazel_rules_scala_guava/BUILD:9:13:
  @io_bazel_rules_scala_guava//:io_bazel_rules_scala_guava depends on
  @io_bazel_rules_scala_listenablefuture//:io_bazel_rules_scala_listenablefuture
  in repository @io_bazel_rules_scala_listenablefuture
  which failed to fetch. no such package
  '@io_bazel_rules_scala_listenablefuture//':
     [ ...java.io.IOException and HTTP 404 from above... ]

ERROR: Analysis of target
  '//test/proto/custom_generator:failing_scala_proto_deps_toolchain_def'
  failed; build aborted:
```
Since Scala 2.11 is stuck on ScalaPB 0.9.8, its dependencies don't
include the `dev.dirs:directories` artifact. Before this change,
building with `--repo_env=SCALA_VERSION=2.11.12 would die with:

```txt
$ bazel build --repo_env=SCALA_VERSION=2.11.12 \
  //src/... //test/...  //third_party/... //scala_proto/...

ERROR: Traceback (most recent call last):
  File "/Users/mbland/src/bazelbuild/rules_scala/WORKSPACE",
  line 75, column 25, in <toplevel>
    scala_proto_repositories()

  File "/Users/mbland/src/bazelbuild/rules_scala/scala_proto/scala_proto.bzl",
  line 17, column 37, in scala_proto_repositories
    scala_proto_default_repositories(**kwargs)

  File "/Users/mbland/src/bazelbuild/rules_scala/scala_proto/default/repositories.bzl",
  line 7, column 17, in scala_proto_default_repositories
    repositories(

  File "/Users/mbland/src/bazelbuild/rules_scala/third_party/repositories/repositories.bzl",
  line 107, column 17, in repositories
    fail("artifact %s not in third_party/repositories/scala_%s.bzl" % (

Error in fail: artifact dev_dirs_directories not in
  third_party/repositories/scala_2_11.bzl

ERROR: Error computing the main repository mapping:
  Encountered error while reading extension file 'go/deps.bzl':
  no such package '@io_bazel_rules_go//go':
  error loading package 'external': Could not load //external package
```

This is a preview of scala_proto toolchainization, given the addition of
`scala_version` and `register_toolchains` parameters to
`scala_proto_default_repositories`.
Without this, the `com.thesamet.scalapb:compilerplugin` artifact is out
of sync with other ScalaPB artifacts, leading to this error:

```txt
$ bazel build //test/proto_cross_repo_boundary/...

ERROR: .../external/proto_cross_repo_boundary/BUILD.bazel:3:14:
  scala @proto_cross_repo_boundary//:sample_proto failed:
  (Exit 1): scalac failed: error executing command
  (from target @proto_cross_repo_boundary//:sample_proto)

bazel-out/.../bin/src/java/io/bazel/rulesscala/scalac/scalac
  @bazel-out/.../bin/external/proto_cross_repo_boundary/sample_proto_scalapb.jar-0.params

bazel-out/.../bin/external/proto_cross_repo_boundary/_scalac/sample_proto/sources/1_sample_proto_scala_scalapb.srcjar/sample/sample/Sample.scala:11:
  error: class Any needs to be a trait to be mixed in
    ) extends scalapb.GeneratedMessage with scalapb.Message[Sample] with scalapb.lenses.Updatable[Sample] {
                                                    ^
Target //test/proto_cross_repo_boundary:sample_scala_proto failed to build
```
There were API changes from scalapb-runtime 0.9.7 causing the following
errors:

```txt
ERROR: .../test/src/main/scala/scalarules/test/extra_protobuf_generator/BUILD:3:14:
  scala @//test/src/main/scala/scalarules/test/extra_protobuf_generator:extra_protobuf_generator
  failed: (Exit 1): scalac failed: error executing Scalac command
  (from target //test/src/main/scala/scalarules/test/extra_protobuf_generator:extra_protobuf_generator)
  bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/src/java/io/bazel/rulesscala/scalac/scalac
    ... (remaining 1 argument skipped)

test/src/main/scala/scalarules/test/extra_protobuf_generator/ExtraProtobufGenerator.scala:20:
  error: value nameSymbol is not a member of com.google.protobuf.Descriptors.Descriptor
      .add(s"final case object Custom${message.nameSymbol}{}")
                                               ^
test/src/main/scala/scalarules/test/extra_protobuf_generator/ExtraProtobufGenerator.scala:34:
  error: value fileDescriptorObjectName is not a member of com.google.protobuf.Descriptors.FileDescriptor
    b.setName(file.scalaDirectory + "/Custom" + file.fileDescriptorObjectName + ".scala")
                                                     ^
test/src/main/scala/scalarules/test/extra_protobuf_generator/ExtraProtobufGenerator.scala:66:
  error: value FileDescriptorPimp is not a member of scalapb.compiler.DescriptorImplicits
          import implicits.FileDescriptorPimp
                 ^
```

Since Scala 2.11 can't advance past scalapb-runtime 0.9.8, we use
`select_for_scala_version` to select the appropriate `ProtobufAdapter`
to maintain API compatibility.

Also, `import implicits.FileDescriptorPimp` turned out to be unnecessary
after all, even under Scala 2.11.
Fixes the `test_demonstrate_INCORRECT_scala_proto_library_stamp` test
case from `test/shell/test_strict_dependency.sh` after the ScalaPB
0.11.17 bump.

Somehow, bumping that library (presumably, instead of gRPC, et. al.)
changed the parameter list of `TestMessage` from `String` to
`String,UnknownFieldSet`. The test then failed because the following
command produced this unexpected error:

```txt
$ bazel build --verbose_failures \
  //test_expect_failure/missing_direct_deps/scala_proto_deps:uses_transitive_scala_proto \
  --extra_toolchains=//test/toolchains:ast_plus_one_deps_strict_deps_error

ERROR: .../test_expect_failure/missing_direct_deps/scala_proto_deps/BUILD:25:13:
  Building test_expect_failure/missing_direct_deps/scala_proto_deps/libtransitive.jar
  (1 source file) failed: (Exit 1): java failed: error executing command
  (from target //test_expect_failure/missing_direct_deps/scala_proto_deps:transitive)

  (cd ...
    [ ...snip... ]
  external/remotejdk11_macos_aarch64/bin/java -XX:-CompactStrings
    [ ...snip... ]
  external/remote_java_tools/java_tools/JavaBuilder_deploy.jar
  @bazel-out/.../bin/test_expect_failure/missing_direct_deps/scala_proto_deps/libtransitive.jar-0.params
  @bazel-out/.../bin/test_expect_failure/missing_direct_deps/scala_proto_deps/libtransitive.jar-1.params)

test_expect_failure/missing_direct_deps/scala_proto_deps/UseTestMessage.java:7:
  error: constructor TestMessage in class TestMessage
  cannot be applied to given types;

  private final TestMessage m = new TestMessage("");
                                ^
  required: String,UnknownFieldSet
  found: String
  reason: actual and formal argument lists differ in length
```

After this change, the same command produces the following failure that
the test is expecting to catch:

```txt
ERROR: .../test_expect_failure/missing_direct_deps/scala_proto_deps/BUILD:18:14:
  scala @//test_expect_failure/missing_direct_deps/scala_proto_deps:uses_transitive_scala_proto
  failed: (Exit 1): scalac failed: error executing command
  (from target //test_expect_failure/missing_direct_deps/scala_proto_deps:uses_transitive_scala_proto)

  (cd .../execroot/io_bazel_rules_scala && \
  exec env - \
  bazel-out/.../bin/src/java/io/bazel/rulesscala/scalac/scalac
  @bazel-out/.../bin/test_expect_failure/missing_direct_deps/scala_proto_deps/uses_transitive_scala_proto.jar-0.params)

test_expect_failure/missing_direct_deps/scala_proto_deps/UseScalaProtoIndirectly.scala:6:
  error: Target '@//test_expect_failure/missing_direct_deps/scala_proto_deps:some_proto'
  is used but isn't explicitly declared, please add it to the deps.

You can use the following buildozer command:
  buildozer 'add deps
    @//test_expect_failure/missing_direct_deps/scala_proto_deps:some_proto'
    //test_expect_failure/missing_direct_deps/scala_proto_deps:uses_transitive_scala_proto

  val foo: TestMessage = new UseTestMessage().getM
           ^
one error found
Build failure with errors.
Target //test_expect_failure/missing_direct_deps/scala_proto_deps:uses_transitive_scala_proto
  failed to build
```
Already did this for `test/TestServer.scala` as part of the gRPC bump.
`test_version/version_specific_tests_dir/TestServer.scala` gets the same
treatment now to fix `test_scala_version 2.11.12` from
`test_version.sh`, which failed with:

```
ERROR: .../test_version/test_scala_version_1731027515/BUILD:161:14:
  scala @//:lib_with_scala_proto_dep failed: (Exit 1): scalac failed:
  error executing command (from target //:lib_with_scala_proto_dep)

bazel-out/.../bin/external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/scalac
  @bazel-out/.../bin/lib_with_scala_proto_dep.jar-0.params

TestServer.scala:70: error: too many arguments for method usePlaintext: ()?0
    val channel = ManagedChannelBuilder.forAddress(host, port).usePlaintext(true).build
                                                                           ^
```
More specifically, add it to the list of core scala repository imports.
It had been generated before, but somehow hadn't been required until
now.

With this change, all tests now pass after the ScalaPB 0.11.17, gRPC
1.68.1, and Guava 33.3.1-jre version bumps.

Fixes the following error in `test_scala_version 2.12.20` from
`test_version.sh`:

```txt
ERROR: .../external/scala_proto_rules_scalapb_compilerplugin/BUILD:9:13:
  no such package '@org_scala_lang_modules_scala_collection_compat//':

  The repository '@org_scala_lang_modules_scala_collection_compat'
  could not be resolved:

  Repository '@org_scala_lang_modules_scala_collection_compat'
  is not defined and referenced by
  '@scala_proto_rules_scalapb_compilerplugin//:scala_proto_rules_scalapb_compilerplugin'

ERROR: Analysis of target '//proto:test_proto_nogrpc' failed; build aborted:
```
Wraps `scalapb.ScalaPbCodeGenerator` to catch and return all errors.
Without this, any errors escaping from `scalapb.ScalaPbCodeGenerator`
will cause the `scala_proto` aspect workers to hang.

Also deletes some `@io_bazel_rules_scala` references, replacing them
with `Label` instances as appropriate.

The `test_scala_version 2.11.12` case from `test_version.sh` would
previously hang given the combination of ScalaPB 0.9.8 (the newest
available for Scala 2.11) and Protobuf v28.3. Running the following in a
`test_version/test_scala_version_*` repo generated by `test_version.sh`
reproduced the hang:

```txt
$ bazel build --repo_env=SCALA_VERSION=2.11.12 //proto:test_proto

[ ...wait 10 seconds, then CTRL-C... ]

INFO: Analyzed target //proto:test_proto (2 packages loaded, 22 targets configured).
INFO: Found 1 target...
[1,038 / 1,047] 4 actions running
    ProtoScalaPBRule proto/test_service_scala_scalapb.srcjar; 10s worker
    ProtoScalaPBRule proto/test2_scala_scalapb.srcjar; 10s worker
Target //proto:test_proto failed to build
```

With this change, the command now exposes the exact incompatibility
between these versions:

```txt
$ bazel build --repo_env=SCALA_VERSION=2.11.12 //proto:test_proto

ERROR: .../test_version/test_scala_version_1731169107/proto/BUILD:14:14:
  ProtoScalaPBRule proto/test3_scala_scalapb.srcjar failed: (Exit 1):
  scalapb_worker failed: error executing command
  (from target //proto:test3)

bazel-out/.../bin/external/io_bazel_rules_scala/src/scala/scripts/scalapb_worker
  ... (remaining 2 arguments skipped)

--scala_out: java.lang.NoSuchMethodError:
  'void com.google.protobuf.Descriptors$FileDescriptor.internalBuildGeneratedFileFrom(java.lang.String[],
    com.google.protobuf.Descriptors$FileDescriptor[],
    com.google.protobuf.Descriptors$FileDescriptor$InternalDescriptorAssigner)'
      at scalapb.options.compiler.Scalapb.<clinit>(Scalapb.java:10592)
      at scalapb.ScalaPbCodeGenerator$.run(ScalaPbCodeGenerator.scala:14)
      at scalapb.ScalaPbCodeGenerator$.run(ScalaPbCodeGenerator.scala:10)
      at scripts.ScalaPbCodeGenerator$.run(ScalaPbCodeGeneratorWrapper_2_11.scala:8)
      at protocbridge.frontend.PluginFrontend$$anonfun$runWithBytes$2.apply(PluginFrontend.scala:52)
      at protocbridge.frontend.PluginFrontend$$anonfun$runWithBytes$2.apply(PluginFrontend.scala:52)
      at scala.util.Try$.apply(Try.scala:192)
      at protocbridge.frontend.PluginFrontend$.runWithBytes(PluginFrontend.scala:51)
      at protocbridge.frontend.PluginFrontend$.runWithInputStream(PluginFrontend.scala:103)
      at protocbridge.frontend.PosixPluginFrontend$$anonfun$prepare$1.apply$mcV$sp(PosixPluginFrontend.scala:33)
      at protocbridge.frontend.PosixPluginFrontend$$anonfun$prepare$1.apply(PosixPluginFrontend.scala:31)
      at protocbridge.frontend.PosixPluginFrontend$$anonfun$prepare$1.apply(PosixPluginFrontend.scala:31)
      at scala.concurrent.impl.Future$PromiseCompletingRunnable.liftedTree1$1(Future.scala:24)
      at scala.concurrent.impl.Future$PromiseCompletingRunnable.run(Future.scala:24)
      at scala.concurrent.impl.ExecutionContextImpl$AdaptedForkJoinTask.exec(ExecutionContextImpl.scala:121)
      at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
      at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
      at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
      at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)

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)

Target //proto:test_proto failed to build
```
This will support the upcoming schema whereby `scala_toolchains` will
import all artifact IDs via macros like this, and invoke `repositories`
itself. This will avoid instantiating the same artifact repository
multiple times for different frameworks that depend on it.

Under `WORKSPACE`, this isn't a problem, but it's an error under Bzlmod,
as a module extension can only instantiate a repository once. The commit
message to toolchainize Scalafmt will provide more details on this.
@mbland
Copy link
Contributor Author

mbland commented Nov 22, 2024

FYI, and FWIW, Iust filed scalapb/ScalaPB#1771 to see about applying fixes upstream to avoid scalapb.ScalaPbCodeGenerator hangs in future versions.

Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants