Toolchainize //scala/scalafmt:scalafmt_toolchain #1678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a pretty straightforward and easy update on top of the previous `setup_toolchains` and `scala_toolchains_repo` changes. Part of bazelbuild#1482.
Replaces the nonexistent `//test/coverage/...` target with packages that actually exist, rendering the test more meaningful. A spurious failure caused me to run the following to inspect what was happening: ```txt RULES_SCALA_TEST_ONLY=test_build_is_identical ./test_reproducibility.sh ``` This caused me to see that the following command was failing because `//test/coverage/...` didn't exist: ```txt bazel build --collect_code_coverage -- //test/coverage/... ```
Reduces string duplication in `//scala/scalafmt/toolchain/*.bzl` files.
It's likely that no one will ever rely on the default when defining their own `scalafmt_toolchain`. While investigating bazelbuild#1675, I realized the `dep_providers` default was set to a nonexistent target. This didn't break our tests because our Scalafmt toolchains are created by `setup_scala_toolchain`, which sets `dep_providers` explicitly. I thought about aliasing the provider generated for the toolchain for `SCALA_VERSION` in `@io_bazel_rules_scala_toolchains//scalafmt`, or generating a new one. I ultimately decided to remove the default, because the `deps_providers` is literally the only attribute. Anyone defining their own `scalafmt_toolchain` will certainly define their own `deps_providers` target(s).
Copied the regular expression used to optionally match canonical repo name prefixes in `buildozer` commands from bazelbuild#1622. These never failed when building with Bzlmod and Bazel 7.4.1, but did under Bazel 6.5.0.
Fixes `{strict_deps,unused_dependency_checker}_test` from
`//third_party/dependency_analyzer/src/test` under Bazel 8 by updating
`$(location)` to `$(rootpath)`. Part of bazelbuild#1652.
`//third_party/dependency_analyzer/src/test:strict_deps_test` and
`//third_party/dependency_analyzer/src/test:unused_dependency_checker_test`
both failed with errors of the form:
```txt
StrictDepsTest:
- error on indirect dependency target *** FAILED *** (379 milliseconds)
nice errors on sequence of strings.this.infos.exists(nice errors on
sequence of strings.this.checkErrorContainsMessage(target)) was false
expected an error on //commons:Target to appear in errors (with
buildozer command)!
Errors: List(object apache is not a member of package org)
(StrictDepsTest.scala:85)
```
This happened both under `WORKSPACE` and Bzlmod. Copying the test script
with the following (with `$ID` being one of `7`, `8`, `7-updated`, or
`8-updated`) helped reveal the differences:
```txt
cp bazel-bin/third_party/dependency_analyzer/src/test/strict_deps_test \
strict_deps_test-$ID.sh
```
Under Bazel 7, we find the following lines whether using `$(location)`
or `$(rootpath)` on the `org.apache.commons` artifact (the other
artifacts already use `$(rootpath)`:
```txt
-Dscala.library.location=external/io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar
-Dscala.reflect.location=external/io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar
-Dguava.jar.location=external/com_google_guava_guava_21_0_with_file/guava-21.0.jar
-Dapache.commons.jar.location=external/org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar
```
Under Bazel 8, notice that the `external/` prefix has become `../` for
the other paths already using `$(rootpath)`, but remains `external/` for
the `$(location)` artifact. This breaks the Bazel 8 build:
```txt
-Dscala.library.location=../io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar
-Dscala.reflect.location=../io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar
-Dguava.jar.location=../com_google_guava_guava_21_0_with_file/guava-21.0.jar
-Dapache.commons.jar.location=external/org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar
```
Under Bazel 8, using `$(rootpath)` for the `org.apache.commons` artifact
converts its `external/`, fixing the Bazel 8 build:
```txt
-Dscala.library.location=../io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar
-Dscala.reflect.location=../io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar
-Dguava.jar.location=../com_google_guava_guava_21_0_with_file/guava-21.0.jar
-Dapache.commons.jar.location=../org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar
```
This was referenced Jan 15, 2025
liucijus
approved these changes
Jan 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Mostly a pretty straightforward and easy update on top of the previous
setup_toolchainsandscala_toolchains_repochanges. Part of #1482.However, it also includes a few opportunistic changes:
test_reproducibility.shto replace the nonexistent//test/coverage/...target with packages thatactually exist, rendering the test more meaningful.
SCALAFMT_TOOLCHAIN_TYPEconstant to reduce string duplication.dep_providersdefault fromscala_toolchain, since it's the only attribute, so it doesn't make sense to have a default (inspired by scalafmt compilation breaks after upgrade to "Update to Scalafmt 3.8.3" #1675).//third_partytests to replace$(location)with$(rootpath), ensuring they still pass under Bazel 8 (Bazel 8.0.0 compatibility #1652).See the commit messages for more details.
Motivation
The first commit contains the next methodical step in the implementation of the
scala_toolchains()macro and the@io_bazel_rules_scala_toolchainsrepository. This "toolchainization" is what will enable us to add aMODULE.bazelfile to registerrules_scalatoolchains automatically, based on optional user configuration values.Normally I'd split each of the other changes into separate pull requests, but since these are so small, it seemed it might expedite the process to include them all here. All are completely tested, and since they're encapsulated in separate commits, I'm happy to break them down into separate pull requests if preferred.