Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Nov 27, 2024

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

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.

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.

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 both scala_proto and twitter_scrooge depend upon io_bazel_rules_scala_guava:

$ 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 (#1639, #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 lists 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.)

Adding the fetch_sources_by_id parameter to repositories() enables scala_toolchains to build the artifact_ids_to_fetch_sources mapping from artifact ID lists returned by these new macros. The values assigned to each id match the original fetch_sources settings in the corresponding original WORKSPACE macros.

Removing @io_bazel_rules_scala target prefixes means Bzlmod users won't necessarily have to set the repo_name parameter of bazel_dep when using rules_scala.

@mbland
Copy link
Contributor Author

mbland commented Dec 2, 2024

Note that I've updated this to move the new Bzlmodified scala_toolchains() macro to //scala:toolchains.bzl per #1652, to start the process of partitioning WORKSPACE/Bzlmod macros off from BUILD macros. I've prepped a bunch of follow up commits to migrate the rest of the toolchains already, which also follow this principle.

For the details on how I think I'll be able to fold in a lot of Bazel 8 + rules_java 8 prep at the same time as the Bzlmod changes, see: #1652 (comment)

@mbland mbland force-pushed the bzlmod-toolchain-testing branch from c4fa6c2 to 61dfa2e Compare December 2, 2024 05:16
@mbland
Copy link
Contributor Author

mbland commented Dec 2, 2024

I just pushed another commit to update junit_toolchain() and specs2_junit_toolchain() to match scalatest_toolchain(). But I also noticed a problem. Borrowing from the commit message:

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.

Happy to implement either decision. Just let me know.

@mbland mbland force-pushed the bzlmod-toolchain-testing branch 2 times, most recently from 8c5f61e to f62d8a7 Compare December 10, 2024 17:51
@mbland
Copy link
Contributor Author

mbland commented Dec 10, 2024

I just added a commit to move scala/private/macros/toolchains_repo.bzl to scala/toolchains_repo.bzl for reasons explained in the commit message. Then I rebased this on #1671, which should get merged before this pull request, to ensure the build continues to pass.

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
```
@mbland mbland force-pushed the bzlmod-toolchain-testing branch from f62d8a7 to edec717 Compare December 16, 2024 15:56
@mbland
Copy link
Contributor Author

mbland commented Dec 16, 2024

Rebased after #1671 and #1672.

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.

1 participant