-
Notifications
You must be signed in to change notification settings - Fork 278
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
Toolchainize all testing toolchains #1653
base: master
Are you sure you want to change the base?
Conversation
Note that I've updated this to move the new Bzlmodified For the details on how I think I'll be able to fold in a lot of Bazel 8 + |
c4fa6c2
to
61dfa2e
Compare
I just pushed another commit to update I realized that the old pattern of calling So if we want to keep these macros, it seems like we should maybe restore the original toolchain targets in Happy to implement either decision. Just let me know. |
8c5f61e
to
f62d8a7
Compare
I just added a commit to move |
Moves the Scalatest, JUnit, and Specs2 toolchains into `@io_bazel_rules_scala_toolchains//testing`. Part of bazelbuild#1482. Updates all `WORKSPACE` files to set the appropriate `scala_toolchains` parameters and to remove the unnecessary repository import and toolchain registration macros. Adds a `fetch_sources_by_id` parameter to `repositories` from `third_party/repositories/repositories.bzl`. This enables `scala_toolchains` to build the `artifact_ids_to_fetch_sources` mapping from artifact ID lists returned by new macros extracted from `WORKSPACE` macros. The values assigned to each id match the original `fetch_sources` settings in the corresponding original `WORKSPACE` macros. Updates `scala/scala_maven_import_external.bzl` to generate a `load` line for `//scala:scala_import.bzl` based on the repo's canonical name, not `@io_bazel_rules_scala`. As usual, includes several other opportunistic removals of the `@io_bazel_rules_scala` repo name prefix to avoid an internal dependency on that name. This means Bzlmod users won't necessarily have to set the `repo_name` parameter of `bazel_dep` when using `rules_scala`. --- Introduces more macros to return a framework's Maven artifact dependencies, rather than inlining them in a `repositories` call. These inlined lists are replaced by macro invocations, and now the `scala_toolchains` macro can invoke these macros to collect artifact IDs to pass to `repositories`. This also allows for future changes to introduce a `scala_version` parameter if necessary, similar to how `scalafmt_artifact_ids` already works. This is important to avoid collisions when creating repositories for artifacts upon which more than one framework depends under Bzlmod. `WORKSPACE` doesn't seem affected by these collisions, but Bzlmod will produce errors like the following, where both `scala_proto` and `twitter_scrooge` depend upon `io_bazel_rules_scala_guava`: ```txt $ bazel build //src/... ERROR: .../scala/scala_maven_import_external.bzl:299:24: Traceback (most recent call last): File ".../scala/extensions/deps.bzl", line 140, column 21, in _scala_deps_impl scala_toolchains( File ".../scala/private/macros/toolchains.bzl", line 140, column 17, in scala_toolchains _scrooge( File ".../twitter_scrooge/twitter_scrooge.bzl", line 96, column 17, in twitter_scrooge repositories( File ".../third_party/repositories/repositories.bzl", line 113, column 37, in repositories _scala_maven_import_external( File ".../scala/scala_maven_import_external.bzl", line 263, column 30, in scala_maven_import_external jvm_maven_import_external( File ".../scala/scala_maven_import_external.bzl", line 299, column 24, in jvm_maven_import_external jvm_import_external(jar_urls = jar_urls, srcjar_urls = srcjar_urls, coordinates = artifact, **kwargs) Error in repository_rule: A repo named io_bazel_rules_scala_guava_2_13_15 is already generated by this module extension at .../scala/scala_maven_import_external.bzl:299:24 ERROR: Analysis of target '//src/java/io/bazel/rulesscala/worker:worker_test' failed; build aborted: error evaluating module extension scala_deps in //scala/extensions:deps.bzl ``` Recent updates to `scripts/create_repository.py` (bazelbuild#1639, bazelbuild#1642) make it easy to emit full direct dependency lists for artifacts included in `third_party/repositories/scala_*.bzl`. This increases the likelihood of collisions, since this expanded metadata forces the macros that instantiate artifact repos to instantiate even more repos. By fetching list of artifact IDs from these macros, `scala_toolchains` can now consolidate them into dictionary keys. Then it passes these unique keys to `repositories` directly, avoiding the problem of instantiating the same repo multiple times in the same module extension. This, in turn, also avoids the need to add parameters to the original `WORKSPACE` macros that instantiate dependencies to avoid collisions under Bzlmod. The `scala_toolchains` macro never needs to call these original macros, under either `WORKSPACE` or Bzlmod. Finally, it also reduces duplication between these artifact ID lists and the `_*_DEPS` symbols originally from `testing/BUILD` (and now in `testing/deps.bzl`). The dependency labels are now generated programatically. (Aside: As I mentioned, we may eventually need to pass a Scala version argument to these macros. It will be possible to cross that bridge without too much trouble if and when that day comes. Or I can try to future proof it in a follow up pull request.)
Removes this symbol from `scala/scala.bzl` as well as `setup_scala_testing_toolchain`, and deletes `scala/private/macros/toolchains.bzl`. Part of bazelbuild#1482 and bazelbuild#1652. This is required for Bazel 8 and `rules_java` 8 compatibility, but is also compatible with Bazel 6 and 7. In bazelbuild#1652, @hvadehra suggested partitioning the `.bzl` files such that `WORKSPACE` doesn't `load` a file that tries to `load` symbols from `rules_java`. I successfully did so in a separate branch, and along with other minor changes, got `rules_scala` to build with `rules_java` 8.5.1. The other changes will come in separate pull requests, but it makes sense to land this change now before adding any other toolchains to `scala_toolchains`. --- Arguably, we should remove all macros exported from `scala/scala.bzl` that only instantiate toolchain dependencies and define toolchains. That may be a breaking change for some users, but will ultimately be necessary for these macros to remain compatible with Bazel 8.
Eliminates reliance on the default `@io_bazel_rules_scala_junit_junit` artifact repository.
These macros now mirror the implementation of `scalatest_toolchain()`. However, I realized that the old pattern of calling `scalatest_repositories()` followed by `scalatest_toolchain()` will no longer work without first calling `scala_toolchains(scalatest = True)`. This is because the `alias` targets in `testing/BUILD` that replace the previous implementations all point to `@io_bazel_rules_scala_toolchains`. So if we want to keep these macros, it seems like we should maybe restore the original toolchain targets in `testing/BUILD`. If we don't, we can remove these macros, but we can document these as breaking changes, and update other documentation accordingly.
Like "Move `scala_toolchains` to `scala/toolchains.bzl`", removes the `scala_toolchains_repo` symbol from `scala/scala.bzl` and makes it available from `scala/toolchains_repo.bzl`. This avoids a future `test_scala_version 2.12.20` failure during Bazel 8 builds after adding `twitter_scrooge` toolchain support in the new `test_version/version_specific_tests_dir/scrooge_repositories.bzl` file. Otherwise, this new file would load `toolchains_repo` from `scala/scala.bzl`. The `test_version/test_scala_version_.../WORKSPACE` file generated from `test_version/WORKSPACE.template` would then transitively load `.bzl` files with `rules_java` symbols, breaking the test. ```txt $ RULES_SCALA_TEST_ONLY="test_scala_version 2.12.20" ./test_version.sh ERROR: Traceback (most recent call last): File ".../external/rules_scala/scala/private/common_attributes.bzl", line 18, column 28, in <toplevel> "deps": attr.label_list( Error in label_list: Illegal argument: element in 'providers' is of unexpected type. Either all elements should be providers, or all elements should be lists of providers, but got list with an element of type NoneType. ERROR: Error computing the main repository mapping: at test_version/test_scala_version_.../scrooge_repositories.bzl:1:6: at .../scala/scala.bzl:30:5: at .../scala/private/rules/scala_junit_test.bzl:5:5: initialization of module 'scala/private/common_attributes.bzl' failed ```
f62d8a7
to
edec717
Compare
Description
Moves the Scalatest, JUnit, and Specs2 toolchains into
@io_bazel_rules_scala_toolchains//testing
. Part of #1482.Introduces more macros to return a framework's Maven artifact dependencies, rather than inlining them in a
repositories
call. These inlined lists are replaced by macro invocations, and now thescala_toolchains
macro can invoke these macros to collect artifact IDs to pass torepositories
. This also allows for future changes to introduce ascala_version
parameter if necessary, similar to howscalafmt_artifact_ids
already works.Updates all
WORKSPACE
files to set the appropriatescala_toolchains
parameters and to remove the unnecessary repository import and toolchain registration macros.Adds a
fetch_sources_by_id
parameter torepositories
fromthird_party/repositories/repositories.bzl
.Updates
scala/scala_maven_import_external.bzl
to generate aload
line for//scala:scala_import.bzl
based on the repo's canonical name, not@io_bazel_rules_scala
.As usual, includes several other opportunistic removals of the
@io_bazel_rules_scala
repo name prefix to avoid an internal dependency on that name.Motivation
The macros returning artifact ID lists help avoid collisions when creating repositories dependencies upon which multiple frameworks depend under Bzlmod.
WORKSPACE
doesn't seem affected by these collisions, but Bzlmod will produce errors like the following, where bothscala_proto
andtwitter_scrooge
depend uponio_bazel_rules_scala_guava
:Recent updates to
scripts/create_repository.py
(#1639, #1642) make it easy to emit full direct dependency lists for artifacts included inthird_party/repositories/scala_*.bzl
. This increases the likelihood of collisions, since this expanded metadata forces the macros that instantiate artifact repos to instantiate even more repos.By fetching lists of artifact IDs from these macros,
scala_toolchains
can now consolidate them into dictionary keys. Then it passes these unique keys torepositories
directly, avoiding the problem of instantiating the same repo multiple times in the same module extension.This, in turn, also avoids the need to add parameters to the original
WORKSPACE
macros that instantiate dependencies to avoid collisions under Bzlmod. Thescala_toolchains
macro never needs to call these original macros, under eitherWORKSPACE
or Bzlmod.Finally, it also reduces duplication between these artifact ID lists and the
_*_DEPS
symbols originally fromtesting/BUILD
(and now intesting/deps.bzl
). The dependency labels are now generated programatically.(Aside: As I mentioned, we may eventually need to pass a Scala version argument to these macros. It will be possible to cross that bridge without too much trouble if and when that day comes. Or I can try to future proof it in a follow up pull request.)
Adding the
fetch_sources_by_id
parameter torepositories()
enablesscala_toolchains
to build theartifact_ids_to_fetch_sources
mapping from artifact ID lists returned by these new macros. The values assigned to each id match the originalfetch_sources
settings in the corresponding originalWORKSPACE
macros.Removing
@io_bazel_rules_scala
target prefixes means Bzlmod users won't necessarily have to set therepo_name
parameter ofbazel_dep
when usingrules_scala
.