Minimal cross-build support

mateuszkuta256 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

mateuszkuta256 commented 5 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

bazelbuild / rules_scala

Minimal cross-build support #1546

Description

Motivation