Skip to content

Fix last_green build after disable_autoloads flip #1738

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

Conversation

mbland
Copy link
Contributor

@mbland mbland commented May 17, 2025

Description

Explicitly loads all symbols affected by --incompatible_disable_autoloads_in_main_repo to fix last_green Bazel builds. Adds rules_shell as a dev dependency and updates files to fix test_bzlmod_macros.sh and test_dependency_versions.sh.

As a result of using Buildifier to add the missing load statements, Buildifier reordered the load statements as well.

Fixes the following breakage when using the last_green build from bazelbuild/bazel@f08d561 (and other breakages blocked by this one):

$ bazel run //tools:lint_check

ERROR: scala/private/rules/scala_test.bzl:130:21:
  name 'JavaInfo' is not defined

ERROR: .../external/+dev_deps+com_github_bazelbuild_buildtools/buildifier/BUILD.bazel:3:10:
  While resolving toolchains for target
  @@+dev_deps+com_github_bazelbuild_buildtools//buildifier:buildifier
  (b5bbebe): invalid registered toolchain
  '//test/proto:scalapb_toolchain':
  error loading package 'test/proto':
  at /Users/mbland/src/bazel-contrib/rules_scala/scala/scala.bzl:35:5:
  compilation of module 'scala/private/rules/scala_test.bzl' failed

ERROR: Analysis of target '//tools:lint_check' failed; build aborted:
  Analysis failed

Fixed using Buildifier 8.2.0 by first running the following to get the list of appropriate warnings:

buildifier --lint=warn -r . 2>&1 | grep -- native- |
  awk '{print $2}' | sort | uniq

then running:

buildifier --lint=fix \
  --warnings=native-java-common,native-java-info,native-sh-test,native-sh-binary \
  -r .

I also ran it separately on the changed files in deps/test and scala/private/macros/test, since Buildifier didn't automatically find them.

Motivation

bazelbuild/bazel@d87eaf5 from 2025-05-08 at 8:03am EDT flipped
--incompatible_disable_autoloads_in_main_repo to true to prepare for Bazel 9. I did confirm that setting
--noincompatible_disable_autoloads_in_main_repo resolved the breakage before using Buildifier to add the necessary load statements.

Pull request #1735 was the last to pass the last_green CI build using bazelbuild/bazel@94e0b44 from 2025-05-08 at 7:39am EDT. This was two commits before the commit that flipped --incompatible_disable_autoloads_in_main_repo:

Pull request #1736 failed the last_green CI build using bazelbuild/bazel@cac54c4 from 2025-05-09 at 6:14am EDT. That job ran on 2025-05-09 at 6:47am EDT. The last_green configuration in .bazelci/presubmit.yml emits a warning if the build fails. This warning was present on the build page, but we've overlooked it until now:

Part of this change copies more files from test/jmh into the test_dependency_versions.sh test directory. It also replicates targets from //test/jmh in deps/test/BUILD.bazel.test to replace the previous dependency on @rules_scala//test/jmh:add_numbers.

This is because //test/jmh now has a load dependency on @rules_shell, which is a dev_dependency of @rules_scala. Since @rules_shell isn't visible to @rules_scala when it isn't the root module, this broke the build within test_dependency_versions.sh.

Explicitly loads all symbols affected by
`--incompatible_disable_autoloads_in_main_repo` to fix `last_green`
Bazel builds. Adds `rules_shell` as a dev dependency and updates files
to fix `test_bzlmod_macros.sh` and `test_dependency_versions.sh`.

As a result of using Buildifier to add the missing `load` statements,
Buildifier reordered the `load` statements as well.

Fixes the following breakage when using the `last_green` build from
bazelbuild/bazel@f08d561 (and other
breakages blocked by this one):

```sh
$ bazel run //tools:lint_check

ERROR: scala/private/rules/scala_test.bzl:130:21:
  name 'JavaInfo' is not defined

ERROR: .../external/+dev_deps+com_github_bazelbuild_buildtools/buildifier/BUILD.bazel:3:10:
  While resolving toolchains for target
  @@+dev_deps+com_github_bazelbuild_buildtools//buildifier:buildifier
  (b5bbebe): invalid registered toolchain
  '//test/proto:scalapb_toolchain':
  error loading package 'test/proto':
  at /Users/mbland/src/bazel-contrib/rules_scala/scala/scala.bzl:35:5:
  compilation of module 'scala/private/rules/scala_test.bzl' failed

ERROR: Analysis of target '//tools:lint_check' failed; build aborted:
  Analysis failed
```

Fixed using Buildifier 8.2.0 by first running the following to get the
list of appropriate warnings:

```txt
buildifier --lint=warn -r . 2>&1 | grep -- native- |
  awk '{print $2}' | sort | uniq
```

then running:

```txt
buildifier --lint=fix \
  --warnings=native-java-common,native-java-info,native-sh-test,native-sh-binary \
  -r .
```

I also ran it separately on the changed files in `deps/test` and
`scala/private/macros/test`, since Buildifier didn't automatically find
them.

---

bazelbuild/bazel@d87eaf5 from
2025-05-08 at 8:03am EDT flipped
`--incompatible_disable_autoloads_in_main_repo` to `true` to prepare for
Bazel 9. I did confirm that setting
`--noincompatible_disable_autoloads_in_main_repo` resolved the breakage
before using Buildifier to add the necessary `load` statements.

Pull request bazel-contrib#1735 was the last to pass the `last_green` CI build using
bazelbuild/bazel@94e0b44 from
2025-05-08 at 7:39am EDT. This was two commits before the commit that
flipped `--incompatible_disable_autoloads_in_main_repo`:

- https://buildkite.com/bazel/rules-scala-scala/builds/5596#0196afc2-4af3-4202-8232-d5f5fe113349

Pull request bazel-contrib#1736 failed the `last_green` CI build using
bazelbuild/bazel@cac54c4 from
2025-05-09 at 6:14am EDT. That job ran on 2025-05-09 at 6:47am EDT. The
`last_green` configuration in `.bazelci/presubmit.yml` emits a warning
if the build fails. This warning was present on the build page, but
we've overlooked it until now:

- https://buildkite.com/bazel/rules-scala-scala/builds/5598#0196b4a8-e593-431a-ab4c-8d8471d9b08b

Part of this change copies more files from `test/jmh` into the
`test_dependency_versions.sh` test directory. It also replicates targets
from `//test/jmh` in `deps/test/BUILD.bazel.test` to replace
the previous dependency on `@rules_scala//test/jmh:add_numbers`.

This is because `//test/jmh` now has a `load` dependency on
`@rules_shell`, which is a `dev_dependency` of `@rules_scala`. Since
`@rules_shell` isn't visible to `@rules_scala` when it isn't the root
module, this broke the build within `test_dependency_versions.sh`.
@mbland mbland requested review from liucijus and simuons as code owners May 17, 2025 20:03
The following build experienced this seemingly transient error in
`./test_coverage on macOS` (all other jobs in the same build passed):

- https://buildkite.com/bazel/rules-scala-scala/builds/5608#0196dfd9-3da9-4fbd-b284-e63e849f9d0a/89-128

```txt
WARNING: Download from
https://github.com/protocolbuffers/protobuf/releases/download/v30.2/protoc-30.2-osx-x86_64.zip
failed: class java.io.IOException GET returned 500 Internal Server Error
```
mbland added a commit to EngFlow/example that referenced this pull request May 17, 2025
Explicitly loads all symbols affected by
`--incompatible_disable_autoloads_in_main_repo` to fix `last_green`
Bazel builds. Adds the flag to `.bazelrc`, adds `rules_shell` as a
dependency, and contains lots of other Buildifier formatting touch ups.

bazelbuild/bazel@d87eaf5 from
2025-05-08 at 8:03am EDT flipped
`--incompatible_disable_autoloads_in_main_repo` to `true` to prepare for
Bazel 9. See also bazel-contrib/rules_scala#1738.

Before this change, setting the flag caused these errors:

```txt
$ bazel build --incompatible_disable_autoloads_in_main_repo //...

ERROR: java/com/engflow/notificationqueue/demoserver/BUILD:5:1:
  name 'proto_library' is not defined
ERROR: /demoserver/BUILD:10:1:
  name 'java_proto_library' is not defined (did you mean 'java_grpc_library'?)
ERROR: docker/network/BUILD:19:10: //docker/network:docker-network-test:
  no such attribute 'srcs' in 'java_test' rule
ERROR: docker/network/BUILD:19:10: //docker/network:docker-network-test:
  no such attribute 'test_class' in 'java_test' rule
ERROR: docker/sandbox/BUILD:15:12: //docker/sandbox:hello-java:
  no such attribute 'srcs' in 'java_binary' rule
ERROR: docker/sandbox/BUILD:15:12: //docker/sandbox:hello-java:
  no such attribute 'main_class' in 'java_binary' rule
ERROR: docker/sysbox/dind_test/BUILD:15:1:
  name 'sh_test' is not defined (did you mean 'cc_test'?)
ERROR: package contains errors: docker/sysbox/dind_test
ERROR: python/pytest/BUILD:3:1:
  name 'py_library' is not defined (did you mean 'cc_library'?)
ERROR: package contains errors: docker/sandbox
ERROR: package contains errors: java/com/engflow/notificationqueue/demoserver
ERROR: docker/network/BUILD:39:13: //docker/network:jdk:
  no such attribute 'java_home' in 'java_runtime' rule
ERROR: package contains errors: docker/network
ERROR: package contains errors: python/pytest
ERROR: java/com/engflow/example/BUILD:3:13: //java/com/engflow/example:example:
  no such attribute 'srcs' in 'java_library' rule
ERROR: java/com/engflow/example/BUILD:8:10: //java/com/engflow/example:ExampleTest:
  no such attribute 'srcs' in 'java_test' rule
ERROR: java/com/engflow/example/BUILD:8:10: //java/com/engflow/example:ExampleTest:
  no such attribute 'deps' in 'java_test' rule
ERROR: package contains errors: java/com/engflow/example
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'srcs' in 'java_binary' rule
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'main_class' in 'java_binary' rule
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'deps' in 'java_binary' rule
ERROR: package contains errors: java/com/engflow/notificationqueue
WARNING: Target pattern parsing failed.
ERROR: Skipping '//...': Error evaluating '//...':
  error loading package 'docker/network':
  Package 'docker/network' contains errors
ERROR: Error evaluating '//...':
  error loading package 'docker/network':
  Package 'docker/network' contains errors
INFO: Elapsed time: 2.821s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
```

Applied the fixes using Buildifier 8.2.0 with the following script:

```bash

WARNINGS=(
  $(buildifier --lint=warn -r . 2>&1 | grep -- native- |
    awk '{print $2}' | sort | uniq | sed -e 's/:$//')
)

IFS=, WARNINGS="${WARNINGS[*]}"
echo "warnings: ${WARNINGS}"
buildifier --lint=fix --warnings="$WARNINGS" -r .
```
@mbland
Copy link
Contributor Author

mbland commented May 17, 2025

@simuons I've also got another change teed up to land after this PR that contains some version bumps and small documentation fixes. Maybe after this pull request and that, it could be time for v7.0.1?

mbland added a commit to EngFlow/example that referenced this pull request May 20, 2025
Explicitly loads all symbols affected by
`--incompatible_disable_autoloads_in_main_repo` to fix `last_green`
Bazel builds. Adds the flag to `.bazelrc`, adds `rules_shell` as a
dependency, and contains lots of other Buildifier formatting touch ups.

bazelbuild/bazel@d87eaf5 from
2025-05-08 at 8:03am EDT flipped
`--incompatible_disable_autoloads_in_main_repo` to `true` to prepare for
Bazel 9. See also bazel-contrib/rules_scala#1738.

Before this change, setting the flag caused these errors:

```txt
$ bazel build --incompatible_disable_autoloads_in_main_repo //...

ERROR: java/com/engflow/notificationqueue/demoserver/BUILD:5:1:
  name 'proto_library' is not defined
ERROR: /demoserver/BUILD:10:1:
  name 'java_proto_library' is not defined (did you mean 'java_grpc_library'?)
ERROR: docker/network/BUILD:19:10: //docker/network:docker-network-test:
  no such attribute 'srcs' in 'java_test' rule
ERROR: docker/network/BUILD:19:10: //docker/network:docker-network-test:
  no such attribute 'test_class' in 'java_test' rule
ERROR: docker/sandbox/BUILD:15:12: //docker/sandbox:hello-java:
  no such attribute 'srcs' in 'java_binary' rule
ERROR: docker/sandbox/BUILD:15:12: //docker/sandbox:hello-java:
  no such attribute 'main_class' in 'java_binary' rule
ERROR: docker/sysbox/dind_test/BUILD:15:1:
  name 'sh_test' is not defined (did you mean 'cc_test'?)
ERROR: package contains errors: docker/sysbox/dind_test
ERROR: python/pytest/BUILD:3:1:
  name 'py_library' is not defined (did you mean 'cc_library'?)
ERROR: package contains errors: docker/sandbox
ERROR: package contains errors: java/com/engflow/notificationqueue/demoserver
ERROR: docker/network/BUILD:39:13: //docker/network:jdk:
  no such attribute 'java_home' in 'java_runtime' rule
ERROR: package contains errors: docker/network
ERROR: package contains errors: python/pytest
ERROR: java/com/engflow/example/BUILD:3:13: //java/com/engflow/example:example:
  no such attribute 'srcs' in 'java_library' rule
ERROR: java/com/engflow/example/BUILD:8:10: //java/com/engflow/example:ExampleTest:
  no such attribute 'srcs' in 'java_test' rule
ERROR: java/com/engflow/example/BUILD:8:10: //java/com/engflow/example:ExampleTest:
  no such attribute 'deps' in 'java_test' rule
ERROR: package contains errors: java/com/engflow/example
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'srcs' in 'java_binary' rule
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'main_class' in 'java_binary' rule
ERROR: java/com/engflow/notificationqueue/BUILD:5:12: //java/com/engflow/notificationqueue:client:
  no such attribute 'deps' in 'java_binary' rule
ERROR: package contains errors: java/com/engflow/notificationqueue
WARNING: Target pattern parsing failed.
ERROR: Skipping '//...': Error evaluating '//...':
  error loading package 'docker/network':
  Package 'docker/network' contains errors
ERROR: Error evaluating '//...':
  error loading package 'docker/network':
  Package 'docker/network' contains errors
INFO: Elapsed time: 2.821s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
```

Applied the fixes using Buildifier 8.2.0 with the following script:

```bash

WARNINGS=(
  $(buildifier --lint=warn -r . 2>&1 | grep -- native- |
    awk '{print $2}' | sort | uniq | sed -e 's/:$//')
)

IFS=, WARNINGS="${WARNINGS[*]}"
echo "warnings: ${WARNINGS}"
buildifier --lint=fix --warnings="$WARNINGS" -r .
```
Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @mbland!

Maybe after this pull request and that, it could be time for v7.0.1?

Sure thing

@simuons simuons merged commit fb08327 into bazel-contrib:master May 23, 2025
1 check passed
@mbland mbland deleted the fix-last-green-build-with-disabled-autoload branch May 26, 2025 17:28
mbland added a commit to mbland/rules_scala that referenced this pull request Aug 3, 2025
Removes almost all of the copied `deps/test` sources and targets in
favor of invoking `@rules_scala` and `@multi_frameworks_toolchain` tests
directly. Moves targets depending on `@rules_python` and `@rules_shell`
to new packages so test packages don't break when `@rules_scala` isn't
the main module.

Introduces `RULES_SCALA_TARGETS`, `MULTI_FRAMEWORKS_TOOLCHAIN_TARGETS`,
and `ALL_TARGETS` arrays to easily specify compatibility test targets
instead of copying them. Only keeps a single `ScalafmtTest` target
within `deps/test/BUILD.bazel.test`, which is a special case (described
below) and executes successfully on Windows.

Though these are new packages, they're comprised of existing files and
targets from their parent packages:

- //test/jmh/runtime
- //test/sh_tests
- //test/src/main/scala/scalarules/test/twitter_scrooge/strings

Also:

- Marks every `bazel_dep` for `@latest_dependencies` with
  `dev_dependency = True`.

- Replaces `$(location ...)` instances in moved targets with
  `$(rootpath ...)` (and in one case, with `$(execpath ...)`) per
  bazelbuild/bazel#25198.

---

While drafting a blog post describing `test_dependency_versions.sh`, I
revisited the decision to copy targets and files in bazel-contrib#1729 and bazel-contrib#1738.
Executing test targets from `@rules_scala` directly from the test module
actually works, making the test far less complex, and making for better
testing advice.

This required moving targets that referenced dev dependencies to their
own packages, fixing `@rules_scala` test package breakages. Making the
`@latest_dependencies` module a dev dependency eliminated module version
warnings from `@multi_frameworks_toolchain`, since the test sets older
dependency versions. Then making `@latest_dependencies` a dev dependency
everywhere, instead of only in `@multi_frameworks_toolchain`, seemed
more consistent.

The test retains the `//:ScalafmtTest` target because `rules_scala`
doesn't actually provide Scalafmt rules, but only provides
`ext_scalafmt` to create custom rules, per:

- docs/phase_scalafmt.md
- docs/customizable_phase.md

I discovered this after going down a rabbit hole regarding the
implicitly defined (and ostensibly deprecated) `.format` and
`.format-test` predeclared outputs for Scalafmt rules. Long story short,
implicitly defined predeclared outputs have been "deprecated" since
2018-03-05, predating Bazel 0.28.0:

- https://bazel.build/versions/8.3.0/extending/rules#requesting_output_files
- https://bazel.build/versions/8.3.0/extending/rules#deprecated_predeclared_outputs
- https://bazel.build/versions/8.3.0/rules/lib/globals/bzl#rule.outputs
- 2018-03-05: Output Map Madness
  https://docs.google.com/document/d/1ic9lJPn-0VqgKcqSbclVWwYDW2eiV-9k6ZUK_xE6H5E/edit
- 2019-04-08: bazelbuild/bazel#7977: incompatible_no_rule_outputs_param:
  disable outputs param of rule function
  bazelbuild/bazel#7977
- 2019-06-07: Rollforward "Disable outputs param of rule function" with
  fix (introduced --incompatible_no_rule_outputs_param in Bazel 0.28.0)
  bazelbuild/bazel@36c70a6
- 2019-09-04: Document the replacements for the `outputs` parameter to
  the `rule()` function. (Bazel 1.0.0)
  bazelbuild/bazel@e29ddda

However, I learned that the `.format` and `.format-test` targets are
Bash scripts run _after_ the original rule executes Scalafmt. The
generated scripts don't depend on the `//scala/scalafmt:scalafmt` binary
at all. Plus, they only work when invoked within the main module, since
they reference relative paths within `BUILD_WORKSPACE_DIRECTORY`. And
finally, `bazel test @rules_scala//test/scalafmt:formatted-test` fails
because it tries to declare output files in a nonexistent directory.

```txt
ERROR: .../external/rules_scala~/test/scalafmt/BUILD:43:20:
  in scalafmt_scala_test rule
  @@rules_scala~//test/scalafmt:formatted-test:

Traceback (most recent call last):
  File ".../external/rules_scala~/scala/private/rules/scala_test.bzl",
    line 38, column 22, in _scala_test_impl
      return run_phases(
  File ".../external/rules_scala~/scala/private/phases/api.bzl",
    line 45, column 23, in run_phases
      return _run_phases(ctx, builtin_customizable_phases, target = None)
  File ".../external/rules_scala~/scala/private/phases/api.bzl",
    line 77, column 32, in _run_phases
      new_provider = function(ctx, current_provider)
  File ".../external/rules_scala~/scala/private/phases/phase_scalafmt.bzl",
    line 10, column 46, in phase_scalafmt
      manifest, files, srcs = _build_format(ctx)
  File ".../external/rules_scala~/scala/private/phases/phase_scalafmt.bzl",
    line 30, column 44, in _build_format
      file = ctx.actions.declare_file("{}.fmt.output".format(src.short_path))

Error in declare_file:
  the output artifact
  'external/rules_scala~/test/rules_scala~/test/scalafmt/formatted/formatted-test.scala.fmt.output'
  is not under package directory
  'external/rules_scala~/test/scalafmt'
  for target '@@rules_scala~//test/scalafmt:formatted-test'

ERROR: .../external/rules_scala~/test/scalafmt/BUILD:43:20:
  Analysis of target '@@rules_scala~//test/scalafmt:formatted-test'
  failed
```

So invoking `bazel test //:ScalafmtTest` (via `/...` from `ALL_TARGETS`)
is sufficient, since we're only validating that the toolchains execute
properly. (`test_scalafmt.sh` tests the behavior of the `.format` and
`.format-test` scripts, _including_ on Windows via `--run_under=bash`.)
This also means we can invoke this target on Windows, instead of having
special case logic to avoid invoking the previous `bazel run` command.
simuons pushed a commit that referenced this pull request Aug 5, 2025
Removes almost all of the copied `deps/test` sources and targets in
favor of invoking `@rules_scala` and `@multi_frameworks_toolchain` tests
directly. Moves targets depending on `@rules_python` and `@rules_shell`
to new packages so test packages don't break when `@rules_scala` isn't
the main module.

Introduces `RULES_SCALA_TARGETS`, `MULTI_FRAMEWORKS_TOOLCHAIN_TARGETS`,
and `ALL_TARGETS` arrays to easily specify compatibility test targets
instead of copying them. Only keeps a single `ScalafmtTest` target
within `deps/test/BUILD.bazel.test`, which is a special case (described
below) and executes successfully on Windows.

Though these are new packages, they're comprised of existing files and
targets from their parent packages:

- //test/jmh/runtime
- //test/sh_tests
- //test/src/main/scala/scalarules/test/twitter_scrooge/strings

Also:

- Marks every `bazel_dep` for `@latest_dependencies` with
  `dev_dependency = True`.

- Replaces `$(location ...)` instances in moved targets with
  `$(rootpath ...)` (and in one case, with `$(execpath ...)`) per
  bazelbuild/bazel#25198.

---

While drafting a blog post describing `test_dependency_versions.sh`, I
revisited the decision to copy targets and files in #1729 and #1738.
Executing test targets from `@rules_scala` directly from the test module
actually works, making the test far less complex, and making for better
testing advice.

This required moving targets that referenced dev dependencies to their
own packages, fixing `@rules_scala` test package breakages. Making the
`@latest_dependencies` module a dev dependency eliminated module version
warnings from `@multi_frameworks_toolchain`, since the test sets older
dependency versions. Then making `@latest_dependencies` a dev dependency
everywhere, instead of only in `@multi_frameworks_toolchain`, seemed
more consistent.

The test retains the `//:ScalafmtTest` target because `rules_scala`
doesn't actually provide Scalafmt rules, but only provides
`ext_scalafmt` to create custom rules, per:

- docs/phase_scalafmt.md
- docs/customizable_phase.md

I discovered this after going down a rabbit hole regarding the
implicitly defined (and ostensibly deprecated) `.format` and
`.format-test` predeclared outputs for Scalafmt rules. Long story short,
implicitly defined predeclared outputs have been "deprecated" since
2018-03-05, predating Bazel 0.28.0:

- https://bazel.build/versions/8.3.0/extending/rules#requesting_output_files
- https://bazel.build/versions/8.3.0/extending/rules#deprecated_predeclared_outputs
- https://bazel.build/versions/8.3.0/rules/lib/globals/bzl#rule.outputs
- 2018-03-05: Output Map Madness
  https://docs.google.com/document/d/1ic9lJPn-0VqgKcqSbclVWwYDW2eiV-9k6ZUK_xE6H5E/edit
- 2019-04-08: bazelbuild/bazel#7977: incompatible_no_rule_outputs_param:
  disable outputs param of rule function
  bazelbuild/bazel#7977
- 2019-06-07: Rollforward "Disable outputs param of rule function" with
  fix (introduced --incompatible_no_rule_outputs_param in Bazel 0.28.0)
  bazelbuild/bazel@36c70a6
- 2019-09-04: Document the replacements for the `outputs` parameter to
  the `rule()` function. (Bazel 1.0.0)
  bazelbuild/bazel@e29ddda

However, I learned that the `.format` and `.format-test` targets are
Bash scripts run _after_ the original rule executes Scalafmt. The
generated scripts don't depend on the `//scala/scalafmt:scalafmt` binary
at all. Plus, they only work when invoked within the main module, since
they reference relative paths within `BUILD_WORKSPACE_DIRECTORY`. And
finally, `bazel test @rules_scala//test/scalafmt:formatted-test` fails
because it tries to declare output files in a nonexistent directory.

```txt
ERROR: .../external/rules_scala~/test/scalafmt/BUILD:43:20:
  in scalafmt_scala_test rule
  @@rules_scala~//test/scalafmt:formatted-test:

Traceback (most recent call last):
  File ".../external/rules_scala~/scala/private/rules/scala_test.bzl",
    line 38, column 22, in _scala_test_impl
      return run_phases(
  File ".../external/rules_scala~/scala/private/phases/api.bzl",
    line 45, column 23, in run_phases
      return _run_phases(ctx, builtin_customizable_phases, target = None)
  File ".../external/rules_scala~/scala/private/phases/api.bzl",
    line 77, column 32, in _run_phases
      new_provider = function(ctx, current_provider)
  File ".../external/rules_scala~/scala/private/phases/phase_scalafmt.bzl",
    line 10, column 46, in phase_scalafmt
      manifest, files, srcs = _build_format(ctx)
  File ".../external/rules_scala~/scala/private/phases/phase_scalafmt.bzl",
    line 30, column 44, in _build_format
      file = ctx.actions.declare_file("{}.fmt.output".format(src.short_path))

Error in declare_file:
  the output artifact
  'external/rules_scala~/test/rules_scala~/test/scalafmt/formatted/formatted-test.scala.fmt.output'
  is not under package directory
  'external/rules_scala~/test/scalafmt'
  for target '@@rules_scala~//test/scalafmt:formatted-test'

ERROR: .../external/rules_scala~/test/scalafmt/BUILD:43:20:
  Analysis of target '@@rules_scala~//test/scalafmt:formatted-test'
  failed
```

So invoking `bazel test //:ScalafmtTest` (via `/...` from `ALL_TARGETS`)
is sufficient, since we're only validating that the toolchains execute
properly. (`test_scalafmt.sh` tests the behavior of the `.format` and
`.format-test` scripts, _including_ on Windows via `--run_under=bash`.)
This also means we can invoke this target on Windows, instead of having
special case logic to avoid invoking the previous `bazel run` command.
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

Successfully merging this pull request may close these issues.

2 participants