Skip to content

Commit 299d448

Browse files
mblandmbland
committed
Fix setup_scala_toolchains, clean up test scripts
Adds a `Label()` wrapper to `setup_scala_toolchains()` and `scala_cross_version_select() to fix `@rules_scala_config` invisibilty build breakages under Bzlmod. Enables Bash strict mode for all test scripts via `set -euo pipefail` and fixes the resulting breakages. Removes `use_repo(scala_config, "rules_scala_config")` from `dt_patches/test_dt_patches{,_user_srcjar}/MODULE.bazel. This caused `dt_patches/dt_patch_test.sh` to break before adding the `Label()` wrapper to `setup_scala_toolchains()`. Also updates `dt_patches/dt_patch_test.sh` to use `test/shell/test_runner.sh` and `test/shell/test_helper.sh`. Replaces previous instances of its own `run_test_local` with the result of `get_test_runner`. Updates `run_in_test_repo` argument parsing, updates `run_in_test_repo` calls to use proper string quoting, and ensures any failures exit the script. Other general test updates include: - Adds a new `dt_patches_linux` job to `.bazelci/presubmit.yml` to run `dt_patches/dt_patch_test.sh` as part of the CI build. - Sets `set -euo pipefail` in `test_runner.sh` and `test_helper.sh` and fixes all unbound variables and pipe failures. Updates test helpers in those files to perform better argument quoting, argument pass through, command execution, and output printing. - Updates test helpers in `test_runner.sh` and `test_helper.sh` to show more information when the new `verbose_test_output` helper returns success, even when a test passes. Removes `exit 0` everywhere so successful tests return control to `run_test_local` when `verbose_test_output` is in effect. Adds an exit in `run_test_local` when `RULES_SCALA_TEST_ONLY` is nonempty. - Adds `#!/usr/bin/env bash` to a number of test files. This avoids having some test helper `echo` statements from printing the `-e` argument when running the test file directly. (As opposed to running them via `test_all.sh` or `test_rules_scala.sh`.) - Adds `test/shell/test_macros.sh` to `test_rules_scala.sh`. Updates that fixed previously incorrect behavior of specific tests include: - Fixes `test_scala_import_source_jar.sh` after Bash strict mode fixes revealed underlying breakages. Specifically, `if [[ $1 ]]` in `assert_file_exists()` always returned true, which meant the function always returned true, because the src jar never existed. Also, the expected repo path was never valid under Bzlmod, since it relied on the `WORKSPACE` specific repo path naming convention. Changing the condition to `if [[ -n "$1" ]]` fulfilled the actual intention, and revealed the broken repo path and `fetch_sources` never being true. Ensures `fetch_sources` is `True` for `@com_google_guava_guava_21_0` from `//scala/private/extensions/dev_deps.bzl%dev_deps`. Also makes the directory path to the repo Bzlmod compatible. (Fortunately, the underlying behavior the test intended to validate was already correct.) - Fixes `test_scala_classpath.sh` to actually grep for the `expected` string. Previously is was grepping for the empty string, thanks to the unbound `$method` variable, and always passed. Fixing this revealed that the test would fail when run if the test target artifacts already existed. Updated it to remove the test target artifacts so that the test could run correctly every time. (Again, the underlying behavior was already correct.) Test updates resulting from Bash strict mode that didn't fundamentally change test behavior include: - Fixed the unbound `$target_bes` variable in `test_build_event_protocol.sh` by replacing it with `${bes_file}`. The original intention appeared to be `${target}_bes.txt`, and the test previously just happened to work by using a file named `.txt`. - Refactored `test_invalid_scalacopts.sh` to use `action_should_fail_with{,out}_message`. This resolves the previous pipe failure after turning on Bash strict mode. - Updates `test_macros.sh` to use Bash stderr redirection syntax. Overlaps with a change in bazel-contrib#1740, but shouldn't conflict. - All other test updates defined previously unbound variables and fixed improper argument quotation. --- This contains an easy fix for a basic Bzlmod breakage. In looking to provide a test for it, I went down the path of improving _all_ the shell tests by applying Bash strict mode. This turned into a larger pull request than I intended; as always, I'm happy to split it up if so desired. Vince Rose reported the `setup_scala_toolchains` breakage under Bzlmod in the #scala channel of the Bazel Slack Workspace on 2025-05-21: - https://bazelbuild.slack.com/archives/CDCKJ2KFZ/p1747870770592779 `dt_patches/dt_patch_test.sh` would've caught it had I not added `use_repo(scala_config, "rules_scala_config")` to the `MODULE.bazel` files. Removing these `use_repo` calls reproduced the error, and they passed again once I reintroduced the proper `Label` instances. Rather than add another test, I thought it was time to ensure `dt_patch_test.sh` behaved properly and could run in CI. Also, @jayconrod recently reminded me of Bash strict mode during the course of an internal code review. So I added Bash strict mode to `dt_patch_test.sh` and refactored it to ensure that a test failure properly exited the script. That went well, so I decided to set Bash strict mode in `test_runner.sh` and `test_helper.sh`. Hilarity ensued. Having Bash strict mode in place should greatly reduce the chances of adding tests that pass without actually doing what we think they're doing. Several of the updates show more info when `RULES_SCALA_TEST_ONLY` and `RULES_SCALA_TEST_VERBOSE` are in effect. This helped me immensely while getting to the root of these problems, especially when defining a previously unbound variable and having a test immediately pass. These `RULES_SCALA_TEST_*` variables helped me verify that they were _really_ passing, or accidentally getting by. There's still opportunities to refactor some of the tests to use more of the existing test helpers. I could do more test script cleanup passes in one or more future pull requests.
1 parent fb08327 commit 299d448

20 files changed

+352
-264
lines changed

‎.bazelci/presubmit.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ tasks:
117117
- echo "build --tool_java_language_version=21" >> .bazelrc
118118
- echo "build --tool_java_runtime_version=21" >> .bazelrc
119119
- "./test_rules_scala.sh"
120+
dt_patches_linux:
121+
name: "dt_patches/dt_patch_test"
122+
platform: ubuntu2004
123+
shell_commands:
124+
- "dt_patches/dt_patch_test.sh"
120125
dependency_versions_linux:
121126
name: "./test_dependency_versions"
122127
platform: ubuntu2004

‎dt_patches/dt_patch_test.sh

Lines changed: 115 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -1,160 +1,154 @@
11
#!/usr/bin/env bash
22

3+
set -euo pipefail
4+
35
dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
4-
NC='\033[0m'
5-
GREEN='\033[0;32m'
6-
RED='\033[0;31m'
7-
8-
run_test_local() {
9-
# runs the tests locally
10-
set +e
11-
SECONDS=0
12-
TEST_ARG=$@
13-
echo "running test $TEST_ARG"
14-
RES=$($TEST_ARG 2>&1)
15-
RESPONSE_CODE=$?
16-
DURATION=$SECONDS
17-
if [ $RESPONSE_CODE -eq 0 ]; then
18-
echo -e "${GREEN} Test \"$TEST_ARG\" successful ($DURATION sec) $NC"
19-
else
20-
echo $RES
21-
echo -e "${RED} Test \"$TEST_ARG\" failed $NC ($DURATION sec) $NC"
22-
return $RESPONSE_CODE
23-
fi
24-
}
6+
# shellcheck source=./test_runner.sh
7+
. "${dir}"/../test/shell/test_runner.sh
8+
. "${dir}"/../test/shell/test_helper.sh
9+
runner="$(get_test_runner "${1:-local}")"
2510

2611
run_in_test_repo() {
27-
local test_command=$1
28-
local test_repo=$2
12+
local test_repo="$1"
13+
local test_command=("${@:2}")
14+
local response_code=0
2915

30-
cd "${dir}/${test_repo}" || exit 1
31-
${test_command}
32-
RESPONSE_CODE=$?
16+
cd "${dir}/${test_repo}" || return 1
17+
"${test_command[@]}" || response_code=$?
3318

3419
bazel shutdown
35-
3620
cd ../..
37-
38-
return $RESPONSE_CODE
21+
return $response_code
3922
}
4023

4124
test_compiler_patch() {
4225
local SCALA_VERSION="$1"
4326

44-
run_in_test_repo "bazel build //... --repo_env=SCALA_VERSION=${SCALA_VERSION} //..." "test_dt_patches"
27+
run_in_test_repo \
28+
test_dt_patches \
29+
bazel build "--repo_env=SCALA_VERSION=${SCALA_VERSION}" //...
4530
}
4631

4732
test_compiler_srcjar() {
4833
set -o pipefail
4934
local SCALA_VERSION="$1"
5035

51-
run_in_test_repo "bazel build //... --repo_env=SCALA_VERSION=${SCALA_VERSION} //..." "test_dt_patches_user_srcjar" 2>&1 | (! grep "canonical reproducible")
36+
run_in_test_repo \
37+
test_dt_patches_user_srcjar \
38+
bazel build "--repo_env=SCALA_VERSION=${SCALA_VERSION}" //... 2>&1 |
39+
(! grep "canonical reproducible")
5240
}
5341

5442
test_compiler_srcjar_nonhermetic() {
5543
set -o pipefail
5644
local SCALA_VERSION="$1"
5745

58-
run_in_test_repo "bazel build //... --repo_env=SCALA_VERSION=${SCALA_VERSION} //..." "test_dt_patches_user_srcjar" 2>&1 | grep "canonical reproducible"
46+
run_in_test_repo \
47+
test_dt_patches_user_srcjar \
48+
bazel build "--repo_env=SCALA_VERSION=${SCALA_VERSION}" //... 2>&1 |
49+
grep 'canonical reproducible'
5950
}
6051

6152
test_compiler_srcjar_error() {
6253
local SCALA_VERSION="$1"
63-
local EXPECTED_ERROR="scala_compiler_srcjar invalid"
6454

65-
run_in_test_repo "bazel build //... --repo_env=SCALA_VERSION=${SCALA_VERSION} //..." "test_dt_patches_user_srcjar" 2>&1 | grep "$EXPECTED_ERROR"
55+
run_in_test_repo \
56+
test_dt_patches_user_srcjar \
57+
action_should_fail_with_message \
58+
'scala_compiler_srcjar invalid' \
59+
build "--repo_env=SCALA_VERSION=${SCALA_VERSION}" //...
6660
}
6761

68-
#run_test_local test_compiler_patch 2.11.0
69-
#run_test_local test_compiler_patch 2.11.1
70-
#run_test_local test_compiler_patch 2.11.2
71-
#run_test_local test_compiler_patch 2.11.3
72-
#run_test_local test_compiler_patch 2.11.4
73-
#run_test_local test_compiler_patch 2.11.5
74-
#run_test_local test_compiler_patch 2.11.6
75-
#run_test_local test_compiler_patch 2.11.7
76-
#run_test_local test_compiler_patch 2.11.8
77-
#run_test_local test_compiler_patch 2.11.9
78-
#run_test_local test_compiler_patch 2.11.10
79-
#run_test_local test_compiler_patch 2.11.11
80-
run_test_local test_compiler_patch 2.11.12
81-
82-
#run_test_local test_compiler_patch 2.12.0
83-
run_test_local test_compiler_patch 2.12.1
84-
run_test_local test_compiler_patch 2.12.2
85-
run_test_local test_compiler_patch 2.12.3
86-
run_test_local test_compiler_patch 2.12.4
87-
run_test_local test_compiler_patch 2.12.5
88-
run_test_local test_compiler_patch 2.12.6
89-
run_test_local test_compiler_patch 2.12.7
90-
run_test_local test_compiler_patch 2.12.8
91-
run_test_local test_compiler_patch 2.12.9
92-
run_test_local test_compiler_patch 2.12.10
93-
run_test_local test_compiler_patch 2.12.11
94-
run_test_local test_compiler_patch 2.12.12
95-
run_test_local test_compiler_patch 2.12.13
96-
run_test_local test_compiler_patch 2.12.14
97-
run_test_local test_compiler_patch 2.12.15
98-
run_test_local test_compiler_patch 2.12.16
99-
run_test_local test_compiler_patch 2.12.17
100-
run_test_local test_compiler_patch 2.12.18
101-
run_test_local test_compiler_patch 2.12.19
102-
run_test_local test_compiler_patch 2.12.20
103-
104-
run_test_local test_compiler_patch 2.13.0
105-
run_test_local test_compiler_patch 2.13.1
106-
run_test_local test_compiler_patch 2.13.2
107-
run_test_local test_compiler_patch 2.13.3
108-
run_test_local test_compiler_patch 2.13.4
109-
run_test_local test_compiler_patch 2.13.5
110-
run_test_local test_compiler_patch 2.13.6
111-
run_test_local test_compiler_patch 2.13.7
112-
run_test_local test_compiler_patch 2.13.8
113-
run_test_local test_compiler_patch 2.13.10
114-
run_test_local test_compiler_patch 2.13.11
115-
run_test_local test_compiler_patch 2.13.12
116-
run_test_local test_compiler_patch 2.13.14
117-
run_test_local test_compiler_patch 2.13.15
118-
run_test_local test_compiler_patch 2.13.16
119-
120-
run_test_local test_compiler_patch 3.1.0 # Minimal supported version
121-
run_test_local test_compiler_patch 3.1.3
122-
run_test_local test_compiler_patch 3.2.2
123-
run_test_local test_compiler_patch 3.3.6
124-
run_test_local test_compiler_patch 3.4.3
125-
run_test_local test_compiler_patch 3.5.2
126-
run_test_local test_compiler_patch 3.6.4
127-
run_test_local test_compiler_patch 3.7.0
128-
129-
run_test_local test_compiler_srcjar_error 2.12.11
130-
run_test_local test_compiler_srcjar_error 2.12.12
131-
run_test_local test_compiler_srcjar_error 2.12.13
62+
#$runner test_compiler_patch 2.11.0
63+
#$runner test_compiler_patch 2.11.1
64+
#$runner test_compiler_patch 2.11.2
65+
#$runner test_compiler_patch 2.11.3
66+
#$runner test_compiler_patch 2.11.4
67+
#$runner test_compiler_patch 2.11.5
68+
#$runner test_compiler_patch 2.11.6
69+
#$runner test_compiler_patch 2.11.7
70+
#$runner test_compiler_patch 2.11.8
71+
#$runner test_compiler_patch 2.11.9
72+
#$runner test_compiler_patch 2.11.10
73+
#$runner test_compiler_patch 2.11.11
74+
$runner test_compiler_patch 2.11.12
75+
76+
#$runner test_compiler_patch 2.12.0
77+
$runner test_compiler_patch 2.12.1
78+
$runner test_compiler_patch 2.12.2
79+
$runner test_compiler_patch 2.12.3
80+
$runner test_compiler_patch 2.12.4
81+
$runner test_compiler_patch 2.12.5
82+
$runner test_compiler_patch 2.12.6
83+
$runner test_compiler_patch 2.12.7
84+
$runner test_compiler_patch 2.12.8
85+
$runner test_compiler_patch 2.12.9
86+
$runner test_compiler_patch 2.12.10
87+
$runner test_compiler_patch 2.12.11
88+
$runner test_compiler_patch 2.12.12
89+
$runner test_compiler_patch 2.12.13
90+
$runner test_compiler_patch 2.12.14
91+
$runner test_compiler_patch 2.12.15
92+
$runner test_compiler_patch 2.12.16
93+
$runner test_compiler_patch 2.12.17
94+
$runner test_compiler_patch 2.12.18
95+
$runner test_compiler_patch 2.12.19
96+
$runner test_compiler_patch 2.12.20
97+
98+
$runner test_compiler_patch 2.13.0
99+
$runner test_compiler_patch 2.13.1
100+
$runner test_compiler_patch 2.13.2
101+
$runner test_compiler_patch 2.13.3
102+
$runner test_compiler_patch 2.13.4
103+
$runner test_compiler_patch 2.13.5
104+
$runner test_compiler_patch 2.13.6
105+
$runner test_compiler_patch 2.13.7
106+
$runner test_compiler_patch 2.13.8
107+
$runner test_compiler_patch 2.13.10
108+
$runner test_compiler_patch 2.13.11
109+
$runner test_compiler_patch 2.13.12
110+
$runner test_compiler_patch 2.13.14
111+
$runner test_compiler_patch 2.13.15
112+
$runner test_compiler_patch 2.13.16
113+
114+
$runner test_compiler_patch 3.1.0 # Minimal supported version
115+
$runner test_compiler_patch 3.1.3
116+
$runner test_compiler_patch 3.2.2
117+
$runner test_compiler_patch 3.3.6
118+
$runner test_compiler_patch 3.4.3
119+
$runner test_compiler_patch 3.5.2
120+
$runner test_compiler_patch 3.6.4
121+
$runner test_compiler_patch 3.7.0
122+
123+
$runner test_compiler_srcjar_error 2.12.11
124+
$runner test_compiler_srcjar_error 2.12.12
125+
$runner test_compiler_srcjar_error 2.12.13
132126

133127
# These tests are semi-stateful, if two tests are run sequentially with the
134128
# same Scala version, the DEBUG message about a canonical reproducible form
135129
# that we grep for will only be outputted the first time (on Bazel >= 6).
136130
# So we clean the repo first to ensure consistency.
137131

138-
run_in_test_repo "bazel clean --expunge" "test_dt_patches_user_srcjar"
139-
140-
run_test_local test_compiler_srcjar 2.12.14
141-
run_test_local test_compiler_srcjar 2.12.15
142-
run_test_local test_compiler_srcjar 2.12.16
143-
run_test_local test_compiler_srcjar_nonhermetic 2.12.17
144-
run_test_local test_compiler_srcjar_nonhermetic 2.12.18
145-
run_test_local test_compiler_srcjar_nonhermetic 2.12.19
146-
run_test_local test_compiler_srcjar_nonhermetic 2.12.20
147-
148-
run_test_local test_compiler_srcjar_nonhermetic 2.13.11
149-
run_test_local test_compiler_srcjar_nonhermetic 2.13.12
150-
run_test_local test_compiler_srcjar_nonhermetic 2.13.14
151-
run_test_local test_compiler_srcjar_nonhermetic 2.13.15
152-
run_test_local test_compiler_srcjar_nonhermetic 2.13.16
153-
154-
run_test_local test_compiler_srcjar 3.1.3
155-
run_test_local test_compiler_srcjar 3.2.2
156-
run_test_local test_compiler_srcjar_nonhermetic 3.3.6
157-
run_test_local test_compiler_srcjar 3.4.3
158-
run_test_local test_compiler_srcjar_nonhermetic 3.5.2
159-
run_test_local test_compiler_srcjar_nonhermetic 3.6.4
160-
run_test_local test_compiler_srcjar_nonhermetic 3.7.0
132+
run_in_test_repo 'test_dt_patches_user_srcjar' bazel clean --expunge
133+
134+
$runner test_compiler_srcjar 2.12.14
135+
$runner test_compiler_srcjar 2.12.15
136+
$runner test_compiler_srcjar 2.12.16
137+
$runner test_compiler_srcjar_nonhermetic 2.12.17
138+
$runner test_compiler_srcjar_nonhermetic 2.12.18
139+
$runner test_compiler_srcjar_nonhermetic 2.12.19
140+
$runner test_compiler_srcjar_nonhermetic 2.12.20
141+
142+
$runner test_compiler_srcjar_nonhermetic 2.13.11
143+
$runner test_compiler_srcjar_nonhermetic 2.13.12
144+
$runner test_compiler_srcjar_nonhermetic 2.13.14
145+
$runner test_compiler_srcjar_nonhermetic 2.13.15
146+
$runner test_compiler_srcjar_nonhermetic 2.13.16
147+
148+
$runner test_compiler_srcjar 3.1.3
149+
$runner test_compiler_srcjar 3.2.2
150+
$runner test_compiler_srcjar_nonhermetic 3.3.6
151+
$runner test_compiler_srcjar 3.4.3
152+
$runner test_compiler_srcjar_nonhermetic 3.5.2
153+
$runner test_compiler_srcjar_nonhermetic 3.6.4
154+
$runner test_compiler_srcjar_nonhermetic 3.7.0

‎dt_patches/test_dt_patches/MODULE.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ scala_config = use_extension(
2121
scala_config.settings(
2222
enable_compiler_dependency_tracking = True,
2323
)
24-
use_repo(scala_config, "rules_scala_config")
2524

2625
bazel_dep(name = "compiler_sources")
2726
local_path_override(

‎dt_patches/test_dt_patches_user_srcjar/MODULE.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ scala_config = use_extension(
2121
scala_config.settings(
2222
enable_compiler_dependency_tracking = True,
2323
)
24-
use_repo(scala_config, "rules_scala_config")
2524

2625
bazel_dep(name = "compiler_sources")
2726
local_path_override(

‎scala/private/extensions/dev_deps.bzl

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,12 @@ def dev_deps_repositories(
6868

6969
repositories(
7070
fetch_sources = fetch_sources,
71+
fetch_sources_by_id = {
72+
# Required by test/shell/test_scala_import_source_jar.sh. Without
73+
# this, the first test will always fail, and the
74+
# `BAZEL_JVM_FETCH_SOURCES` environment variable has no effect.
75+
"com_google_guava_guava_21_0": True,
76+
},
7177
for_artifact_ids = [
7278
# test adding a scala jar:
7379
"com_twitter__scalding_date",

‎scala/private/macros/setup_scala_toolchain.bzl

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,10 @@ def setup_scala_toolchain(
9999
toolchain = ":%s_impl" % name,
100100
toolchain_type = Label("//scala:toolchain_type"),
101101
target_settings = [
102-
"@rules_scala_config//:scala_version" +
103-
version_suffix(scala_version),
102+
Label(
103+
"@rules_scala_config//:scala_version" +
104+
version_suffix(scala_version),
105+
),
104106
],
105107
visibility = visibility,
106108
)

‎scala/scala_cross_version_select.bzl

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ def select_for_scala_version(default = [], **kwargs):
4141
"""
4242

4343
return select({
44-
"@rules_scala_config//:scala_version" + version_suffix(scala_version): _matches_for_version(scala_version, kwargs, default)
44+
Label(
45+
"@rules_scala_config//:scala_version" + version_suffix(scala_version),
46+
): _matches_for_version(scala_version, kwargs, default)
4547
for scala_version in SCALA_VERSIONS
4648
})
4749

‎test/shell/test_build_event_protocol.sh

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,22 @@ dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
44
runner=$(get_test_runner "${1:-local}")
55

66
scala_binary_common_jar_is_exposed_in_build_event_protocol() {
7-
local target=$1
8-
local target_suffix=${2:-""}
7+
local target="$1"
8+
local target_suffix="${2:-}"
9+
local bes_file="${target}_bes.txt"
10+
local jar_file="test/${target}${target_suffix}.jar"
911
set +e
10-
bazel build test:$target --build_event_text_file=$target_bes.txt
11-
cat $target_bes.txt | grep "test/$target$target_suffix.jar"
12-
if [ $? -ne 0 ]; then
13-
echo "test/$target$target_suffix.jar was not found in build event protocol:"
14-
cat $target_bes.txt
15-
rm $target_bes.txt
12+
13+
bazel build "test:${target}" "--build_event_text_file=${bes_file}"
14+
cat "${bes_file}" | grep "${jar_file}"
15+
if [[ $? -ne 0 ]]; then
16+
echo "${jar_file} was not found in build event protocol:"
17+
cat "${bes_file}"
18+
rm "${bes_file}"
1619
exit 1
1720
fi
1821

19-
rm $target_bes.txt
22+
rm "${bes_file}"
2023
set -e
2124
}
2225

0 commit comments

Comments
 (0)