-
Notifications
You must be signed in to change notification settings - Fork 275
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
Allow configuring multiple Scala versions #1579
Conversation
scala_config.bzl
Outdated
@@ -28,7 +28,7 @@ def _store_config(repository_ctx): | |||
) | |||
|
|||
# All versions supported | |||
scala_versions = [scala_version] | |||
scala_versions = [scala_version] + [v for v in repository_ctx.attr.scala_versions if v != 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.
Hi, @aszady, all looks good to me. Just one question. Shouldn't there be a check that scala_version
(default) is specified in scala_versions
(all supported)?
Now scala_config(scala_version = "2.12.18", scala_versions = ["3.3.1"])
would result in a default version "2.12.18"
and all configured ["2.12.18", "3.3.1"]
I think this is different from what doc says. What do you think? Am I missing something?
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.
The doc currently says only about the variables SCALA_VERSION
, SCALA_VERSIONS
in the generated file @io_bazel_rules_scala_config//:config.bzl
.
We can of course make them more consistent with the args to scala_config
if it is too confusing.
I pushed a revision that enforces containing scala_version
in scala_versions
– instead of silently fixing the user's oversight.
One can also still not provide the scala_versions
at all and it will default to [scala_version]
(compatibility).
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 that use-case without scala_versions
:) somehow managed to forget.
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.
Thanks
Description
In my belief, all the changes required in order to enable this feature are done.
We can flip the switch!
Please find the following draft PRs as a support for this change:
Motivation
#1290