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

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

Closed
gergelyfabian opened this issue Jan 14, 2025 · 8 comments
Closed

Comments

@gergelyfabian
Copy link
Contributor

If I build my example project with rules_scala (in version 190fbee), 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 190fbee 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
Copy link
Contributor Author

Same error for 719f353.

The latest version (6e36591) 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
Copy link
Contributor

mbland commented Jan 14, 2025

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
Copy link
Contributor Author

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

@gergelyfabian
Copy link
Contributor Author

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
Copy link
Contributor Author

gergelyfabian commented Jan 14, 2025

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

@gergelyfabian gergelyfabian reopened this Jan 14, 2025
@mbland
Copy link
Contributor

mbland commented Jan 14, 2025

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.

mbland added a commit to mbland/rules_scala that referenced this issue Jan 15, 2025
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).
@gergelyfabian
Copy link
Contributor Author

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
Copy link
Contributor Author

gergelyfabian commented Jan 15, 2025

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

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

No branches or pull requests

2 participants