Skip to content

Conversation

@aszady
Copy link
Contributor

@aszady aszady commented May 15, 2024

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

@aszady aszady marked this pull request as ready for review June 3, 2024 08:52
@aszady aszady requested review from liucijus and simuons as code owners June 3, 2024 08:52
scala_config.bzl Outdated

# All versions supported
scala_versions = [scala_version]
scala_versions = [scala_version] + [v for v in repository_ctx.attr.scala_versions if v != scala_version]
Copy link
Collaborator

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?

Copy link
Contributor Author

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).

Copy link
Collaborator

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.

Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@liucijus liucijus merged commit d4d8316 into bazel-contrib:master Jun 4, 2024
@aszady aszady deleted the enable branch June 11, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants