Skip to content

feat: bit more configurable proto toolchain #1718

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

Merged
merged 12 commits into from
Mar 26, 2025

Conversation

kczulko
Copy link
Contributor

@kczulko kczulko commented Mar 17, 2025

Description

Hello,

Over a year ago I had the experience of porting a project from sbt to rules_scala. One of the quirks of such a migration was protobuf. That time I've used rules_proto_grpc to overcome this part but I thought it should be possible with built-in proto toolchain from rules_scala. Now I finally had some time to take this journey and check it. Here are the results:

  1. proto_enable_all_options removed - I think that user should define the options. This all seems to be a bit out of date, e.g. scala3_sources was not included there.
  2. grpc dep id removed - Similar to above. The code that was merging compile deps was checking with_grpc flag that also got removed. I took an approach that here, as well, user knows what he wants so if grpc flag was added to the toolchain configuration, that means you need to provide proper dependencies as well. IMHO this grpc dep id was a bit artificial but I'm keen to hear some other opinion.
  3. main_generator removed - for this approach the main_generator becomes the default one and got moved to named_generators with its default name set to scala.

I assume there might be a places which are not ready yet (like docs). Let's see what CI will tell us.

Motivation

  • User can define proto flags per plugin (either scalapb, fs2grpc, pekko-grpc, etc)
  • Introducing support for more than a 3 proto flags (e.g. scala3_sources)

Copy link
Contributor

@mbland mbland left a comment

Choose a reason for hiding this comment

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

If/when this goes in, it'll need a section added under "Breaking changes in rules_scala 7.x" in README.md. Probably an h4 (starting with ####) at the end of the "New scala_toolchains() API for WORKSPACE" section (before "@io_bazel_rules_scala_config is now @rules_scala_config").

Run bazel run //tools:lint_fix to fix the ./test_lint.sh error.

Also, it's good to run ./test_all.sh before pushing. It'll run all the CI scripts on your local machine, plus dt_patches/dt_patch_test.sh.

It looks like "test_scala_version 2.12.20" from test_version.sh failed:

ERROR: /workdir/test_version/test_scala_version_1742216337/BUILD:161:14:
  scala @//:lib_with_scala_proto_dep failed:
  (Exit 1): scalac failed:
  error executing Scalac command
  (from target //:lib_with_scala_proto_dep)
  bazel-out/k8-opt-exec-ST-a828a81199fe/bin/external/rules_scala/src/java/io/bazel/rulesscala/scalac/scalac
    @bazel-out/k8-fastbuild/bin/lib_with_scala_proto_dep.jar-0.params
--
  | TestServer.scala:6: error:
  |   object TestServiceGrpc is not a member of package test.proto.test_service
  | import test_service.TestServiceGrpc

If you want to iterate on this , you can run RULES_SCALA_TEST_ONLY="test_scala_version 2.12.20" ./test_version.sh. (Hmm, maybe I should add these tips to README.md?)

@simuons
Copy link
Collaborator

simuons commented Mar 18, 2025

Hi, @kczulko, thanks for PR. Your approach looks good to me. I remember there was a discussion in the past "list of opts vs individual attribute in toolchain for each opt" but I don't remember remember where it was. I'll try to look around and get back to this.

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.

As I said overall I like the change. But I think we should handle "default" generator differently. Waiting for your comment.

@kczulko kczulko force-pushed the kczulko/proto-fixes branch from 3e7ee3e to 6569e90 Compare March 26, 2025 12:43
@kczulko
Copy link
Contributor Author

kczulko commented Mar 26, 2025

@simuons @mbland
I think it's ready for the next review round. I've addressed the scalapb_toolchain suggestion together with the new README.md section that Mike wanted.

Let me know if that's ok for you.

Best regards,
Karol

Copy link
Contributor

@mbland mbland left a comment

Choose a reason for hiding this comment

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

Thanks for adding the docs. I've some editorial suggestions that hopefully make things a little clearer.

@simuons
Copy link
Collaborator

simuons commented Mar 26, 2025

Hi, @kczulko, looks good to me now. Please address comments by @mbland and I'll merge it.

@kczulko kczulko force-pushed the kczulko/proto-fixes branch from df7e4bb to 3c25cdf Compare March 26, 2025 17:46
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 @kczulko!

@simuons simuons merged commit c1e58d3 into bazel-contrib:master Mar 26, 2025
2 checks passed
avinashak added a commit to discord/rules_scala that referenced this pull request Apr 10, 2025
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.

3 participants