bazelbuild / rules_scala

Scala rules for Bazel
Apache License 2.0
369 stars 285 forks source link

scalafmt compilation breaks after upgrade to "Update to Scalafmt 3.8.3" #1675

Closed gergelyfabian closed 2 months ago

gergelyfabian commented 2 months ago

If I build my example project with rules_scala (in version 190fbee676632eb67be34b5ee91613391fc8e633), I get the following errors:

external/io_bazel_rules_scala/scala/scalafmt/scalafmt/ScalafmtWorker.scala:35: error: Symbol 'type scala.meta.inputs.InputException' is missing from the classpath.
This symbol is required by 'class scala.meta.parsers.ParseException'.
Make sure that type InputException is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'ParseException.class' was compiled against an incompatible version of scala.meta.inputs.
      case e @ (_: org.scalafmt.Error | _: scala.meta.parsers.ParseException) => {
                                           ^
external/io_bazel_rules_scala/scala/scalafmt/scalafmt/ScalafmtWorker.scala:35: warning: fruitless type test: a value of type Throwable cannot also be a scala.meta.parsers.ParseException
      case e @ (_: org.scalafmt.Error | _: scala.meta.parsers.ParseException) => {
                                                              ^
external/io_bazel_rules_scala/scala/scalafmt/scalafmt/ScalafmtAdapter.scala:5: error: object sysops is not a member of package org.scalafmt
import org.scalafmt.sysops.FileOps
                    ^
external/io_bazel_rules_scala/scala/scalafmt/scalafmt/ScalafmtAdapter.scala:9: error: not found: value FileOps
        FileOps.readFile(file.toPath())(codec)
        ^
external/io_bazel_rules_scala/scala/scalafmt/scalafmt/ScalafmtAdapter.scala:5: warning: Unused import
import org.scalafmt.sysops.FileOps

How to reproduce:

git clone https://github.com/gergelyfabian/bazel-scala-example
git checkout upgrade-rules-scala-to-scalafmt-changes
bazel build //...

The version just before 190fbee676632eb67be34b5ee91613391fc8e633 worked ok. Note: in my repo I also upgraded scalafmt to align with the version in rules_scala. Tested the previous version of rules_scala without scalafmt changes.

gergelyfabian commented 2 months ago

Same error for 719f353b85129106a745d9825be2c09231d4fcae.

The latest version (6e36591103889e6cce6e2f71266544b313aeeaab) has a bigger blocker:

ERROR: /home/user/opt/bazel-scala-example/example-lib/BUILD:18:10: While resolving toolchains for target //example-lib:java_test (33bab35): invalid registered toolchain '@io_bazel_rules_scala//testing:scalatest_toolchain_2_13_15': no such target '@@io_bazel_rules_scala//testing:scalatest_toolchain_2_13_15': target 'scalatest_toolchain_2_13_15' not declared in package 'testing' defined by /home/user/.cache/bazel/_bazel_user/64527517dacf7170f7d914889f83481c/external/io_bazel_rules_scala/testing/BUILD
mbland commented 2 months ago

I've reproduced this locally, and I believe this is because I inadvertently broke the default dep_providers of the scala_toolchain rule from //scala/scalafmt/toolchain:toolchain.bzl the example project's scalafmt_toolchain doesn't provide enough dep_providers artifacts.

Still working on a confirmation/fix, but the reason this didn't break the build is because all our own tests end up using setup_scala_toolchain() from //scala/scalafmt/toolchain:setup_scalafmt_toolchain.bzl. This implementation already does the right thing, adding all the required deps_providers. But the example project's //tools/jdk package rolls its own using scala_toolchain directly, and currently only provides three deps in its :scalafmt_classpath_provider:

# Scalafmt                                                                      
declare_deps_provider(                                                          
    name = "scalafmt_classpath_provider",
    deps_id = "scalafmt_classpath",
    visibility = ["//visibility:public"],                                       
    deps = [                                                                    
        "@maven//:com_geirsson_metaconfig_core_%s" % scala_binary_suffix,       
        "@maven//:org_scalameta_parsers_%s" % scala_binary_suffix,              
        "@maven//:org_scalameta_scalafmt_core_%s" % scala_binary_suffix,        
    ],                                                                          
)

This may have been sufficient for Scalafmt 3.0.0, but 3.8.3 added significantly more (#1631).

Will reply further after I've tested my hypothesis, but I have to run for a bit.

gergelyfabian commented 2 months ago

Interesting... I think I have followed a rules_scala documentation when setting up this in the past...

gergelyfabian commented 2 months ago

Nice, I can confirm that this is a fix:

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

 scalafmt_default_config()

-register_toolchains("//tools/jdk:scalafmt_toolchain")
+scalafmt_repositories()

So, I started using the toolchain made available by rules_scala.

gergelyfabian commented 2 months ago

Sorry, I was wrong, when I use the scalafmt_repositories, when I build a "format-test" target, I get:

ERROR: no such package '@@org_scala_lang_scalap_2_12_20//file': BUILD file not found in directory 'file' of external repository @@org_scala_lang_scalap_2_12_20. Add a BUILD file to a directory to mark it as a package.

This is not happening for all targets though...

mbland commented 2 months ago

After the Scalatest stanza replacement, I got bazel build //... to succeed with the following patch:

diff --git a/WORKSPACE b/WORKSPACE
index 1df7df2..106c53a 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -41,11 +41,11 @@ http_archive(
     ],
 )

-RULES_SCALA_VERSION = "190fbee676632eb67be34b5ee91613391fc8e633"
+RULES_SCALA_VERSION = "a8ae50ef8c6f9b4bf551e9d6ccf0b796dd07539d"

 http_archive(
     name = "io_bazel_rules_scala",
-    #integrity = "sha256-SKK6zg+DchyPWe3efuM6p2ly926IzL+IFz4w2oDC6Is=",
+    integrity = "sha256-s+6dL4C28BwS1TSKOe6LgL/rN+wmqzAfVxlSU+tejVE=",
     strip_prefix = "rules_scala-%s" % RULES_SCALA_VERSION,
     #url = "https://github.com/bazelbuild/rules_scala/releases/download/v%s/rules_scala-v%s.tar.gz" % (RULES_SCALA_VERSION, RULES_SCALA_VERSION),
     url = "https://github.com/bazelbuild/rules_scala/archive/{}.zip".format(RULES_SCALA_VERSION),
@@ -56,21 +56,11 @@ load("//tools:scala_version.bzl", "scala_binary_suffix", "scala_binary_version",

 scala_config(scala_version = scala_version)

-load("@io_bazel_rules_scala//scala:scala.bzl", "rules_scala_setup", "rules_scala_toolchain_deps_repositories")
+load("@io_bazel_rules_scala//scala:toolchains.bzl", "scala_toolchains")

-# Loads other rules Rules Scala depends on.
-rules_scala_setup()
-
-rules_scala_toolchain_deps_repositories()
-
-register_toolchains("//tools/jdk:my_scala_toolchain")
-
-# optional: setup ScalaTest toolchain and dependencies
-load("@io_bazel_rules_scala//testing:scalatest.bzl", "scalatest_repositories", "scalatest_toolchain")
-
-scalatest_repositories()
-
-scalatest_toolchain()
+scala_toolchains(
+    testing = True,
+)

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

@@ -182,4 +172,7 @@ load("@maven//:defs.bzl", "pinned_maven_install")

 pinned_maven_install()

-register_toolchains("//tools/jdk:java21_toolchain_definition")
+register_toolchains(
+    "//tools/jdk:java21_toolchain_definition",
+    "@io_bazel_rules_scala_toolchains//...:all",
+)
diff --git a/tools/jdk/BUILD.bazel b/tools/jdk/BUILD.bazel
index 67f12d4..bf7a34d 100644
--- a/tools/jdk/BUILD.bazel
+++ b/tools/jdk/BUILD.bazel
@@ -85,9 +85,14 @@ declare_deps_provider(
     deps_id = "scalafmt_classpath",
     visibility = ["//visibility:public"],
     deps = [
-        "@maven//:com_geirsson_metaconfig_core_%s" % scala_binary_suffix,
-        "@maven//:org_scalameta_parsers_%s" % scala_binary_suffix,
-        "@maven//:org_scalameta_scalafmt_core_%s" % scala_binary_suffix,
+        "@maven//:%s_%s" % (dep, scala_binary_suffix)
+        for dep in [
+            "com_geirsson_metaconfig_core",
+            "org_scalameta_parsers",
+            "org_scalameta_scalafmt_core",
+            "org_scalameta_scalafmt_sysops",
+            "org_scalameta_trees",
+        ]
     ],
 )

This seems to be the minimal set of packages required for this particular target. The missing InputException symbol from earlier comes from org_scalameta_trees; removing this dep reproduces the error.

I figured this out by looking at _SCALAFMT_DEPS and _SCALAFMT_DEPS_2_12 from //scala/scalafmt/scalafmt_repositories.bzl in rules_scala and mapping different deps to the rules_jvm_external schema of the example project.

I'm not sure how best to add a test for this, or how to document it for folks wanting to roll their own Scalafmt toolchain. I'll think on it, but I'm open to suggestions.

gergelyfabian commented 2 months ago

Thank you! Upgraded my example repo's master branch to latest rules_scala, added most of the above fixes, and it works. Left my own scala toolchain for now.

gergelyfabian commented 2 months ago

My changes were:

diff --git a/WORKSPACE b/WORKSPACE
index d5e4f10..b08029f 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -41,13 +41,14 @@ http_archive(
     ],
 )

-RULES_SCALA_VERSION = "6.6.0"
+RULES_SCALA_VERSION = "a8ae50ef8c6f9b4bf551e9d6ccf0b796dd07539d"

 http_archive(
     name = "io_bazel_rules_scala",
-    integrity = "sha256-5zTu+VzybAFxVmvcJNg72Cva+Mp4c77GzpsNUkva8F0=",
+    integrity = "sha256-s+6dL4C28BwS1TSKOe6LgL/rN+wmqzAfVxlSU+tejVE=",
     strip_prefix = "rules_scala-%s" % RULES_SCALA_VERSION,
-    url = "https://github.com/bazelbuild/rules_scala/releases/download/v%s/rules_scala-v%s.tar.gz" % (RULES_SCALA_VERSION, RULES_SCALA_VERSION),
+    #url = "https://github.com/bazelbuild/rules_scala/releases/download/v%s/rules_scala-v%s.tar.gz" % (RULES_SCALA_VERSION, RULES_SCALA_VERSION),
+    url = "https://github.com/bazelbuild/rules_scala/archive/{}.zip".format(RULES_SCALA_VERSION),
 )

 load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config")
@@ -55,22 +56,15 @@ load("//tools:scala_version.bzl", "scala_binary_suffix", "scala_binary_version",

 scala_config(scala_version = scala_version)

-load("@io_bazel_rules_scala//scala:scala.bzl", "rules_scala_setup", "rules_scala_toolchain_deps_repositories")
+load("@io_bazel_rules_scala//scala:toolchains.bzl", "scala_toolchains")

-# Loads other rules Rules Scala depends on.
-rules_scala_setup()
-
-rules_scala_toolchain_deps_repositories()
+scala_toolchains(
+    fetch_sources = True,
+    testing = True,
+)

 register_toolchains("//tools/jdk:my_scala_toolchain")

-# optional: setup ScalaTest toolchain and dependencies
-load("@io_bazel_rules_scala//testing:scalatest.bzl", "scalatest_repositories", "scalatest_toolchain")
-
-scalatest_repositories()
-
-scalatest_toolchain()
-
 load("@io_bazel_rules_scala//scala/scalafmt:scalafmt_repositories.bzl", "scalafmt_default_config", "scalafmt_repositories")

 scalafmt_default_config()
@@ -165,7 +159,7 @@ maven_install(
         "org.scala-lang:scala-library:jar:%s" % scala_version,
         "org.scala-lang:scala-reflect:jar:%s" % scala_version,
         "org.scala-lang:scala-compiler:jar:%s" % scala_version,
-        "org.scalameta:scalafmt-core_%s:3.0.0" % scala_binary_version,
+        "org.scalameta:scalafmt-core_%s:3.8.3" % scala_binary_version,
     ],
     # Some useful options that you may want to try:
     fetch_sources = True,
@@ -181,4 +175,7 @@ load("@maven//:defs.bzl", "pinned_maven_install")

 pinned_maven_install()

-register_toolchains("//tools/jdk:java21_toolchain_definition")
+register_toolchains(
+    "//tools/jdk:java21_toolchain_definition",
+    "@io_bazel_rules_scala_toolchains//...:all",
+)
diff --git a/tools/jdk/BUILD.bazel b/tools/jdk/BUILD.bazel
index 67f12d4..bf7a34d 100644
--- a/tools/jdk/BUILD.bazel
+++ b/tools/jdk/BUILD.bazel
@@ -85,9 +85,14 @@ declare_deps_provider(
     deps_id = "scalafmt_classpath",
     visibility = ["//visibility:public"],
     deps = [
-        "@maven//:com_geirsson_metaconfig_core_%s" % scala_binary_suffix,
-        "@maven//:org_scalameta_parsers_%s" % scala_binary_suffix,
-        "@maven//:org_scalameta_scalafmt_core_%s" % scala_binary_suffix,
+        "@maven//:%s_%s" % (dep, scala_binary_suffix)
+        for dep in [
+            "com_geirsson_metaconfig_core",
+            "org_scalameta_parsers",
+            "org_scalameta_scalafmt_core",
+            "org_scalameta_scalafmt_sysops",
+            "org_scalameta_trees",
+        ]
     ],
 )

The rest is more-or-less obvious, like upgrading dependencies (for Scalafmt).