Minimal cross-build support #1546

mateuszkuta256 · 2024-02-13T10:14:46Z

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

#1290

simuons · 2024-02-13T17:26:39Z

scala/private/common_attributes.bzl

@@ -90,6 +90,30 @@ implicit_deps = {
 default = Label("@io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac"),
 allow_files = True,
 ),
+ "_scalac_before_2_12_13": attr.label(


Hi, @mateuszkuta256, thanks for PR. Does that mean that every scala version needs to be setup in workspace for this toolchain to work? ie all flavours of scalac worker needs to be compiled?

hey, let me explain this change in detail, as I am searching for an alternative solution anyway...
sources for scalac are defined here
these are conditional upon: if SCALA_MAJOR_VERSION.startswith("2")
so I simply made a few scalac_... targets, separate for 2.11, 2 and 3
would love to keep a single scalac, but missing an idea of how to achieve that...
if we don't specify e.g. scala 3.3.1 in a config setting then I believe _scalac3 is skipped (?)
at least the build is successful without complaining that scala3 deps are missing

Right, I see. At this point it's just a label and magic happens on action when you select scalac.

As for alternatives I was playing with toolchain setup where each scala version is encapsulated/scoped in it's own repository and compiles it's own scalac (similar to remote_java_repository). simuons@919471a Still not sure where this leads to.

I see you got rid of the circular dependency between toolchain and scalac caused by toolchain_deps:scala_compile_classpath dep. It is something that I was missing in my first drafts and allows to apply nice solutions 👍
in the meantime, I tested something like this mateuszkuta256@f247cc8
_scalac is removed from implicit attributes and attached to scala_library and other rules. Then we can make a version specific implementations, e.g.
scala_3_3_binary = make_scala_binary(scala_version = "3.3.1")
since you found a way to remove circular dependencies, the same thing can be applied directly on the toolchain here
from the user perspective, it's even nicer, because can use regular scala_library without making version specific one

scala/private/phases/phase_compile.bzl

mateuszkuta256 · 2024-02-20T10:00:25Z

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: mateuszkuta256@f247cc8

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 Minimal cross-build support #1546 (comment):
mateuszkuta256@4622b97
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 mateuszkuta256@0c0be9b#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 · 2024-02-22T06:55:00Z

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

mateuszkuta256 · 2024-02-23T08:59:24Z

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 · 2024-03-04T15:16:52Z

@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

This looks really promising and exciting. Probably transitions is the way to go (just need to be careful with memory and performance considerations)

However I think this change is quite big and should be split in few steps:

As a first step I think we should move to toolchains and get rid of branching on SCALA_VERSION constant (I feel this needed for bzlmod as well)
Then probably 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
Then add transitions which should be considerably easier when having scala version selection by toolchain or build_setting (instead of env var)

simuons · 2024-03-05T08:09:24Z

scala/private/macros/setup_scala_toolchain.bzl

+ )
+ for scala_version in SCALA_VERSIONS:
+ sanitized_scala_version = sanitize_version(scala_version)
+ setup_scala_toolchain(


This looks a bit problematic. I mean this will set up toolchains with default settings except classpath. This probably shows abuse of toolchains. I'm thinking maybe toolchain should contain scala version specific settings (ie classpath and scalac_opts) while other settings like strict_deps_mode etc should be expressed as build_settings.

Of course toolchains can be set up manually but looks a bit tedious.

simuons · 2024-03-05T08:14:44Z

scala/private/phases/phase_compile.bzl

@@ -105,6 +104,9 @@ def phase_compile_common(ctx, p):
 return _phase_compile_default(ctx, p)

 def _phase_compile_default(ctx, p, _args = struct()):
+ scala_version = ctx.attr.scala_version if hasattr(ctx.attr, "scala_version") and ctx.attr.scala_version != "" else SCALA_VERSION
+ buildijar_default_value = True if scala_version.startswith("2.") else False


I would prefer this to go into scala_toolchain rule implementation an be exposed as a property in a returned provider. I'd like to get rid of branching in rules/phases by scala version if possible. I think toolchains is proper place for such things because it already knows for which scala version it is defined.

simuons · 2024-03-05T08:19:35Z

scala/versions/versions.bzl

+load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
+load("@io_bazel_rules_scala//scala:scala_cross_version.bzl", "extract_major_version")
+
+_SCALA_VERSIONS = [


I don't think it is feasible to hold all possible scala versions here. I think versions/build_setting/config_setting should be generated in scala_config repository.

simuons · 2024-03-05T08:24:09Z

scala_config.bzl

 "enable_compiler_dependency_tracking": attr.bool(
 mandatory = True,
 ),
 },
- environ = ["SCALA_VERSION", "ENABLE_COMPILER_DEPENDENCY_TRACKING"],
+ environ = ["SCALA_VERSION", "SCALA_VERSIONS", "ENABLE_COMPILER_DEPENDENCY_TRACKING"],


I think SCALA_VERSIONS is not needed in environ. Well SCALA_VERSION is here because it was a hacky solution to test different scala versions. I think once we move to proper toolchains then SCALA_VERSION should be replaced with --extra_toolchains or with --@rules_scala//scala:version=3.3.1 build setting.

simuons · 2024-03-05T08:28:29Z

scala_config.bzl

 "enable_compiler_dependency_tracking": attr.bool(
 mandatory = True,
 ),
 },
- environ = ["SCALA_VERSION", "ENABLE_COMPILER_DEPENDENCY_TRACKING"],
+ environ = ["SCALA_VERSION", "SCALA_VERSIONS", "ENABLE_COMPILER_DEPENDENCY_TRACKING"],
 )

 def scala_config(
 scala_version = _default_scala_version(),


I'm thinking what should be the semantics of scala_version and scala_versions. I think scala_versions should contain all versions required by repository to build and scala_version should signify default version (ie default value for scala_version build setting). For backwards compatibility scala_versions could be optional and default to [scala_version]

simuons · 2024-03-05T08:38:38Z

examples/crossbuild/WORKSPACE

@@ -0,0 +1,47 @@
+workspace(name = "cross_build")


Could you add this workspace to /test/shell/test_examples.sh

simuons · 2024-03-05T09:34:17Z

@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 · 2024-03-06T12:05:23Z

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 · 2024-03-07T10:31:09Z

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 · 2024-03-08T09:15:56Z

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 · 2024-03-08T09:49:30Z

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 · 2024-03-08T11:15:57Z

I extracted the first part of this PR into a smaller one #1552
another PR will add a transition for toolchain selection
poc for the whole feature

mateuszkuta256 added 9 commits February 12, 2024 09:55

config property for supported scala versions

19d7cff

versions config setting

79789c0

register toolchain repositories with various labels

000c0fc

register toolchain for each supported scala version

d8259b1

add version transition to scala rules

7afc81a

introduce 'scalac' for each version

a02f738

examples

f530160

tests

6af4850

lint

fdf2929

mateuszkuta256 requested review from liucijus and simuons as code owners February 13, 2024 10:14

mateuszkuta256 changed the title ~~Cross build support~~ Minimal cross-build support Feb 13, 2024

simuons reviewed Feb 13, 2024

View reviewed changes

simuons reviewed Feb 15, 2024

View reviewed changes

scala/private/phases/phase_compile.bzl Show resolved Hide resolved

mateuszkuta256 requested a review from simuons February 27, 2024 10:13

mateuszkuta256 added 2 commits February 27, 2024 17:15

register config settings in a loop

33ffea6

compile phase fix

884c503

simuons requested changes Mar 5, 2024

View reviewed changes

mateuszkuta256 mentioned this pull request Mar 8, 2024

Introduce 'SCALA_VERSIONS' config setting #1552

Draft

mateuszkuta256 marked this pull request as draft March 8, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal cross-build support #1546

Minimal cross-build support #1546

mateuszkuta256 commented Feb 13, 2024 •

edited

Loading

simuons Feb 13, 2024 •

edited

Loading

mateuszkuta256 Feb 14, 2024

simuons Feb 15, 2024 •

edited

Loading

mateuszkuta256 Feb 16, 2024

mateuszkuta256 commented Feb 20, 2024 •

edited

Loading

simuons commented Feb 22, 2024

mateuszkuta256 commented Feb 23, 2024 •

edited

Loading

lukaszwawrzyk commented Mar 4, 2024

simuons left a comment

simuons Mar 5, 2024

simuons Mar 5, 2024

simuons Mar 5, 2024

simuons Mar 5, 2024

simuons Mar 5, 2024

simuons Mar 5, 2024

simuons commented Mar 5, 2024

mateuszkuta256 commented Mar 6, 2024

mateuszkuta256 commented Mar 7, 2024

simuons commented Mar 8, 2024

mateuszkuta256 commented Mar 8, 2024

mateuszkuta256 commented Mar 8, 2024

Minimal cross-build support #1546

Are you sure you want to change the base?

Minimal cross-build support #1546

Conversation

mateuszkuta256 commented Feb 13, 2024 • edited Loading

Description

Motivation

simuons Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simuons Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszkuta256 commented Feb 20, 2024 • edited Loading

simuons commented Feb 22, 2024

mateuszkuta256 commented Feb 23, 2024 • edited Loading

lukaszwawrzyk commented Mar 4, 2024

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

simuons commented Mar 5, 2024

mateuszkuta256 commented Mar 6, 2024

mateuszkuta256 commented Mar 7, 2024

simuons commented Mar 8, 2024

mateuszkuta256 commented Mar 8, 2024

mateuszkuta256 commented Mar 8, 2024

mateuszkuta256 commented Feb 13, 2024 •

edited

Loading

simuons Feb 13, 2024 •

edited

Loading

simuons Feb 15, 2024 •

edited

Loading

mateuszkuta256 commented Feb 20, 2024 •

edited

Loading

mateuszkuta256 commented Feb 23, 2024 •

edited

Loading