Migrate to Bzlmod #74

jadenPete · 2024-12-04T17:54:32Z

This PR removes WORKSPACE and tests/WORKSPACE, migrating the ruleset to Bzlmod. For the most part, the ruleset can be used just as it was before (although including it is much simpler, thanks to Bzlmod's system for importing external repositories). There are a few breaking changes, though:

Toolchains must be registered manually (i.e. without macros like scala_register_toolchains and scala_proto_register_toolchains), since Bzlmod only allows them to be registered in MODULE.bazel-like files
- See https://bazel.build/external/migration#register-toolchains
The default Scala toolchain is no longer set via scala_register_toolchains, since that macro no longer exists. Instead, users of the ruleset must set a flag like --@rules_scala_annex//rules/scala:scala-toolchain=annex_zinc_2_13 in their .bazelrc file.
- Previously, scala_register_toolchains invoked a repository rule that would generate the configuration value for the Scala toolchain. That way, the default could be set dynamically in the repository importing rules_scala_annex. This is no longer possible, since rules_scala_annex can't share repositories like rules_scala_annex_scala_toolchain with the repository that imports it.
For the targets in rules/scala_with_scalafmt.bzl, the default Scalafmt configuration file can't be set via scalafmt_default_config for the same reason mentioned above. Instead, we set it via a toolchain and use the new scalafmt_toolchain_name attribute to override the file for specific targets.
The emulate_rules_scala workspace macro, which was used to provide compatibility with bazelbuild/rules_scala's rules, has been deleted. It hasn't been updated in years and relied on deprecated Bazel constructs like bind and //external/... that are no longer supported in Bzlmod. I figured that now that we're pursuing unifying the rulesets, there's little point in keeping it around.

jadenPete · 2024-12-04T17:59:57Z

MODULE.bazel

+bazel_dep(name = "bazel_skylib", version = "1.7.1")
+
+# TODO: Use buildifier instead of buildifier_prebuilt once
+# https://github.com/bazelbuild/bazel-central-registry/issues/380 is resolved


I realized after I wrote this that we could just use archive_override, but I don't think it makes much sense to have users of the ruleset fetch an entire Go toolchain and compile buildifier from scratch when it's only used internally.

jjudd

Took a look through things. Had a few questions and comments. Mostly LGTM.

jjudd · 2024-12-06T03:02:20Z

MODULE.bazel

+
+# TODO: Use buildifier instead of buildifier_prebuilt once
+# https://github.com/bazelbuild/bazel-central-registry/issues/380 is resolved
+bazel_dep(name = "buildifier_prebuilt", version = "7.3.1")


Can we mark dev dependencies as such, so they don't propagate to downstream projects? I believe we can just set dev_dependency = True on the bazel_dep call to make that the case.

I originally tried marking this as a dev dependency, but then the test repository failed to build because we reference buildifier_prebuilt in @rules_scala_annex//:BUILD.bazel.

Huh that's interesting. Do we need to do that?

We do for BUILD file formatting. I think we do it because the CI hosts don't necessarily have buildifier installed, so we need to pull it in through the repository.

jjudd · 2024-12-06T03:03:11Z

MODULE.bazel

+bazel_dep(name = "rules_java", version = "7.12.2")
+bazel_dep(name = "rules_jvm_external", version = "6.5")
+bazel_dep(name = "rules_pkg", version = "1.0.1")
+bazel_dep(name = "stardoc", version = "0.7.1")


I think this is also a dev dependency

See my comment here:
#74 (comment)

jjudd · 2024-12-06T03:03:21Z

MODULE.bazel

+bazel_dep(name = "buildifier_prebuilt", version = "7.3.1")
+bazel_dep(name = "rules_java", version = "7.12.2")
+bazel_dep(name = "rules_jvm_external", version = "6.5")
+bazel_dep(name = "rules_pkg", version = "1.0.1")


I think this is also a dev dependency

See my comment here:
#74 (comment)

jjudd · 2024-12-06T03:12:12Z

README.md

-http_archive(
-    name = "rules_scala_annex",
-    integrity = "sha256-WjZvojiclkiyVxQ1NqkH1lDeGaDLyzQOGiDsCfhVAec=",
+archive_override(


Do folks need to set up toolchains in their repos with bzlmod or are our toolchains just set up in their repo by including our ruleset?

Nope, they're set up by including our ruleset. I think that's the one aspect of external repositories in Bzlmod that isn't isolated—toolchains are shared throughout the entire Bazel server. See the documentation on toolchain resolution, namely the bit about the order in which toolchains are resolved.

jjudd · 2024-12-06T03:13:55Z

docs/newdocs/scala_versions.md

-    "@scala_compiler_2_13//jar",
-    "@scala_library_2_13//jar",
-    "@scala_reflect_2_13//jar",
+    "@annex//:org_scala_lang_scala_compiler",


Do we want these to be @annex or should we assume that the person setting up the Scala toolchain has their own maven dependency tree?

Oh, that's a good point. The comment above says "You'll need to pull these in via rules_jvm_external", but it would be helpful to mention that annex should be replaced with the name of their dependency tree. I'll do that.

I suggest you rename it something like @maven I worry that people will miss your note and be confused as to how this is supposed to come from annex. I think both the @maven and your note will hopefully avoid that confusion.

Good idea. I'll rename it.

jjudd · 2024-12-06T03:27:31Z

tests/coverage/BUILD

@@ -8,14 +8,14 @@ scala_test(
    ],
    scala_toolchain_name = "test_zinc_2_13",
    runtime_deps = [
-        "@hamcrest_core//jar",
-        "@junit_interface//jar",
+        "@annex_test//:com_novocode_junit_interface",


Nice does this mean you removed the last vestiges of the jvm imported deps and everything is now using rules_jvm_external? If so, that's awesome.

jjudd · 2024-12-06T03:28:02Z

tests/MODULE.bazel

+
+http_file = use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl", "http_file")
+
+http_file(


This is scope creep, but can this be downloaded with rules_jvm_external instead?

See my comment here:
#74 (comment)

jjudd · 2024-12-06T03:28:15Z

tests/MODULE.bazel

+    urls = ["https://repo.maven.apache.org/maven2/com/chuusai/shapeless_2.13/2.3.7/shapeless_2.13-2.3.7.jar"],
+)
+
+http_file(


Similar thought here. Can this be downloaded with rules_jvm_external instead?

I considered migrating it to rules_jvm_external, but thought it was best if it continued to be fetched via http_file. That way, it wouldn't have JavaInfo or any extra providers added, which tests that scala_import doesn't rely on any of this information.

Ah. Do you want to add a comment to these http_files explaining why they aren't part of rules_jvm_external? I think others may have the same thought as me in the future.

jjudd · 2024-12-06T03:30:11Z

tests/dependencies/indirect/BUILD

@@ -1,19 +1,19 @@
 load("@rules_scala_annex//rules:register_toolchain.bzl", "register_zinc_toolchain")
 load("@rules_scala_annex//rules:scala.bzl", "scala_library")
-load("@rules_scala_annex//rules/scala:workspace.bzl", "scala_2_13_version")
+load("@rules_scala_annex//rules/scala:versions.bzl", "scala_2_13_version")


It feels weird to have versions come from annex and then the jars come from the test workspace, but perhaps that is just how things are now?

Hmm, it does. Would you prefer we split versions.bzl into two files: one for rules_scala_annex and another for rules_scala_annex_test? I figured that even though we're forced to declare the versions in multiple places and define separate dependencies trees, it would still be best to have a single source of truth.

Yeah probably best to minimize the number of places versions can come from and leave it as is.

jjudd · 2024-12-06T03:30:50Z

MODULE.bazel

+    "//src/main/scala:annex_zinc_3",
+)
+
+# Please ensure these stay up-to-date with the versions in `rules/scala/versions.bzl`.


Can we update this to ask folks to keep these in sync with versions.bzl and the test MODULE.bazel?

See my comment here:
#74 (comment)

Similar response.

This commit redeclares every dependency in `@annex//...` depended on in `/tests` in the `annex_test` repository. Fortunately in most cases, but unfortunately in this case, Bzlmod prohibits repositories from depending on each other's external repositories. That means that the repository in `/tests` can't depend on anything in `@annex//...` unless it defines an `annex` repository itself. I considered using `include` so that we could declare `annex` with the same set of dependencies in both repositories, but unfortunately, `/tests` wouldn't be able to `include` such a file because, according to the documentation for `include`, "only files in the main repo may be included".

…ain_type toolchain

jadenPete · 2024-12-06T18:50:30Z

Squashed.

Jaden Peterson added 2 commits December 4, 2024 12:33

Include rules_scala_annex in rules_scala_annex_test via MODULE.bazel

5382849

Declare most dependencies in MODULE.bazel

9d30d50

jadenPete requested a review from jjudd December 4, 2024 17:54

jadenPete self-assigned this Dec 4, 2024

jadenPete commented Dec 4, 2024

View reviewed changes

This was referenced Dec 5, 2024

Migrate to Bzlmod lucidsoftware/rules_play_routes#57

Merged

Migrate to Bzlmod lucidsoftware/rules_twirl#47

Merged

jjudd reviewed Dec 6, 2024

View reviewed changes

Jaden Peterson added 9 commits December 6, 2024 13:50

Depend on rules_jvm_external in MODULE.bazel

d92221c

Depend on bazel_skylib in MODULE.bazel

c71dd94

Manually register the //scalapb_scala_proto_toolchain:compiler_toolch…

dee2b33

…ain_type toolchain

Manually register the Scala toolchains

f80b42d

Provide Scalafmt configuration via a toolchain

b5fd124

Slimmed down tests/WORKSPACE as much as possible

21a369c

Removed emulate_rules_scala

2868152

Updated the documentation

4c0ec8d

jadenPete force-pushed the bzlmod-migration branch from 90657da to 4c0ec8d Compare December 6, 2024 18:50

Made as many dependencies dev dependencies as can be

5de9438

jadenPete force-pushed the bzlmod-migration branch from 12a9d24 to 5de9438 Compare December 6, 2024 19:23

jjudd approved these changes Dec 6, 2024

View reviewed changes

jadenPete merged commit d9e701d into lucid-master Dec 6, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Bzlmod #74

Migrate to Bzlmod #74

jadenPete commented Dec 4, 2024 •

edited

Loading

jadenPete Dec 4, 2024

jjudd left a comment

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jjudd Dec 6, 2024

jadenPete Dec 6, 2024

jjudd Dec 6, 2024

jadenPete commented Dec 6, 2024


		http_file = use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl", "http_file")

		http_file(

Migrate to Bzlmod #74

Migrate to Bzlmod #74

Conversation

jadenPete commented Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

jjudd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadenPete commented Dec 6, 2024

jadenPete commented Dec 4, 2024 •

edited

Loading