-
-
Notifications
You must be signed in to change notification settings - Fork 287
Fix custom twitter_scrooge
toolchain breakages
#1747
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
base: master
Are you sure you want to change the base?
Fix custom twitter_scrooge
toolchain breakages
#1747
Conversation
Adds the `examples/twitter_scrooge` repository, adds `test_twitter_scrooge_example` to `test/shell/test_examples.sh`, and enables overriding all `twitter_scrooge` dependencies. Fixes bazel-contrib#1744. Adds `docs/twitter_scrooge.md` and includes extensive comments in `examples/twitter_scrooge/{BUILD,MODULE.bazel,WORKSPACE}`. Also updates `test/shell/test_examples.sh`: - It now uses `run_tests` from `test/shell/test_runner.sh`, and all test functions begin with `test_`. This enables automatic discovery and easy skipping of test cases (using the prefix `_test_`). - Replaces `test_example` with `run_in_example_dir`. This removes a subshell per test case, and allows for passing individually quoted arguments. --- Fixes two breakages related to overriding default `twitter_scrooge` dependencies under Bzlmod (neither problem manifested under `WORKSPACE). The first happened when using `setup_scrooge_toolchain` without instantiating the default Scala and `twitter_scrooge` toolchains and importing some of their repos. Even with all the available dependency parameters to `setup_scrooge_toolchain` specified, the build would break with errors like: ```txt no such package '@@[unknown repo 'io_bazel_rules_scala_scopt_2_13_16' requested from @@]//': The repository '@@[unknown repo 'io_bazel_rules_scala_scopt_2_13_16' requested from @@]' could not be resolved: No repository visible as '@io_bazel_rules_scala_scopt_2_13_16' from main repository and referenced by '//tools/scala:compiler_classpath_provider' ``` This was because `setup_scala_toolchain` still contained hardcoded references to `io_bazel_rules_scala_scopt` and other internal dependency repos. The workaround was to instantiate the builtin toolchains, then use `use_repo` to bring these internal repositories into scope. The fix was to make all of these repos overridable via `setup_scala_toolchain` and the `scala_deps.twitter_scrooge()` tag class. Now providing all the dependency arguments to `setup_scala_toolchain` avoids the need instantiate the builtin toolchains. The second breakage happened when specifying overrides for the `scala_deps.twitter_scrooge()` tag class: ```txt @@rules_scala+//scala/extensions:rules_scala++scala_deps+rules_scala_toolchains: expected value of type 'string' for dict value element, but got Label("@@rules_jvm_external++maven+maven//:org_apache_thrift_libthrift") ``` This happened because: - The `scala_deps` module extension compiled its `twitter_scrooge` tag class Labels into a dictionary. - It passed this `string` to `Label` dict to `scala_toolchains()`, which assigned it to the `twitter_scrooge_options` attribute of `scala_toolchains_repo`. - The `scala_toolchains_repo` attribute is of type `attr.string_dict`, not `attr.string_keyed_label_dict`. The fix was to translate the Labels in this dictionary into strings at the point of `scala_toolchains_repo` assignment. We don't officially support Bazel 6 anymore, but I didn't want to change the attribute to `string_keyed_label_dict` just yet. Things still work fine without it, and it gives Bazel 6 users a little more wiggle room and time to migrate.
Hmm, looks like Bazel release server issues are bringing down the build: 2025/06/12 18:49:21 Downloading https://releases.bazel.build/7.6.1/release/bazel-7.6.1-linux-x86_64...
--
| 2025/06/12 18:49:56 could not download Bazel:
failed to download bazel: failed to download bazel:
HTTP GET https://releases.bazel.build/7.6.1/release/bazel-7.6.1-linux-x86_64 failed:
unable to complete 3 requests to https://releases.bazel.build/7.6.1/release/bazel-7.6.1-linux-x86_64
within 30s. Most recent failure: HTTP 500 I'll wait a bit and push an empty commit to get another build going a little later. |
Noticed build failures for EngFlow's internal CI today, too. One of my colleagues noted that, apparently, GCP is having a day today. Will push the empty commit to trigger a new build once the GCP fires are out. |
This is left over from my first draft, when I'd written the new tests in `test/shell/test_twitter_scrooge_toolchains.sh`.
OK, GCP is happy again, and I just pushed a new (nonempty) commit. The build is happy. While I was waiting, I tried building under |
Fixes a few problems when building under `WORKSPACE` with Bazel 8.2.1 (7.6.1 doesn't require these changes). Adds to `.bazelrc` the `--incompatible_autoload_externally=` flag as a common option for all builds, and a (disabled) line of options for `WORKSPACE` builds. Bumps these development dependency versions: - `com_google_buildifier_buildtools`: 5.1.0 => 8.2.1 - `rules_shell`: 0.4.1 => 0.5.0 --- Though `WORKSPACE` is on the way out, we should ensure that `rules_scala` remains as compatible as it can be until it's totally gone. All of these errors happened when running `./test_all` with Bazel 8.2.1 and `WORKSPACE` enabled while working on bazel-contrib#1747. The first error was the following "cycle". (I later realized it's somehow due to bazelbuild/rules_java#294 from `rules_java` 8.12.0. See the note at the very end below.) ```sh $ bazel run //tools:lint_check ERROR: Cycle caused by autoloads, failed to load .bzl file '@@bazel_features_version//:version.bzl'. Add 'bazel_features_version' to --repositories_without_autoloads or disable autoloads by setting '--incompatible_autoload_externally=' More information on bazelbuild/bazel#23043. ``` `--incompatible_autoload_externally=` fixed this problem, but also precipitated all the other errors below. - bazelbuild/bazel#23043 - https://bazel.build/reference/command-line-reference#common_options-flag--incompatible_autoload_externally Updating `com_github_bazelbuild_buildtools` to v8.2.1 fixes the next error, whereby Bazel no longer autoloaded `sh_test`. v5.1.0 was missing the required `load("@rules_shell//shell:sh_test.bzl", "sh_test")` statement, added in v8.0.3 by bazelbuild/buildtools#1332: ```sh $ bazel run //tools:lint_check ERROR: .../external/com_github_bazelbuild_buildtools/buildifier/BUILD.bazel:60:1: name 'sh_test' is not defined (did you mean 'cc_test'?) ERROR: .../external/com_github_bazelbuild_buildtools/buildifier/BUILD.bazel: no such target '@@com_github_bazelbuild_buildtools//buildifier:runner.bash.template': target 'runner.bash.template' not declared in package 'buildifier' defined by .../external/com_github_bazelbuild_buildtools/buildifier/BUILD.bazel; however, a source file of this name exists. (Perhaps add 'exports_files(["runner.bash.template"])' to buildifier/BUILD?) ERROR: /Users/mbland/src/bazel-contrib/rules_scala/tools/BUILD:19:11: every rule of type _buildifier implicitly depends upon the target '@@com_github_bazelbuild_buildtools//buildifier:runner.bash.template', but this target could not be found because of: no such target '@@com_github_bazelbuild_buildtools//buildifier:runner.bash.template': target 'runner.bash.template' not declared in package 'buildifier' defined by .../external/com_github_bazelbuild_buildtools/buildifier/BUILD.bazel; however, a source file of this name exists. (Perhaps add 'exports_files(["runner.bash.template"])' to buildifier/BUILD?) ERROR: Analysis of target '//tools:lint_check' failed; build aborted: Analysis failed ``` Upgrading to v8.2.1 and updating `//tools:lint_check` to become a `buildifier_test` also finally got rid of this next warning. This required exporting the `MODULE.bazel` file from the root package, disabling one lint warning, and accepting a couple of new lint fixes: ```txt DEBUG: .../external/+dev_deps+com_github_bazelbuild_buildtools/buildifier/internal/factory.bzl:17:10: DEPRECATION NOTICE: value 'check' for attribute 'mode' will be removed in the future. Migrate '@@//tools:lint_check' to buildifier_test. ``` Adding `rules_jvm_external` 6.7 to `//scala:latest_deps.bzl` fixes this next error, described in detail in: - protocolbuffers/protobuf#19129 (comment) ```sh $ bazel build --test_output=errors src/... test/... ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:169:21: @@com_google_protobuf//java/core:lite_mvn-lib: no such attribute 'javacopts' in 'java_library' rule ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:169:21: @@com_google_protobuf//java/core:lite_mvn-lib: no such attribute 'resources' in 'java_library' rule (did you mean 'features'?) ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:169:21: @@com_google_protobuf//java/core:lite_mvn-lib: no such attribute 'runtime_deps' in 'java_library' rule ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:287:21: @@com_google_protobuf//java/core:core_mvn-lib: no such attribute 'javacopts' in 'java_library' rule ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:287:21: @@com_google_protobuf//java/core:core_mvn-lib: no such attribute 'resources' in 'java_library' rule (did you mean 'features'?) ERROR: .../external/com_google_protobuf/java/core/BUILD.bazel:287:21: @@com_google_protobuf//java/core:core_mvn-lib: no such attribute 'runtime_deps' in 'java_library' rule ERROR: .../external/com_google_protobuf/BUILD.bazel:475:6: Target '@@com_google_protobuf//java/core:core' contains an error and its package is in error and referenced by '@@com_google_protobuf//:protobuf_java' ERROR: Analysis of target '//test/proto/custom_generator:failing_scala_proto_deps_toolchain_def' failed; build aborted: Analysis failed ``` All of the previous errors only affected `rules_scala`'s own development builds and test runs, when it is the main repository/root module. This final error is something users could possibly run into when using `--incompatible_autoload_externally=` with `rules_scala`. `jvm_import` statements in Maven dependency repos generated by `jvm_import_external` from `//scala:scala_maven_import_external.bzl` began to fail. Defining `_JAVA_IMPORT_RULE_LOAD` and using it as the default value for the `rule_load` attribute of the `_jvm_import_external` rule fixed this: ```sh $ bazel test --test_output=errors third_party/... ERROR: .../external/org_apache_commons_commons_lang_3_5_without_file/BUILD:7:12: @@org_apache_commons_commons_lang_3_5_without_file//:org_apache_commons_commons_lang_3_5_without_file: no such attribute 'jars' in 'java_import' rule ERROR: .../external/org_apache_commons_commons_lang_3_5_without_file/BUILD:7:12: @@org_apache_commons_commons_lang_3_5_without_file//:org_apache_commons_commons_lang_3_5_without_file: no such attribute 'neverlink' in 'java_import' rule ERROR: .../external/org_apache_commons_commons_lang_3_5_without_file/BUILD:14:12: @@org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file: no such attribute 'jars' in 'java_import' rule ERROR: .../third_party/dependency_analyzer/src/test/BUILD:4:6: Target '@@org_apache_commons_commons_lang_3_5_without_file//:linkable_org_apache_commons_commons_lang_3_5_without_file' contains an error and its package is in error and referenced by '//third_party/dependency_analyzer/src/test:strict_deps_test' ERROR: Analysis of target '//third_party/dependency_analyzer/src/test:strict_deps_test' failed; build aborted: Analysis failed ``` --- And just now, I'm noticing that I'd already added the following to `.bazelrc`, where the `rules_java` release references bazelbuild/bazel#26119: ```sh # Uncomment for WORKSPACE builds for Bazel [8.0.0, 8.3.0) per: # https://github.com/bazelbuild/rules_java/releases/tag/8.12.0 #common --repositories_without_autoloads=bazel_features_version,bazel_features_globals ``` Oh well. But now we're future proof.
Description
Adds the
examples/twitter_scrooge
repository, addstest_twitter_scrooge_example
totest/shell/test_examples.sh
, and enables overriding alltwitter_scrooge
dependencies. Fixes #1744.Adds
docs/twitter_scrooge.md
and includes extensive comments inexamples/twitter_scrooge/{BUILD,MODULE.bazel,WORKSPACE}
.Also updates
test/shell/test_examples.sh
:It now uses
run_tests
fromtest/shell/test_runner.sh
, and all test functions begin withtest_
. This enables automatic discovery and easy skipping of test cases (using the prefix_test_
).Replaces
test_example
withrun_in_example_dir
. This removes a subshell per test case, and allows for passing individually quoted arguments.Motivation
Fixes two breakages related to overriding default
twitter_scrooge
dependencies under Bzlmod (neither problem manifested under `WORKSPACE).The first happened when using
setup_scrooge_toolchain
without instantiating the default Scala andtwitter_scrooge
toolchains and importing some of their repos. Even with all the available dependency parameters tosetup_scrooge_toolchain
specified, the build would break with errors like:This was because
setup_scala_toolchain
still contained hardcoded references toio_bazel_rules_scala_scopt
and other internal dependency repos. The workaround was to instantiate the builtin toolchains, then useuse_repo
to bring these internal repositories into scope.The fix was to make all of these repos overridable via
setup_scala_toolchain
and thescala_deps.twitter_scrooge()
tag class. Now providing all the dependency arguments tosetup_scala_toolchain
avoids the need instantiate the builtin toolchains.The second breakage happened when specifying overrides for the
scala_deps.twitter_scrooge()
tag class:@@rules_scala+//scala/extensions:rules_scala++scala_deps+rules_scala_toolchains: expected value of type 'string' for dict value element, but got Label("@@rules_jvm_external++maven+maven//:org_apache_thrift_libthrift")
This happened because:
The
scala_deps
module extension compiled itstwitter_scrooge
tag class Labels into a dictionary.It passed this
string
toLabel
dict toscala_toolchains()
, which assigned it to thetwitter_scrooge_options
attribute ofscala_toolchains_repo
.The
scala_toolchains_repo
attribute is of typeattr.string_dict
, notattr.string_keyed_label_dict
.The fix was to translate the Labels in this dictionary into strings at the point of
scala_toolchains_repo
assignment. We don't officially support Bazel 6 anymore, but I didn't want to change the attribute tostring_keyed_label_dict
just yet. Things still work fine without it, and it gives Bazel 6 users a little more wiggle room and time to migrate.