Commit bde68a5
committed
Simplify test_dependency_versions.sh
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.1 parent 933d698 commit bde68a5
File tree
39 files changed
+281
-479
lines changed- deps/test
- dt_patches
- test_dt_patches_user_srcjar
- test_dt_patches
- examples
- crossbuild
- overridden_artifacts
- scala3
- semanticdb
- testing
- multi_frameworks_toolchain
- scalatest_repositories
- specs2_junit_repositories
- twitter_scrooge
- test_cross_build
- test_version
- test
- jmh
- runtime
- sh_tests
- src/main/scala/scalarules/test/twitter_scrooge
- strings
- third_party/test/proto
39 files changed
+281
-479
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | | - | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
| 3 | + | |
58 | 4 | | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
| 5 | + | |
| 6 | + | |
66 | 7 | | |
67 | 8 | | |
68 | 9 | | |
69 | 10 | | |
70 | 11 | | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | | - | |
115 | | - | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | | - | |
139 | | - | |
140 | | - | |
141 | | - | |
142 | | - | |
143 | | - | |
144 | | - | |
145 | | - | |
146 | | - | |
147 | | - | |
148 | | - | |
149 | | - | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | | - | |
156 | | - | |
This file was deleted.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
4 | | - | |
5 | | - | |
6 | | - | |
| 3 | + | |
7 | 4 | | |
8 | 5 | | |
9 | 6 | | |
10 | 7 | | |
11 | 8 | | |
12 | 9 | | |
13 | 10 | | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
14 | 17 | | |
15 | 18 | | |
16 | 19 | | |
| |||
This file was deleted.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
2 | | - | |
3 | | - | |
| 1 | + | |
| 2 | + | |
4 | 3 | | |
5 | 4 | | |
6 | 5 | | |
7 | 6 | | |
8 | 7 | | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
| 11 | + | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| |||
0 commit comments