Minimal cross-build support

[mateuszkuta256]

trafficstars

Description

This PR allows the registration of multiple toolchains for various scala versions. Extends both scala_library and scala_binary with scala_version parameter which allows to pick proper toolchain Detailed explanation here

I am already researching the next steps:

• support scalafmt for multiple scala versions
• fix glitches with BSP
• extend scala_test rule too
• support for scalac_bootstrap

Gonna add some documentation when the API is approved

Motivation

https://github.com/bazelbuild/rules_scala/issues/1290

[mateuszkuta256]

A brief summary:

• in this PR I define a few _scalac targets that are included in common_attributes and selected while 'compile phase' depending on scala_version:

def _select_scalac(ctx):
    scala_version = ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].scala_version
    if scala_version:
        major_version = extract_major_version(scala_version)
        minor_version = extract_minor_version(scala_version)
        if major_version.startswith("2.11") or (major_version.startswith("2.12") and int(minor_version) < 13):
            return ctx.executable._scalac_before_2_12_13
...

• the other solution is to build separate rules scala_library, scala_binary, etc. that encapsulate corresponding _scalac: https://github.com/mateuszkuta256/rules_scala/commit/f247cc8a8d0ad249d78743f2059106a91a10a4c9

scala_2_13_binary = make_scala_binary(scala_version = "2.13.12")
scala_3_1_binary = make_scala_binary(scala_version = "3.1.0")

this one works with BSP perfectly

• an update of https://github.com/bazelbuild/rules_scala/pull/1546#discussion_r1490748804: https://github.com/mateuszkuta256/rules_scala/commit/4622b97c8edbb67cf18b5f4bc5cf3727f03e2336 scalac targets are built circular dependencies so can add those directly to the toolchain. Then 'compile phase' does: ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].scalac instead of ctx.executable._scalac

• make _scalac a list of attributes https://github.com/mateuszkuta256/rules_scala/commit/0c0be9b2a942f7964e80221a8accb0fd6dc306e8#diff-2a1ebcef258f4b2437c9b7313023bbc47fae65dd8fbbb68d3f6a79cb1a96783cR89 instead of adding the workers manually, we can register a list by looping over supported SCALA_VERSIONS can select proper one during compile phase:

def _select_scalac(ctx):
    if hasattr(ctx.attr, "scala_version"):
        version_suffix = sanitize_version(ctx.attr.scala_version)
        for scalac in ctx.attr._scalac:
            if scalac.label.name.endswith(version_suffix):
                return scalac.files_to_run
    return ctx.attr._scalac[0].files_to_run

@simuons @lukaszwawrzyk feel free to test and share your thoughts on which of these is superior

[simuons]

Hi, @mateuszkuta256, sorry for delays. I'll try to take a deeper look on weekend.

[mateuszkuta256]

Hi, @mateuszkuta256, sorry for delays. I'll try to take a deeper look on weekend.

thx! FYI I am checking the remaining rules in the context of cross-build there's a same problem for e.g. scala_test - parameter _scalatest_reporter is built differently, depending on SCALA_VERSION what I propose is to make it a label_list, instead of passing few different attributes for each version https://github.com/mateuszkuta256/rules_scala/blob/356c088ccc8d0c0f98113d73afee25180f26f6e2/scala/private/rules/scala_test.bzl#L81 then, proper version can be selected like this: https://github.com/mateuszkuta256/rules_scala/blob/356c088ccc8d0c0f98113d73afee25180f26f6e2/scala/private/phases/phase_collect_jars.bzl#L22 and we don't define more targets than required, only one per scala_version requested by the user https://github.com/mateuszkuta256/rules_scala/blob/356c088ccc8d0c0f98113d73afee25180f26f6e2/scala/support/support.bzl#L98 we can consider something similar for scalac too

[lukaszwawrzyk]

@simuons Can we do anything to help with the review? Or maybe someone else could also take a look?

At this point it would be good to take a look at the overall design and pick concrete solutions. If you'd feel that it is needed, we can write some doc to explain what happens here in more detail. Let us know if you'd like it or just the comments here and code is enough.

Once we are settled on the overall idea, we can try to split these changes into smaller PRs if it would be helpful.

[simuons]

@lukaszwawrzyk I think doc makes a lot of sense because change is substantial.

Once we are settled on the overall idea, we can try to split these changes into smaller PRs if it would be helpful.

Thanks for this comment. I was unsure do you consider this to be merged or exploring design.

[mateuszkuta256]

thx @simuons, this PR will be split into a few smaller ones. I already prepared the first part: https://github.com/mateuszkuta256/rules_scala/commits/multiple_scala_versions/

get rid of branching on SCALA_VERSION

In there I introduced scala_versions config property with the semantics you suggested:

• scala_version - signifies the default version
• scala_versions - contains all versions required by the repository to build. Default to [scala_version]

there is no more statement like 'if SCALA_VERSION' - instead I rely on elements of SCALA_VERSIONS for example, the configuration like this: scala_version = "2.13.12", scala_versions = ["3.3.1"] will register two targets: scalac_2_13_12, scalac_3_3_1 instead of single target : scalac -> srcs = [...] if SCALA_VERSION...

please take a brief look and confirm this is what you expected, in the meantime I'm gonna continue with "move some of the toolchains properties to build_settings"

[mateuszkuta256]

one more question @simuons

move some of the toolchains properties to build_settings meaning toolchains will contain only scala version specific stuff. Otherwise we will have a combinatorial explosion of scala versions and properties like strict_deps

you mean to introduce same settings as for scala_version? like:

config_setting(
    name = "strict_deps",
    flag_values = ...
)

such a change wouldn't be backward-compatible, or? also, one could argue that global settings for each toolchain are not 'elastic' enough (sounds ok for me, just wanna assure we are on the same page)

[simuons]

Hi, @mateuszkuta256, I think I've introduced more confusion by mentioning build_settings. I thought about it and looks like it's not relevant now. So let's not concentrate on that (I'll share latter what I had in mind). Looks like your multiple_scala_version branch attempts to solve smaller scope of this issue. I took a glance at it and think maybe you should open a PR with that and we could move discussion there. After this will be sorted out you will add transitions. wdyt?

[mateuszkuta256]

Hi, @mateuszkuta256, I think I've introduced more confusion by mentioning build_settings. I thought about it and looks like it's not relevant now. So let's not concentrate on that (I'll share latter what I had in mind). Looks like your multiple_scala_version branch attempts to solve smaller scope of this issue. I took a glance at it and think maybe you should open a PR with that and we could move discussion there. After this will be sorted out you will add transitions. wdyt?

awesome! In general it's a good idea to split this feature into smaller parts. Soon I will open a new PR that is about 'scala_version' property only fyi this branch contains 'transition' part and resolves the remaining comments too

[mateuszkuta256]

I extracted the first part of this PR into a smaller one https://github.com/bazelbuild/rules_scala/pull/1552 another PR will add a transition for toolchain selection poc for the whole feature