Skip to content

Commit

Permalink
Toolchainize //scala/scalafmt:scalafmt_toolchain (bazelbuild#1678)
Browse files Browse the repository at this point in the history
* Toolchainize //scala/scalafmt:scalafmt_toolchain

This is a pretty straightforward and easy update on top of the previous
`setup_toolchains` and `scala_toolchains_repo` changes. Part of bazelbuild#1482.

* Make test_reproducibility.sh build test/coverage_*

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

* Export `SCALAFMT_TOOLCHAIN_TYPE` constant

Reduces string duplication in `//scala/scalafmt/toolchain/*.bzl` files.

* Remove scalafmt_toolchain dep_providers default

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

* Update tests for Bazel 6 + Bzlmod

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.

* Fix `//third_party` tests with `$(rootpath)`

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
```
  • Loading branch information
mbland authored Jan 27, 2025
1 parent a8ae50e commit 08ab275
Show file tree
Hide file tree
Showing 14 changed files with 80 additions and 57 deletions.
7 changes: 1 addition & 6 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ load("//scala:toolchains.bzl", "scala_toolchains")

scala_toolchains(
fetch_sources = True,
scalafmt = True,
testing = True,
)

Expand All @@ -71,12 +72,6 @@ load("//scala_proto:scala_proto.bzl", "scala_proto_repositories")

scala_proto_repositories()

load("//scala/scalafmt:scalafmt_repositories.bzl", "scalafmt_default_config", "scalafmt_repositories")

scalafmt_default_config()

scalafmt_repositories()

# needed for the cross repo proto test
local_repository(
name = "proto_cross_repo_boundary",
Expand Down
8 changes: 4 additions & 4 deletions scala/scalafmt/BUILD
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
load("//scala/scalafmt/toolchain:setup_scalafmt_toolchain.bzl", "setup_scalafmt_toolchains")
load("//scala/scalafmt/toolchain:toolchain.bzl", "export_scalafmt_deps")
load("//scala/scalafmt:phase_scalafmt_ext.bzl", "scalafmt_singleton")
load("//scala:scala.bzl", "scala_binary")
Expand Down Expand Up @@ -37,12 +36,13 @@ scalafmt_singleton(
visibility = ["//visibility:public"],
)

setup_scalafmt_toolchains()

# Alias for backward compatibility:
alias(
name = "scalafmt_toolchain",
actual = "scalafmt_toolchain" + version_suffix(SCALA_VERSION),
actual = (
"@io_bazel_rules_scala_toolchains//scalafmt:scalafmt_toolchain" +
version_suffix(SCALA_VERSION)
),
)

export_scalafmt_deps(
Expand Down
8 changes: 4 additions & 4 deletions scala/scalafmt/phase_scalafmt_ext.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ ext_scalafmt = {
),
"_fmt": attr.label(
cfg = "exec",
default = "//scala/scalafmt",
default = Label("//scala/scalafmt"),
executable = True,
),
"_java_host_runtime": attr.label(
Expand All @@ -30,19 +30,19 @@ ext_scalafmt = {
),
"_runner": attr.label(
allow_single_file = True,
default = "//scala/scalafmt:runner",
default = Label("//scala/scalafmt:runner"),
),
"_testrunner": attr.label(
allow_single_file = True,
default = "//scala/scalafmt:testrunner",
default = Label("//scala/scalafmt:testrunner"),
),
},
"outputs": {
"scalafmt_runner": "%{name}.format",
"scalafmt_testrunner": "%{name}.format-test",
},
"phase_providers": [
"//scala/scalafmt:phase_scalafmt",
Label("//scala/scalafmt:phase_scalafmt"),
],
}

Expand Down
20 changes: 6 additions & 14 deletions scala/scalafmt/scalafmt_repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ load(
"version_suffix",
_default_maven_server_urls = "default_maven_server_urls",
)
load(
"//scala_proto/default:repositories.bzl",
"SCALAPB_COMPILE_ARTIFACT_IDS",
)
load("//third_party/repositories:repositories.bzl", "repositories")
load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSIONS")

Expand Down Expand Up @@ -32,22 +36,16 @@ def scalafmt_default_config(path = ".scalafmt.conf", **kwargs):
_SCALAFMT_DEPS = [
"com_geirsson_metaconfig_core",
"com_geirsson_metaconfig_typesafe_config",
"com_google_protobuf_protobuf_java",
"com_lihaoyi_fansi",
"com_lihaoyi_fastparse",
"com_lihaoyi_sourcecode",
"com_typesafe_config",
"org_scala_lang_modules_scala_collection_compat",
"org_scala_lang_scalap",
"org_scalameta_common",
"org_scalameta_parsers",
"org_scalameta_scalafmt_core",
"org_scalameta_scalameta",
"org_scalameta_trees",
"org_typelevel_paiges_core",
"scala_proto_rules_scalapb_lenses",
"scala_proto_rules_scalapb_runtime",
]
] + SCALAPB_COMPILE_ARTIFACT_IDS

_SCALAFMT_DEPS_2_11 = [
"com_lihaoyi_pprint",
Expand Down Expand Up @@ -79,8 +77,7 @@ def scalafmt_artifact_ids(scala_version):

def scalafmt_repositories(
maven_servers = _default_maven_server_urls(),
overriden_artifacts = {},
bzlmod_enabled = False):
overriden_artifacts = {}):
for scala_version in SCALA_VERSIONS:
repositories(
scala_version = scala_version,
Expand All @@ -89,11 +86,6 @@ def scalafmt_repositories(
overriden_artifacts = overriden_artifacts,
)

if not bzlmod_enabled:
_register_scalafmt_toolchains()

def _register_scalafmt_toolchains():
for scala_version in SCALA_VERSIONS:
native.register_toolchains(str(Label(
"//scala/scalafmt:scalafmt_toolchain" +
version_suffix(scala_version),
Expand Down
10 changes: 6 additions & 4 deletions scala/scalafmt/toolchain/setup_scalafmt_toolchain.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
load("//scala/scalafmt/toolchain:toolchain.bzl", "scalafmt_toolchain")
load(
"//scala/scalafmt/toolchain:toolchain.bzl",
"SCALAFMT_TOOLCHAIN_TYPE",
"scalafmt_toolchain",
)
load("//scala/scalafmt:scalafmt_repositories.bzl", "scalafmt_artifact_ids")
load("//scala:providers.bzl", "declare_deps_provider")
load("//scala:scala_cross_version.bzl", "version_suffix")
Expand Down Expand Up @@ -28,9 +32,7 @@ def setup_scalafmt_toolchain(
version_suffix(scala_version),
],
toolchain = ":%s_impl" % name,
toolchain_type = Label(
"//scala/scalafmt/toolchain:scalafmt_toolchain_type",
),
toolchain_type = SCALAFMT_TOOLCHAIN_TYPE,
visibility = visibility,
)

Expand Down
18 changes: 7 additions & 11 deletions scala/scalafmt/toolchain/toolchain.bzl
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
load("@io_bazel_rules_scala//scala:providers.bzl", _DepsInfo = "DepsInfo")
load("//scala/private/toolchain_deps:toolchain_deps.bzl", "expose_toolchain_deps")

SCALAFMT_TOOLCHAIN_TYPE = Label(
"//scala/scalafmt/toolchain:scalafmt_toolchain_type",
)

def _scalafmt_toolchain_impl(ctx):
toolchain = platform_common.ToolchainInfo(
dep_providers = ctx.attr.dep_providers,
Expand All @@ -10,20 +14,12 @@ def _scalafmt_toolchain_impl(ctx):
scalafmt_toolchain = rule(
_scalafmt_toolchain_impl,
attrs = {
"dep_providers": attr.label_list(
default = [
"@io_bazel_rules_scala//scala/scalafmt:scalafmt_classpath_provider",
],
providers = [_DepsInfo],
),
"dep_providers": attr.label_list(providers = [_DepsInfo]),
},
)

def _export_scalafmt_deps_impl(ctx):
return expose_toolchain_deps(
ctx,
"@io_bazel_rules_scala//scala/scalafmt/toolchain:scalafmt_toolchain_type",
)
return expose_toolchain_deps(ctx, SCALAFMT_TOOLCHAIN_TYPE)

export_scalafmt_deps = rule(
_export_scalafmt_deps_impl,
Expand All @@ -32,6 +28,6 @@ export_scalafmt_deps = rule(
mandatory = True,
),
},
toolchains = ["@io_bazel_rules_scala//scala/scalafmt/toolchain:scalafmt_toolchain_type"],
toolchains = [SCALAFMT_TOOLCHAIN_TYPE],
incompatible_use_toolchain_transition = True,
)
24 changes: 22 additions & 2 deletions scala/toolchains.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

load("//junit:junit.bzl", "junit_artifact_ids")
load("//scala/private:macros/scala_repositories.bzl", "scala_repositories")
load("//scala:toolchains_repo.bzl", "scala_toolchains_repo")
load(
"//scala/scalafmt:scalafmt_repositories.bzl",
"scalafmt_artifact_ids",
"scalafmt_default_config",
)
load("//scala:scala_cross_version.bzl", "default_maven_server_urls")
load("//scala:toolchains_repo.bzl", "scala_toolchains_repo")
load("//scalatest:scalatest.bzl", "scalatest_artifact_ids")
load("//specs2:specs2.bzl", "specs2_artifact_ids")
load("//specs2:specs2_junit.bzl", "specs2_junit_artifact_ids")
Expand All @@ -21,7 +26,9 @@ def scala_toolchains(
scalatest = False,
junit = False,
specs2 = False,
testing = False):
testing = False,
scalafmt = False,
scalafmt_default_config_path = ".scalafmt.conf"):
"""Instantiates @io_bazel_rules_scala_toolchains and all its dependencies.
Provides a unified interface to configuring rules_scala both directly in a
Expand Down Expand Up @@ -66,6 +73,9 @@ def scala_toolchains(
specs2: whether to instantiate the Specs2 JUnit toolchain
testing: whether to instantiate the Scalatest, JUnit, and Specs2 JUnit
toolchains combined
scalafmt: whether to instantiate the Scalafmt toolchain
scalafmt_default_config_path: the relative path to the default Scalafmt
config file within the repository
"""
scala_repositories(
maven_servers = maven_servers,
Expand All @@ -78,6 +88,9 @@ def scala_toolchains(
scala_compiler_srcjars = scala_compiler_srcjars,
)

if scalafmt:
scalafmt_default_config(scalafmt_default_config_path)

if testing:
scalatest = True
junit = True
Expand Down Expand Up @@ -106,6 +119,12 @@ def scala_toolchains(
for scala_version in SCALA_VERSIONS:
version_specific_artifact_ids = {}

if scalafmt:
version_specific_artifact_ids.update({
id: fetch_sources
for id in scalafmt_artifact_ids(scala_version)
})

all_artifacts = (
artifact_ids_to_fetch_sources | version_specific_artifact_ids
)
Expand All @@ -125,6 +144,7 @@ def scala_toolchains(
junit = junit,
specs2 = specs2,
testing = testing,
scalafmt = scalafmt,
)

def scala_register_toolchains():
Expand Down
13 changes: 13 additions & 0 deletions scala/toolchains_repo.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ def _scala_toolchains_repo_impl(repository_ctx):
format_args.update(testing_build_args)
toolchains["testing"] = _TESTING_TOOLCHAIN_BUILD

if repo_attr.scalafmt:
toolchains["scalafmt"] = _SCALAFMT_TOOLCHAIN_BUILD

if len(toolchains) == 0:
fail("no toolchains specified")

Expand All @@ -69,6 +72,7 @@ _scala_toolchains_repo = repository_rule(
"junit": attr.bool(),
"specs2": attr.bool(),
"testing": attr.bool(),
"scalafmt": attr.bool(),
},
)

Expand Down Expand Up @@ -143,3 +147,12 @@ load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSIONS")
for scala_version in SCALA_VERSIONS
]
"""

_SCALAFMT_TOOLCHAIN_BUILD = """
load(
"@@{rules_scala_repo}//scala/scalafmt/toolchain:setup_scalafmt_toolchain.bzl",
"setup_scalafmt_toolchains",
)
setup_scalafmt_toolchains()
"""
4 changes: 2 additions & 2 deletions test/shell/test_scala_library.sh
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ test_scala_library_expect_failure_on_missing_direct_internal_deps() {
}

test_scala_library_expect_failure_on_missing_direct_external_deps_jar() {
dependenecy_target='@com_google_guava_guava_21_0//:com_google_guava_guava_21_0'
dependenecy_target='@[a-z_.~+-]*com_google_guava_guava_21_0//:com_google_guava_guava_21_0'
test_target='test_expect_failure/missing_direct_deps/external_deps:transitive_external_dependency_user'

test_scala_library_expect_failure_on_missing_direct_deps $dependenecy_target $test_target
}

test_scala_library_expect_failure_on_missing_direct_external_deps_file_group() {
dependenecy_target='@com_google_guava_guava_21_0_with_file//:com_google_guava_guava_21_0_with_file'
dependenecy_target='@[a-z_.~+-]*com_google_guava_guava_21_0_with_file//:com_google_guava_guava_21_0_with_file'
test_target='test_expect_failure/missing_direct_deps/external_deps:transitive_external_dependency_user_file_group'

test_scala_library_expect_failure_on_missing_direct_deps $dependenecy_target $test_target
Expand Down
2 changes: 1 addition & 1 deletion test/shell/test_strict_dependency.sh
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ test_strict_deps_filter_excluded_target() {

test_strict_deps_filter_included_target() {
local test_target="//test_expect_failure/missing_direct_deps/filtering:b"
local expected_message="buildozer 'add deps @com_google_guava_guava_21_0//:com_google_guava_guava_21_0' ${test_target}"
local expected_message="buildozer 'add deps @[a-z_.~+-]*com_google_guava_guava_21_0//:com_google_guava_guava_21_0' ${test_target}"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message \
"${expected_message}" ${test_target} \
Expand Down
2 changes: 1 addition & 1 deletion test/shell/test_unused_dependency.sh
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ test_unused_deps_filter_excluded_target() {

test_unused_deps_filter_included_target() {
local test_target="//test_expect_failure/unused_dependency_checker/filtering:b"
local expected_message="buildozer 'remove deps @com_google_guava_guava_21_0//:com_google_guava_guava_21_0' ${test_target}"
local expected_message="buildozer 'remove deps @[a-z_.~+-]*com_google_guava_guava_21_0//:com_google_guava_guava_21_0' ${test_target}"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message \
"${expected_message}" ${test_target} \
Expand Down
5 changes: 1 addition & 4 deletions test_cross_build/WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,8 @@ scala_config(
load("@io_bazel_rules_scala//scala:toolchains.bzl", "scala_toolchains")

scala_toolchains(
scalafmt = True,
scalatest = True,
)

register_toolchains("@io_bazel_rules_scala_toolchains//...:all")

load("@io_bazel_rules_scala//scala/scalafmt:scalafmt_repositories.bzl", "scalafmt_repositories")

scalafmt_repositories()
12 changes: 10 additions & 2 deletions test_reproducibility.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,18 @@ non_deploy_jar_md5_sum() {
find bazel-bin/test -name "*.jar" ! -name "*_deploy.jar" ! -path 'bazel-bin/test/jmh/*' | xargs -n 1 -P 5 $(md5_util) | sort
}


test_build_is_identical() {
local test_coverage_packages=()

for package_dir in test/coverage_*; do
test_coverage_packages+=("//${package_dir}/...")
done

bazel clean #ensure we are starting from square one
bazel build test/...
# Also build instrumented jars.
bazel build --collect_code_coverage -- //test/coverage/...
bazel build --collect_code_coverage -- "${test_coverage_packages[@]}"
non_deploy_jar_md5_sum > hash1
bazel clean
sleep 10 # to make sure that if timestamps slip in we get different ones
Expand All @@ -37,7 +44,8 @@ test_build_is_identical() {
fi

bazel build --disk_cache $random_dir test/...
bazel build --disk_cache $random_dir --collect_code_coverage -- //test/coverage/...
bazel build --disk_cache $random_dir --collect_code_coverage -- \
"${test_coverage_packages[@]}"
non_deploy_jar_md5_sum > hash2
diff hash1 hash2
}
Expand Down
4 changes: 2 additions & 2 deletions third_party/dependency_analyzer/src/test/analyzer_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def tests():
],
jvm_flags = common_jvm_flags + [
"-Dguava.jar.location=$(rootpath @com_google_guava_guava_21_0_with_file//jar)",
"-Dapache.commons.jar.location=$(location @org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file)",
"-Dapache.commons.jar.location=$(rootpath @org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file)",
],
unused_dependency_checker_mode = "off",
deps = scala_std_dependencies + [
Expand All @@ -76,7 +76,7 @@ def tests():
"io/bazel/rulesscala/dependencyanalyzer/UnusedDependencyCheckerTest.scala",
],
jvm_flags = common_jvm_flags + [
"-Dapache.commons.jar.location=$(location @org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file)",
"-Dapache.commons.jar.location=$(rootpath @org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file)",
],
unused_dependency_checker_mode = "off",
deps = scala_std_dependencies + [
Expand Down

0 comments on commit 08ab275

Please sign in to comment.