-
-
Notifications
You must be signed in to change notification settings - Fork 287
Minimal cross-build support #1546
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
Minimal cross-build support #1546
Conversation
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
A brief summary:
this one works with BSP perfectly
@simuons @lukaszwawrzyk feel free to test and share your thoughts on which of these is superior |
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 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
) | ||
for scala_version in SCALA_VERSIONS: | ||
sanitized_scala_version = sanitize_version(scala_version) | ||
setup_scala_toolchain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
load("@bazel_skylib//rules:common_settings.bzl", "string_flag") | ||
load("@io_bazel_rules_scala//scala:scala_cross_version.bzl", "extract_major_version") | ||
|
||
_SCALA_VERSIONS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"enable_compiler_dependency_tracking": attr.bool( | ||
mandatory = True, | ||
), | ||
}, | ||
environ = ["SCALA_VERSION", "ENABLE_COMPILER_DEPENDENCY_TRACKING"], | ||
environ = ["SCALA_VERSION", "SCALA_VERSIONS", "ENABLE_COMPILER_DEPENDENCY_TRACKING"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
@@ -0,0 +1,47 @@ | |||
workspace(name = "cross_build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this workspace to /test/shell/test_examples.sh
@lukaszwawrzyk I think doc makes a lot of sense because change is substantial.
Thanks for this comment. I was unsure do you consider this to be merged or exploring design. |
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/
In there I introduced
there is no more statement like 'if SCALA_VERSION' - instead I rely on elements of 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" |
one more question @simuons
you mean to introduce same settings as for
such a change wouldn't be backward-compatible, or? |
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 |
Closing as it was completed as part of #1290 |
Description
This PR allows the registration of multiple toolchains for various scala versions.
Extends both
scala_library
andscala_binary
withscala_version
parameter which allows to pick proper toolchainDetailed explanation here
I am already researching the next steps:
Gonna add some documentation when the API is approved
Motivation
#1290