From e942d9427d95fbe5a0125fb4c09bd426acfba19d Mon Sep 17 00:00:00 2001
From: Dave Nicponski <dave.nicponski@gmail.com>
Date: Tue, 8 Feb 2022 13:51:24 -0500
Subject: [PATCH] Tweaks to `scalafmt` rules/script to support `bazel test`
 wrappers (#1344)

With these changes, I can use a macro to place a wrapping `sh_test`
target around this script, and change the phase ordering to put
`phase_scalafmt` before the `runfiles` phase, and suddenly it works
as intended!

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for `ext_scalafmt` to place
it before `runfiles` by default.

Example use:

In custom bzl file, I have:

```
_enable_format_targets_and_tests_by_default = True

_ext_scalafmt = dicts.add(
    _base_ext_scalafmt,
    {"attrs": dicts.add(
        _base_ext_scalafmt["attrs"],
        {"format": attr.bool(
            default = _enable_format_targets_and_tests_by_default,
            doc = "Enable the check-format and auto-format synthetic run targets (foo.format-test and foo.format respectively)",
        )},
    )},
    {"phase_providers": [
        "@//path/to/some/buildfile:phase_scalafmt",
    ]},
)

scala_library = make_scala_library(_ext_scalafmt)  # etc for other rule types

def maybe_add_format_test(**kwargs):
    if (kwargs.get("format", _enable_format_targets_and_tests_by_default)):
        name = kwargs["name"]
        sh_test(
            name = "{}.format-test.target".format(name),
            srcs = [":{}.format-test".format(name)],
            data = [
                ":{}".format(name),
            ],
            local = True,
        )

def _scalafmt_singleton_implementation(ctx):
    return [
        _ScalaRulePhase(
            custom_phases = [
                ("-", "runfiles", "scalafmt", _phase_scalafmt),
            ],
        ),
    ]

scalafmt_singleton = rule(
    implementation = _scalafmt_singleton_implementation,
)
```

then in `path/to/some/buildfile/BUILD.bazel`, I have:

```
load("//path/to/above/my_rules.bzl", "scalafmt_singleton")

scalafmt_singleton(
    name = "phase_scalafmt",
    visibility = ["//visibility:public"],
)
```
---
 scala/private/phases/phase_scalafmt.bzl        | 10 ++++++++--
 scala/scalafmt/private/format-test.template.sh | 15 ++++++++++++---
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/scala/private/phases/phase_scalafmt.bzl b/scala/private/phases/phase_scalafmt.bzl
index a42410980..b6115774f 100644
--- a/scala/private/phases/phase_scalafmt.bzl
+++ b/scala/private/phases/phase_scalafmt.bzl
@@ -10,19 +10,25 @@ load(
 
 def phase_scalafmt(ctx, p):
     if ctx.attr.format:
-        manifest, files = _build_format(ctx)
+        manifest, files, srcs = _build_format(ctx)
         _formatter(ctx, manifest, files, ctx.file._runner, ctx.outputs.scalafmt_runner)
         _formatter(ctx, manifest, files, ctx.file._testrunner, ctx.outputs.scalafmt_testrunner)
+
+        # Return a depset containing all the relevant files, so a wrapping `sh_test` can successfully access them.
+        return struct(runfiles = depset([manifest] + files + srcs))
     else:
         _write_empty_content(ctx, ctx.outputs.scalafmt_runner)
         _write_empty_content(ctx, ctx.outputs.scalafmt_testrunner)
+        return None
 
 def _build_format(ctx):
     files = []
+    srcs = []
     manifest_content = []
     for src in ctx.files.srcs:
         # only format scala source files, not generated files
         if src.path.endswith(_scala_extension) and src.is_source:
+            srcs.append(src)
             file = ctx.actions.declare_file("{}.fmt.output".format(src.short_path))
             files.append(file)
             ctx.actions.run(
@@ -40,7 +46,7 @@ def _build_format(ctx):
     manifest = ctx.actions.declare_file("format/{}/manifest.txt".format(ctx.label.name))
     ctx.actions.write(manifest, "\n".join(manifest_content) + "\n")
 
-    return manifest, files
+    return manifest, files, srcs
 
 def _formatter(ctx, manifest, files, template, output_runner):
     ctx.actions.run_shell(
diff --git a/scala/scalafmt/private/format-test.template.sh b/scala/scalafmt/private/format-test.template.sh
index 0ca9d99b9..0648ff52d 100644
--- a/scala/scalafmt/private/format-test.template.sh
+++ b/scala/scalafmt/private/format-test.template.sh
@@ -1,15 +1,24 @@
 #!/bin/bash -e
-WORKSPACE_ROOT="${1:-$BUILD_WORKSPACE_DIRECTORY}"
+
+# Explanation: IF $BUILD_WORKSPACE_DIRECTORY is set to something (as it would be during a
+# `bazel run`), then append a trailing `/`.  If it's not set (as it wouldn't be during
+# a `bazel test` invocation in a wrapping `sh_test` rule), then elide the trailing `/`, and
+# instead rely upon a relative path from the test's runtrees.  The corresponding change
+# to `phase_scalafmt` places the source files into the `runfiles` set, so they'll be symlinked
+# correctly in the appropriate relative location.
+WORKSPACE_ROOT="${1:-${BUILD_WORKSPACE_DIRECTORY}${BUILD_WORKSPACE_DIRECTORY:+/}}"
+
 RUNPATH="${TEST_SRCDIR-$0.runfiles}"/%workspace%
 RUNPATH=(${RUNPATH//bin/ })
 RUNPATH="${RUNPATH[0]}"bin
 
 EXIT=0
+
 while read original formatted; do
     if [[ ! -z "$original" ]] && [[ ! -z "$formatted" ]]; then
-        if ! cmp -s "$WORKSPACE_ROOT/$original" "$RUNPATH/$formatted"; then
+        if ! cmp -s "${WORKSPACE_ROOT}$original" "$RUNPATH/$formatted"; then
             echo $original
-            diff "$WORKSPACE_ROOT/$original" "$RUNPATH/$formatted" || true
+            diff "${WORKSPACE_ROOT}$original" "$RUNPATH/$formatted" || true
             EXIT=1
         fi
     fi