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

Protobuf compatibility findings and suggestions #1647

Open
mbland opened this issue Nov 10, 2024 · 0 comments
Open

Protobuf compatibility findings and suggestions #1647

mbland opened this issue Nov 10, 2024 · 0 comments

Comments

@mbland
Copy link
Contributor

mbland commented Nov 10, 2024

As a side quest to #1482, I've been experimenting with trying to update the Protobuf library beyond version 21.7, released 2022-09-29. The reason is that I anticipate potential complications after the Bzlmod update lands if users have later Protobuf versions in their MODULE.bazel dependency graph.

I've discovered the following details, which we may need to communicate to users through release notes and/or other documentation. Here are the combinations of various dependencies that work with one another. The first row is the current combination.

Protobuf abseil-cpp Bazel 6 ScalaPB Scala
v21.7 20220623.1 0.9.0 All
v21.7 20220623.1 0.9.8 All
v21.7 20220623.1 0.11.17 >= 2.12
v25.5 20240722.0 with --cxxopt 0.9.8 All
v25.5 20240722.0 with --cxxopt 0.11.17 >= 2.12
>= v28.2 20240722.0 with --cxxopt 1.0.0-alpha.1 >= 2.12

Suggestions

I'd suggest updating the .bazelversion to 7.4.0 and bumping Protobuf to v25.5. My reasoning is that these changes would bring rules_scala closer to recent Bazel and Protobuf updates, without potentially breaking existing users.

If we decide to build with Bazel 7 by default instead of Bazel 6, the Protobuf recompilation sensitivity won't be as much of an issue. In my experience, after merging #1619, #1620, and #1622, I haven't found any other Bazel 6 and 7 incompatibilities outside of building newer Protobuf versions.

It would be nice to bump to ScalaPB 1.0.0-alpha.1 to use the latest Protobuf v28.3. However, it seems like that might be too big a bump for existing users right now.

Takeaways

ScalaPB and the scala_proto_aspect are sensitive to the Protobuf version

The scala_proto_library rule uses an aspect that delegates to the protoc-bridge generator framework to generate code. protoc-bridge itself isn't very sensitive, but the ScalaPB.ScalaPbCodeGenerator implementations to which it delegates are very sensitive to the Protobuf library version.

See the protoc-bridge explanation section below for details on why this is the case.

The Protobuf Bazel module remaining at compatibility_level 1 is a problem

All current versions of the Protobuf Bazel module, from before 21.7 and up to the current 29.0-rc2.bcr.1, have a compatibility_level of 1. This means that all modules in the Bzlmod/MODULE.bazel dependency graph will resolve to a single Protobuf version, regardless of its major version number.

This means that rules_scala locks scala_proto users to the maximum Protobuf version that it supports (or to a minimum of v28.2 with ScalaPB 1.0.0-alpha.1). While already true when using WORKSPACE, this will also be the case under Bzlmod, until a Protobuf module release with a different compatibility_level.

Protobuf >= v22 requires C++14 or greater, --cxxopt flags in Bazel 6

Protobuf v22 dropped C++11 support, requiring C++14 or greater. This corresponds to the update in abseil-cpp from 20220623.2, the last to support C++11, to 20230125.0, the first to require C++14.

This is not a problem for Bazel 7. Bazel 6, however, does not use a C++14 or greater toolchain out of the box.

To build these more recent versions of abseil-cpp and Protobuf under Bazel 6, I've added these .bazelrc flags from bazelbuild/bazel#20785 in commit 03d4e20 from mbland/rules_scala:

build:linux --cxxopt=-std=c++17
build:linux --host_cxxopt=-std=c++17
build:macos --cxxopt=-std=c++17
build:macos --host_cxxopt=-std=c++17
build:windows --cxxopt=/std=c++17
build:windows --host_cxxopt=/std=c++17

--cxxopt flags in Bazel 6 cause Protobuf to rebuild more frequently

Protobuf is already notoriously sensitive to recompilation.

This recompilation became so bad, I thought test_scalafmt from test_cross_build.sh was hanging. It was actually recompiling Protobuf on every bazel run invocation. (I've since filed #1646 to resolve this.)

Protobuf v25.5 is the highest version compatible with ScalaPB 0.9.8, 0.11.17

I was able to confirm this by iterating through different configurations of the test_scala_version test case from test_version.sh. I used the RULES_SCALA_TEST_ONLY environment variable from #1646 and the test_version.sh changes from commit ff1a079 from mbland/rules_scala to do this fairly easily.

There is no combination that works with Protobuf v26 or v27

I was able to confirm this using the same methodolgy...

No Protobuf version < v28.2 works with ScalaPB 1.0.0-alpha.1

...as I was able to confirm this as well.

Working branches

I was able to reach the above conclusions by building up changes in the following branches:

protoc-bridge explanation

scala_proto_library uses scala_proto_aspect, which uses the scripts.ScalaPBWorker class from src/scala/scripts/ScalaPBWorker.scala, which calls ProtocBridge.runWithGenerators. This function ultimately runs generator classes in separate threads via Futures. It then runs protoc with --plugin flags pointing at shell wrappers communicating with these generator Futures over a local pipe.

The ScalaPB jars required by the generators (not so much the protoc-bridge framework itself) must be compatible with the current Protobuf library version. Not just with the protobuf-java Maven artifact, which can be a new as you like, but with the build's actual Protobuf repository that provides protoc. Otherwise, ScalaPB code will throw exceptions like java.lang.NoSuchMethodError or java.lang.IllegalAccessError.

For example, from the #1624 commit message:

Exception in thread "main" java.lang.NoSuchMethodError:
  'boolean com.google.protobuf.GeneratedMessageV3.isStringEmpty(java.lang.Object)'

From the #1630 commit message:

--jvm_extra_protobuf_generator_out: java.lang.NoSuchMethodError:
  'java.lang.Object com.google.protobuf.DescriptorProtos$MessageOptions.getExtension(com.google.protobuf.GeneratedMessage$GeneratedExtension)'
    at scalapb.compiler.DescriptorImplicits$ExtendedMessageDescriptor.messageOptions(DescriptorImplicits.scala:532)

From the #1637 commit message:

--jvm_extra_protobuf_generator_out: java.lang.IllegalAccessError:
  class scalapb.options.Scalapb$ScalaPbOptions tried to access method
  'com.google.protobuf.LazyStringArrayList
    com.google.protobuf.LazyStringArrayList.emptyList()'
  (scalapb.options.Scalapb$ScalaPbOptions and
   com.google.protobuf.LazyStringArrayList are in unnamed module
   of loader 'app')

From commit 6f702a6 from mbland/rules_scala, which I plan to submit when bumping to ScalaPB 0.11.17:

--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)

The changes from #1637 and the last commit above catch ScalaPB generator exceptions and return the stack trace as a proper error response. This is how I collected the stack traces above. Without these changes, the build hangs on uncaught exceptions, since the scripts.ScalaPBWorker will wait forever for a dead generator to respond.

Here's an example of a scripts.ScalaPBWorker command generated when building scala_proto_library targets in //test/proto/..., with long path prefixes and many jar paths elided:

.../scalapb_worker.runfiles/io_bazel_rules_scala/../remotejdk11_.../bin/java
  -classpath
    .../io_bazel_rules_scala/src/scala/scripts/scalapb_worker.jar:
    .../io_bazel_rules_scala/src/scala/scripts/scalapb_worker_lib.jar:
    [ ...protobuf, core Scala, and ScalaPB jars... ]
  -DGEN_jvm_extra_protobuf_generator=
   scalarules.test.extra_protobuf_generator.ExtraProtobufGenerator
  -DGEN_scala=scripts.ScalaPbCodeGenerator
  -DPROTOC=bazel-out/.../bin/external/com_google_protobuf/protoc
  -DJARS=
    .../bin/test/src/main/scala/scalarules/test/extra_protobuf_generator/extra_protobuf_generator.jar:
    [ ...protobuf, core Scala, and ScalaPB jars... ]
  scripts.ScalaPBWorker --persistent_worker

The scripts.ScalaPBWorker class calling ProtocBridge.runWithGenerators launches protoc processes like this, with $VARDIR representing the generated /var path:

bazel-out/.../bin/external/com_google_protobuf/protoc
  --plugin=protoc-gen-scala=$VARDIR/protocbridge11068409970088580527
  --plugin=protoc-gen-jvm_extra_protobuf_generator=
    $VARDIR/protocbridge7908074065015031242
  --jvm_extra_protobuf_generator_out
    bazel-out/.../bin/test/proto/
    test2_jvm_extra_protobuf_generator_scalapb.srcjar
  --jvm_extra_protobuf_generator_opt grpc,single_line_to_proto_string
  --scala_out bazel-out/.../bin/test/proto/test2_scala_scalapb.srcjar
  --scala_opt grpc,single_line_to_proto_string
  --descriptor_set_in
    bazel-out/.../bin/test/proto2/test-descriptor-set.proto.bin:
    bazel-out/.../bin/test/proto/test2-descriptor-set.proto.bin
  test/proto/test2.proto

Notice the --plugin= flags above, which specify scripts generated by protoc-bridge that look like this:

#!/bin/sh
set -e
cat /dev/stdin > "$VARDIR/protopipe-14656650241271353812/input"
cat "$VARDIR/protopipe-14656650241271353812/output"

This command will show the scripts.ScalaPBWorker parent process, all the plugin script child processes, and the cat "$VARDIR/protopipe-.../output" grandchild processes if they're still running:

ps -ef | grep -E '[p]roto(cbridge|pipe)'

Without the aforementioned changes, you have to kill the build with CTRL-C when it hangs. bazel shutdown will then clean up the processes that haven't yet become zombified, but such zombified processes must be cleaned up with:

ps -ef | grep -E '[p]roto(cbridge|pipe)' | awk '{print $2}' | xargs kill
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

No branches or pull requests

1 participant