Toolchainize //scala:toolchain_type #1633

mbland · 2024-10-23T18:45:59Z

Description

Moves the toolchain targets for //scala:toolchain_type to a new @io_bazel_rules_scala_toolchains repository as a step towards Bzlmodification. Part of #1482.

Motivation

Instantiating toolchains in their own repository enables module extensions to define the the repositories required by those toolchains within the extension's own namespace. Bzlmod users can then register the toolchains from this repository without having to import all the toolchains' dependencies into their own namespace via use_repo().

The scala_toolchains_repo() macro wraps the underlying repository rule and assigns it the standard name io_bazel_rules_scala_toolchains. Right now it's only instantiating the main Scala toolchain via the default scala = True parameter. Future changes will expand this macro and repository rule with more boolean parameters to instantiate other toolchains, specifically:

scalatest
junit
specs2
twitter_scrooge
jmh
scala_proto and scala_proto_enable_all_options
testing (includes all of scalatest, junit, and specs2)
scalafmt

WORKSPACE users will now have to import and call the scala_toolchains_repo() macro to instantiate
@io_bazel_rules_scala_toolchains.

load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config")

scala_config()

load(
    "//scala:scala.bzl",
    "rules_scala_setup",
    "rules_scala_toolchain_deps_repositories",
    "scala_toolchains_repo",
)

rules_scala_setup()

rules_scala_toolchain_deps_repositories()

scala_toolchains_repo()

register_toolchains("@io_bazel_rules_scala_toolchains//...:all")

Or, they can use the new single scala_toolchains() macro to elide most of the calls:

load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config")

scala_config()

load("//scala:scala.bzl", "scala_toolchains")

scala_toolchains()

register_toolchains("@io_bazel_rules_scala_toolchains//...:all")

This is what the corresponding MODULE.bazel setup would look like:

module(name = "rules_scala", version = "7.0.0")

scala_config = use_extension(
    "//scala/extensions:config.bzl", "scala_config"
)
scala_config.settings(scala_version = "2.13.14")

scala_deps = use_extension("//scala/extensions:deps.bzl", "scala_deps")
scala_deps.toolchains()

The register_toolchains() call in WORKSPACE isn't strictly required at this point, but is recommended. All the WORKSPACE files in this repo already registered their required toolchains using existing macros. However, they've been updated to use register_toolchains(...) instead.

These register_toolchains() calls maintain the order of previous toolchain registrations to avoid the following breakages:

test_compilation_fails_with_plus_one_deps_undefined from test/shell/test_compilation.sh depends on //scala:unused_dependency_checker_error_toolchain resolving first. This toolchains sets the scala_toolchain() parameters dependency_tracking_method = "ast-plus" and unused_dependency_checker_mode = "error", and the @io_bazel_rules_scala_toolchains//scala toolchains don't.
test_scala_binary_allows_opt_in_to_use_of_argument_file_in_runner_for_improved_performance from test/shell/test_scala_binary.sh depends on the use_argument_file_in_runner parameter of scala_toolchain being False. This is the default, but the @io_bazel_rules_scala_toolchains//scala toolchains explicitly set this to True instead.

In the Bzlmod case, the register_toolchains() call isn't necessary at all. This is because @io_bazel_rules_scala_toolchains includes one package per set of toolchains, and the rules_scala MODULE.bazel calls register_toolchains("@io_bazel_rules_scala_toolchains//...:all"). This will automatically register all configured rules_scala toolchains, while allowing users to override any of them using register_toolchains() in their own MODULE.bazel files.

Technically, the scala_deps.toolchains() call isn't required when only using the default scala = True parameter; the rules_scala MODULE.bazel will instantiate this automatically, too.

mbland · 2024-10-25T23:37:59Z

I've rebased this PR onto master after #1635 landed, updated it substantially with two new commits, and adjusted the PR description.

The first new commit introduces the new scala_toolchains() macro, which will be the common funnel point between WORKSPACE and Bzlmod. However, that first commit doesn't add the macro to existing WORKSPACE calls. This demonstrates that the changes to rules_scala_setup and scala_repositories are compatible with existing API usage.

The second commit does replace as many WORKSPACE calls as possible with scala_toolchains() and register_toolchains(). This shows that the new API produces equivalent results, and proves a clear path towards adding the Bzlmod layer.

If you'd like to see the end state I have in mind for this macro, the scala_toolchains_repo() macro/rule, and the Bzlmod extension, check out the equivalent files in my Bzlmod working branch:

I've tested that all the following work with both the first commit and the second:

test_lint.sh
test_all.sh
test_examples.sh
test_coverage.sh
dt_patches/dt_patch_test.sh (after cherry-picking into my local branch the commit from Fix dt_patches/dt_patch_test.sh for Scala 2.13.0 #1632)

Once this change lands, toolchainizing the remaining toolchains should prove very straightforward. Then Bzlmod is possibly only PR away after that.

simuons · 2024-11-04T12:33:55Z

Hi, @mbland, I just want to let you know that I'm taking a look at this. Whole last week I was away, sorry for delay. Give me couple of days on this. I want to understand how final api in bzlmod would look like.

mbland · 2024-11-04T16:09:40Z

@simuons No worries. Thanks for the heads-up.

FWIW, now that I've got Bzlmod and WORKSPACE working together in my Bzlmod working branch, you can compare each WORKSPACE/MODULE.bazel pair in that branch. The repos under examples/ are probably the best representation of what I'd expect most users to end up with. The dt_patches/test_dt_patches_user_srcjar/MODULE.bazel file shows how it looks to register compiler srcjars.

simuons

Hi, @mbland, thanks for PR. Please take a look at comments/questions.

Sorry it's taking too long, past two weeks were busy and PR itself is not trivial (at least for me).

simuons · 2024-11-08T11:36:37Z

scala/private/macros/scala_repositories.bzl

-        dt_patched_compiler_setup(scala_version, scala_compiler_srcjar)
+        dt_patched_compiler_setup(scala_version, srcjars.get(scala_version))
+
+    compiler_sources_repo(


This duplicates compiler_sources_repo below

D'oh! Merge fail. Fixed. (Or, will be once I get through all the comments and push an update.)

simuons · 2024-11-08T11:48:19Z

scala/toolchains.bzl

@@ -4,10 +4,11 @@ load("@io_bazel_rules_scala//scala:scala_cross_version.bzl", "version_suffix")
 def scala_register_toolchains():
    for scala_version in SCALA_VERSIONS:
        native.register_toolchains(
-            "@io_bazel_rules_scala//scala:toolchain" + version_suffix(scala_version),
+            "@io_bazel_rules_scala_toolchains//scala:toolchain" +


Isn't @io_bazel_rules_scala_toolchains//scala guaranteed to have a toolchain for each configured scala version? If so then maybe this macros should do
register_toolchains("@io_bazel_rules_scala_toolchains//...:all") as in workspaces?

Heh, you're right. Done.

simuons · 2024-11-08T11:51:25Z

scala/private/macros/scala_repositories.bzl

@@ -165,8 +170,26 @@ def rules_scala_setup(scala_compiler_srcjar = None):
            url = "https://github.com/bazelbuild/rules_proto/releases/download/6.0.2/rules_proto-6.0.2.tar.gz",
        )

+    if setup_compiler_sources:
+        srcs = {version: scala_compiler_srcjar for version in SCALA_VERSIONS}


Probably this slipped with previous PRs but for me it looks like same compiler sources might be used for multiple scala versions. Probably separate issue need to be open.

Yeah, I noticed that, but this technically maintains the previous behavior. The new _setup_scala_compiler_sources function underlying it doesn't have this problem. That implementation could technically replace this one; let me know what you'd prefer, but that would be a breaking change (or maybe just a bugfix?).

I guess I shouldn't bring this up. Let's leave this as is. It will either go away with bzlmod or will be fixed in a separate PR.

simuons · 2024-11-08T12:44:11Z

scala/private/macros/scala_repositories.bzl

    if load_dep_rules:
-        rules_scala_setup()
+        # When `WORKSPACE` goes away, so can this case.
+        rules_scala_setup(setup_compiler_sources = False)


Maybe it would be better to split rules_scala_setup into two macros: load_rules_dependencies and setup_scala_compiler_sources.
So that rules_scala_setup would call both of these macros. And here load_rules_dependencies would be called conditionally and setup_scala_compiler_sources unconditionally.
It looks a bit odd that half of the macro is disabled and that disabled part is called immediately.

I'd agree, but that would be another break from the existing API. But if you definitely want it now, I'll do it.

Just to make myself clear. What I meant was

def rules_scala_setup(scala_compiler_srcjar = None): load_rules_dependencies() setup_scala_compiler_sources()

So that existing clients of rules_scala_setup won't break

And here have

if load_dep_rules: load_rules_dependencies() setup_scala_compiler_sources(scala_compiler_srcjars)

This would add two new macros to the api but it would be easier to follow the code (at least for me). Unless I completely misunderstood what you mean by "break from the existing API"

Ah, you're so right. I wasn't reading closely enough or thinking clearly enough before. This is definitely better, and doesn't break the existing API at all. Done.

simuons · 2024-11-08T13:07:59Z

scala/private/macros/toolchains.bzl

+        fetch_sources = False,
+        validate_scala_version = True,
+        scala_compiler_srcjars = {},
+        scala = True):


Is there a case when someone would like to call this macro with scala = False? Maybe this macro could be simplified by removing this arg along with num_toolchains and conditionals?

At the time. I thought that would be within the realm of possibility. But now, it does seem silly. Removed.

simuons · 2024-11-08T17:53:37Z

I just want to share my ideas/concerns/struggles I've got while looking at this PR (reason it took longer than needed to review). By no means these are blockers or something that needs to be addressed in this PR:

Number of internal repositories keeps growing. Maybe these could be merged in bzlmod (probably not possible in workspace). For example I have a feeling that scala_config repository should be merged into toolchains as those are closely related. Maybe this is not an issue at all.
There are too many scala toolchains. Comment about toolchains ordering is concerning. Issue is that features orthogonal to scala version are configured on toolchain. Ideally there should be single toolchain per scala version. Maybe I'll continue exploration on Configure rules behaviour/toolchain attributes via build settings #1570

Maybe it's a wrong place/time for this, but I just wanted to share so I won't forget it. Or maybe I just want to get second opinion on this :)

mbland · 2024-11-08T18:23:34Z

I just want to share my ideas/concerns/struggles I've got while looking at this PR (reason it took longer than needed to review). By no means these are blockers or something that needs to be addressed in this PR:

No worries! It's important to get this right.

BTW, I'm replying now as I run the full test suite again. Will push my updates and ping when that's all done.

Number of internal repositories keeps growing. Maybe these could be merged in bzlmod (probably not possible in workspace). For example I have a feeling that scala_config repository should be merged into toolchains as those are closely related. Maybe this is not an issue at all.

I've wondered about this, but haven't reached a conclusion. scala_config defines constants that are then imported everywhere. It may require a significant refactor to accommodate a different schema.

There are too many scala toolchains. Comment about toolchains ordering is concerning. Issue is that features orthogonal to scala version are configured on toolchain. Ideally there should be single toolchain per scala version. Maybe I'll continue exploration on Configure rules behaviour/toolchain attributes via build settings #1570

Yeah, I haven't thought about that specifically, but I see what you mean, like the unused deps toolchain. That said, I don't think this change introduces any new toolchains from what was already present; it just repackages them to enable a proper Bzlmod API.

Maybe it's a wrong place/time for this, but I just wanted to share so I won't forget it. Or maybe I just want to get second opinion on this :)

No worries! Good to start thinking these through as the notion arises.

- Removes an extraneous `compiler_sources_repo` call. - `scala_toolchains` will now _always_ create the main Scala toolchain repository (there's no longer an option to turn it off). - Registers `@io_bazel_rules_scala_toolchains//...:all` in `scala_register_toolchains.

mbland · 2024-11-08T19:01:01Z

OK, everything's updated, tests are all passing, and I've pushed the changes to the repo. Ready for the next round!

- Removes an extraneous `compiler_sources_repo` call. - `scala_toolchains` will now _always_ create the main Scala toolchain repository (there's no longer an option to turn it off). - Registers `@io_bazel_rules_scala_toolchains//...:all` in `scala_register_toolchains.

simuons · 2024-11-12T13:06:20Z

Hi, @mbland, overall looks good. Let's settle on rules_scala_setup question and I think we are good to go.

@simuons

Requested by @simuons in bazelbuild#1633 to make `rules_scala_setup` and `scala_repositories` more readable while maintaining the existing APIs.

mbland · 2024-11-12T15:30:19Z

@simuons In addition to extracting load_rules_dependencies, I just noticed that I could restore all those setup_scala_toolchains() calls in the WORKSPACE files, since we updated that macro to call native.register_toolchains("@io_bazel_rules_scala_toolchains//...:all").

If you want, I'm happy to update this PR accordingly, do it in another PR, or let it go (since all these updates are internal, existing users probably won't notice, and we're introducing the scala_toolchains macro that'll require a documentation update anyway).

Moves the `toolchain` targets for `//scala:toolchain_type` to a new `@io_bazel_rules_scala_toolchains` repository as a step towards Bzlmodification. Part of bazelbuild#1482. Instantiating toolchains in their own repository enables module extensions to define the the repositories required by those toolchains within the extension's own namespace. Bzlmod users can then register the toolchains from this repository without having to import all the toolchains' dependencies into their own namespace via `use_repo()`. --- The `scala_toolchains_repo()` macro wraps the underlying repository rule and assigns it the standard name `io_bazel_rules_scala_toolchains`. Right now it's only instantiating the main Scala toolchain via the default `scala = True` parameter. Future changes will expand this macro and repository rule with more boolean parameters to instantiate other toolchains, specifically: - `scalatest` - `junit` - `specs2` - `twitter_scrooge` - `jmh` - `scala_proto` and `scala_proto_enable_all_options` - `testing` (includes all of `scalatest`, `junit`, and `specs2`) - `scalafmt` --- `WORKSPACE` users will now have to import and call the `scala_toolchains_repo()` macro to instantiate `@io_bazel_rules_scala_toolchains`. ```py load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config") scala_config() load( "//scala:scala.bzl", "rules_scala_setup", "rules_scala_toolchain_deps_repositories", "scala_toolchains_repo", ) rules_scala_setup() rules_scala_toolchain_deps_repositories() scala_toolchains_repo() register_toolchains("@io_bazel_rules_scala_toolchains//...:all") ``` This is what the corresponding `MODULE.bazel` setup would look like: ```py module(name = "rules_scala", version = "7.0.0") scala_config = use_extension( "//scala/extensions:config.bzl", "scala_config" ) scala_config.settings(scala_version = "2.13.14") scala_deps = use_extension("//scala/extensions:deps.bzl", "scala_deps") scala_deps.toolchains() ``` The `register_toolchains()` call in `WORKSPACE` isn't strictly required at this point, but is recommended. However, all the `WORKSPACE` files in this repo already register their required toolchains using existing macros, which have been updated in this change. In fact, calling `register_toolchains()` right after `scala_toolchains_repo()` as shown above breaks two tests that depend on the existing `WORKSPACE` toolchain registration: - `test_compilation_fails_with_plus_one_deps_undefined` from `test/shell/test_compilation.sh` depends on `scala_register_unused_deps_toolchains()` setting up its toolchain to resolve first. `//scala:unused_dependency_checker_error_toolchain` sets the `scala_toolchain()` parameters `dependency_tracking_method = "ast-plus"` and `unused_dependency_checker_mode = "error"`, and the `@io_bazel_rules_scala_toolchains//scala` toolchains don't. - `test_scala_binary_allows_opt_in_to_use_of_argument_file_in_runner_for_improved_performance` from `test/shell/test_scala_binary.sh` depends on the `use_argument_file_in_runner` parameter of `scala_toolchain` being `False`. This is the default, but the `@io_bazel_rules_scala_toolchains//scala` toolchains explicitly set this to `True` instead. In the Bzlmod case, the `register_toolchains()` call isn't necessary at all. This is because `@io_bazel_rules_scala_toolchains` includes one package per set of toolchains, and the rules_scala `MODULE.bazel` calls `register_toolchains("@io_bazel_rules_scala_toolchains//...:all")`. This will automatically register all configured rules_scala toolchains, while allowing users to override any of them using `register_toolchains()` in their own `MODULE.bazel` files. Technically, the `scala_deps.toolchains()` call isn't required when only using the default `scala = True` parameter; the rules_scala `MODULE.bazel` will instantiate this automatically, too.

Extracted a single `scala_toolchains` macro to share between `WORKSPACE` and the `deps.bzl` module extension. This will make it easier to ensure `WORKSPACE` compatibility, and to add a Bazel module extension as a thin layer on top. Part of bazelbuild#1482. This change includes updates to `rules_scala_setup` and `scala_repositories` to support this, while preserving compatibility with existing `WORKSPACE` calls. The next commit will replace many existing `WORKSPACE` calls with `scala_toolchains`.

Also added `@io_bazel_rules_scala_toolchains//...:all` to `register_toolchains()` calls everywhere, even when not specifically necessary. This proves the mechanism is safe and works with `WORKSPACE` now, and will make future updates to consolidate other toolchains less noisy.

Should've been included in the previous commit. All tests still pass.

Missed this one in third_party/test/proto earlier. Also removed unnecessary `USE_BAZEL_VERSION` expression in test_scala_proto_library.sh. If `USE_BAZEL_VERSION` is set, it takes precedence to begin with, and third_party/test/proto/.bazelversion exists now after bazelbuild#1629.

This turns out to be helpful when adapting `test_version/WORKSPACE.template` to the toolchainized version of `twitter_scrooge` for testing. In this case, we want `twitter_scooge` to be in its own customized repo, separate from the one generated by `scala_toolchains`. This seems like it might be generally useful when writing module extensions for alternative toolchains.

- Removes an extraneous `compiler_sources_repo` call. - `scala_toolchains` will now _always_ create the main Scala toolchain repository (there's no longer an option to turn it off). - Registers `@io_bazel_rules_scala_toolchains//...:all` in `scala_register_toolchains.

@simuons

Requested by @simuons in bazelbuild#1633 to make `rules_scala_setup` and `scala_repositories` more readable while maintaining the existing APIs.

mbland requested review from liucijus and simuons as code owners October 23, 2024 18:46

mbland force-pushed the bzlmod-toolchain-scala branch 2 times, most recently from 85924ce to bd2364a Compare October 25, 2024 23:36

mbland mentioned this pull request Oct 28, 2024

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

Open

simuons requested changes Nov 8, 2024

View reviewed changes

mbland force-pushed the bzlmod-toolchain-scala branch from 59bcad1 to 3ddf04c Compare November 11, 2024 16:27

mbland added a commit to mbland/rules_scala that referenced this pull request Nov 12, 2024

Extract load_rules_dependencies macro

5fdf891

Requested by @simuons in bazelbuild#1633 to make `rules_scala_setup` and `scala_repositories` more readable while maintaining the existing APIs.

mbland added 8 commits November 12, 2024 15:33

Remove obsolete WORKSPACE toolchain calls

9015826

Should've been included in the previous commit. All tests still pass.

Extract load_rules_dependencies macro

297645d

Requested by @simuons in bazelbuild#1633 to make `rules_scala_setup` and `scala_repositories` more readable while maintaining the existing APIs.

mbland force-pushed the bzlmod-toolchain-scala branch from 5fdf891 to 297645d Compare November 12, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolchainize //scala:toolchain_type #1633

Toolchainize //scala:toolchain_type #1633

mbland commented Oct 23, 2024 •

edited

Loading

mbland commented Oct 25, 2024

simuons commented Nov 4, 2024

mbland commented Nov 4, 2024 •

edited

Loading

simuons left a comment

simuons Nov 8, 2024

mbland Nov 8, 2024

simuons Nov 8, 2024

mbland Nov 8, 2024

simuons Nov 8, 2024

mbland Nov 8, 2024

simuons Nov 12, 2024

simuons Nov 8, 2024

mbland Nov 8, 2024

simuons Nov 12, 2024

mbland Nov 12, 2024

simuons Nov 8, 2024

mbland Nov 8, 2024

simuons commented Nov 8, 2024

mbland commented Nov 8, 2024

mbland commented Nov 8, 2024

simuons commented Nov 12, 2024

mbland commented Nov 12, 2024

Toolchainize //scala:toolchain_type #1633

Are you sure you want to change the base?

Toolchainize //scala:toolchain_type #1633

Conversation

mbland commented Oct 23, 2024 • edited Loading

Description

Motivation

mbland commented Oct 25, 2024

simuons commented Nov 4, 2024

mbland commented Nov 4, 2024 • edited Loading

simuons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simuons commented Nov 8, 2024

mbland commented Nov 8, 2024

mbland commented Nov 8, 2024

simuons commented Nov 12, 2024

mbland commented Nov 12, 2024

mbland commented Oct 23, 2024 •

edited

Loading

mbland commented Nov 4, 2024 •

edited

Loading