Skip to content

Commit 6f287f2

Browse files
authored
Update repo name handling for Bzlmod compatibility (#1621)
* Add repo name macros for Bzlmod compatibility Part of #1482. These helper macros fix various repository name related errors when building under Bzlmod, while remaining backwards compatible with `WORKSPACE`. Without Bzlmod, these macros return original repo names. With Bzlmod enabled, they avoid the problems described below. I've prepared a change for bazelbuild/bazel-skylib containing these macros with full unit tests. If the maintainers accept that change, we can bump our bazel_skylib version to use the macros from there, and remove the `bzlmod.bzl` file. --- Also includes a couple of other minor touch-ups: - Updated the `runtime_deps` attribute in `repositories()` to add the Scala version suffix, just like `deps`. - Added a `fail()` message to `repositories()` to make it more clear which Scala version dictionary is missing an artifact. - Removed unnecessary internal uses of the `@io_bazel_rules_scala` repo name, applying `Label()` where necessary. - Updated the construction of `dep_providers` in `_default_dep_providers` to remove the repo name, reduce duplication, and make the upcoming toolchain update slightly cleaner. --- Before this change, `repositories()` would originally emit `BUILD` targets including canonical repo names: ```py scala_import( name = "_main~scala_deps~io_bazel_rules_scala_scala_compiler", jars = ["scala-compiler-2.12.18.jar"], ) ``` resulting in errors like: ```txt ERROR: .../_main~_repo_rules~io_bazel_rules_scala/scala/BUILD: no such target '@@_main~scala_deps~io_bazel_rules_scala_scala_compiler//:io_bazel_rules_scala_scala_compiler': target 'io_bazel_rules_scala_scala_compiler' not declared in package '' defined by .../_main~scala_deps~io_bazel_rules_scala_scala_compiler/BUILD and referenced by '@@_main~_repo_rules~io_bazel_rules_scala//scala:default_toolchain_scala_compile_classpath_provider' ``` --- Attaching resources from custom repos to targets under Bzlmod, like in `scalarules.test.resources.ScalaLibResourcesFromExternalDepTest`, would break with: ```txt $ bazel test //test/src/main/scala/scalarules/test/resources:all 1) Scala library depending on resources from external resource-only jar::allow to load resources(scalarules.test.resources.ScalaLibResourcesFromExternalDepTest) java.lang.NullPointerException at scalarules.test.resources.ScalaLibResourcesFromExternalDepTest.get(ScalaLibResourcesFromExternalDepTest.scala:17) at scalarules.test.resources.ScalaLibResourcesFromExternalDepTest.$anonfun$new$3(ScalaLibResourcesFromExternalDepTest.scala:11) at scalarules.test.resources.ScalaLibResourcesFromExternalDepTest.$anonfun$new$2(ScalaLibResourcesFromExternalDepTest.scala:11) ``` `_update_external_target_path` in `resources.bzl` fixes this problem. --- Fixes `test_strict_deps_filter_included_target` from `test/shell/test_strict_dependency.sh` when run under Bzlmod. The `dependency_tracking_strict_deps_patterns` attribute of //test_expect_failure/missing_direct_deps/filtering:plus_one_strict_deps_filter_a_impl contains patterns starting with `@//`. However, in `_phase_dependency()` from `scala/private/phases/phase_dependency.bzl`, these patterns were compared against a stringified Label. Under Bazel < 7.1.0, this works for root target Labels. Under Bazel >= 7.1.0, this breaks for root target Labels under Bzlmod, which start with `@@//`. `adjust_main_repo_prefix` updates the patterns accordingly in `_partition_patterns` from `scala_toolchain.bzl`. `apparent_repo_label_string` makes `_phase_dependency()` more resilient when comparing target Labels against filters containing external apparent repo names. --- Fixes the `alias` targets generated by `_jvm_import_external` from `scala_maven_import_external.bzl` by setting the `target` to the correct apparent repo name. Added `apparent_repo_name(repository_ctx.name)` to `_jvm_import_external` to avoid this familiar error when running `dt_patches/test_dt_patches` tests: ```txt $ bazel build //... ERROR: .../external/_main~compiler_source_repos~scala_reflect/BUILD: no such target '@@_main~compiler_source_repos~scala_reflect//:scala_reflect': target 'scala_reflect' not declared in package '' defined by .../external/_main~compiler_source_repos~scala_reflect/BUILD ERROR: .../dt_patches/test_dt_patches/BUILD:11:22: no such target '@@_main~compiler_source_repos~scala_reflect//:scala_reflect': target 'scala_reflect' not declared in package '' defined by .../external/_main~compiler_source_repos~scala_reflect/BUILD and referenced by '//:dt_scala_toolchain_scala_compile_classpath_provider' ERROR: Analysis of target '//:dt_scala_toolchain_scala_compile_classpath_provider' failed; build aborted: Analysis failed ``` --- As for why we need these macros, we can't rely on hacking the specific canonical repository name format: > Repos generated by extensions have canonical names in the form of > `module_repo_canonical_name+extension_name+repo_name`. Note that the > canonical name format is not an API you should depend on — it's > subject to change at any time. > > - https://bazel.build/external/extension#repository_names_and_visibility The change to no longer encode module versions in canonical repo names in Bazel 7.1.0 is a recent example of Bazel maintainers altering the format: - bazelbuild/bazel#21316 And the maintainers recently replaced the `~` delimiter with `+` in the upcoming Bazel 8 release due to build performance issues on Windows: - bazelbuild/bazel#22865 The core `apparent_repo_name` function assumes the only valid repo name characters are letters, numbers, '_', '-', and '.'. This is valid so long as this condition holds: - https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java#L159-L162 * Bazel 5 updates from bazelbuild/bazel-skylib#548 I don't think we really need them, as I don't think we support Bazel 5. But better to have and not need, I guess. * Fix _MAIN_REPO_PREFIX in bzlmod.bzl Backported from bazelbuild/bazel-skylib#548, whereby I realized `Label(//:all)` would not produce the main repo prefix when imported into other repos. * Expand dependency tracking patterns using Label Replaced `apparent_repo_label_string` and `adjust_main_repo_prefix` based on an idea from @fmeum during his review of bazelbuild/bazel-skylib#548. Added `_expand_patterns`, which uses `native.package_relative_label` to expand the `dependency_tracking_*_deps_patterns` attributes to full, correct `Label` strings. All `test/shell/test_{strict,unused}_dependency.sh` test cases pass. * Fix `scala_toolchains` lint error * Use `apparent_repo_name` only on `repository_ctx` Repurposed `apparent_repo_name` to only work for `repository_ctx` objects, not repository or module names in general. Removed `generated_rule_name` from `repositories.bzl`, since it's no longer necessary. Technically we could eliminate `apparent_repo_name` by making `generated_rule_name` a mandatory attribute of `_jvm_import_external`. However, this feels ultimately clunky and unnecessary. This update to `apparent_repo_name` required removing `_update_external_target_path` and updating `_target_path_by_default_prefixes` to remove `external/<canonical_repo_name>` prefixes. This represents a breaking change for files referencing `external/<repo_name>` paths, but the quick fix is to delete that prefix in the code. This matches the behavior in the same function regarding `resources/` and `java/` prefixes. * Update `_jvm_import_external` JAR alias target Changes the target for the `//jar:jar` alias in `_jvm_import_scala` to the top level repo target directly (`//:%s`) instead of the repo name (`@%s`). Functionally the same, but seems a bit cleaner than referencing the target as though it were external to the repo. * Fix typo in apparent_repo_name comment Caught by @simuons in #1621, thankfully. I hate sloppy comments, and hate it more when I write them!
1 parent 91a45f7 commit 6f287f2

File tree

8 files changed

+92
-26
lines changed

8 files changed

+92
-26
lines changed

scala/private/macros/bzlmod.bzl

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
"""Utilities for working with Bazel modules"""
2+
3+
def apparent_repo_name(repository_ctx):
4+
"""Generates a repository's apparent name from a repository_ctx object.
5+
6+
Args:
7+
repository_ctx: a repository_ctx object
8+
9+
Returns:
10+
An apparent repo name derived from repository_ctx.name
11+
"""
12+
repo_name = repository_ctx.name
13+
14+
# Based on this pattern from the Bazel source:
15+
# com.google.devtools.build.lib.cmdline.RepositoryName.VALID_REPO_NAME
16+
for i in range(len(repo_name) - 1, -1, -1):
17+
c = repo_name[i]
18+
if not (c.isalnum() or c in "_-."):
19+
return repo_name[i + 1:]
20+
21+
return repo_name

scala/private/phases/phase_dependency.bzl

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
# Gathers information about dependency mode and analysis
22

33
load(
4-
"@io_bazel_rules_scala//scala/private:dependency.bzl",
4+
"//scala/private:dependency.bzl",
55
"get_compiler_deps_mode",
66
"get_strict_deps_mode",
77
"new_dependency_info",
88
)
99
load(
10-
"@io_bazel_rules_scala//scala/private:paths.bzl",
10+
"//scala/private:paths.bzl",
1111
_get_files_with_extension = "get_files_with_extension",
1212
_java_extension = "java_extension",
1313
)
@@ -35,7 +35,7 @@ def _phase_dependency(
3535
p,
3636
unused_deps_always_off,
3737
strict_deps_always_off):
38-
toolchain = ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"]
38+
toolchain = ctx.toolchains[Label("//scala:toolchain_type")]
3939

4040
target_label = str(ctx.label)
4141

scala/private/resources.bzl

+7
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,13 @@ def _target_path_by_default_prefixes(resource):
4444
if rel_path:
4545
return rel_path
4646

47+
# Looking inside an external repository. Trim off both the "external/" and
48+
# the repository name components. Especially important under Bzlmod, because
49+
# the canonical repository name may change between versions.
50+
(dir_1, dir_2, rel_path) = path.partition("external/")
51+
if rel_path:
52+
return rel_path[rel_path.index("/"):]
53+
4754
# Both short_path and path have quirks we wish to avoid, in short_path there are times where
4855
# it is prefixed by `../` instead of `external/`. And in .path it will instead return the entire
4956
# bazel-out/... path, which is also wanting to be avoided. So instead, we return the short-path if

scala/scala_maven_import_external.bzl

+6-4
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ the following macros are defined below that utilize jvm_import_external:
3535
- java_import_external - to demonstrate that the original functionality of `java_import_external` stayed intact.
3636
"""
3737

38+
load("//scala/private:macros/bzlmod.bzl", "apparent_repo_name")
3839
load("@bazel_tools//tools/build_defs/repo:utils.bzl", "read_netrc", "read_user_netrc", "use_netrc")
3940

4041
# https://github.com/bazelbuild/bazel/issues/13709#issuecomment-1336699672
@@ -64,19 +65,20 @@ def _jvm_import_external(repository_ctx):
6465
if (repository_ctx.attr.generated_linkable_rule_name and
6566
not repository_ctx.attr.neverlink):
6667
fail("Only use generated_linkable_rule_name if neverlink is set")
67-
name = repository_ctx.attr.generated_rule_name or repository_ctx.name
68+
repo_name = apparent_repo_name(repository_ctx)
69+
name = repository_ctx.attr.generated_rule_name or repo_name
6870
urls = repository_ctx.attr.jar_urls
6971
if repository_ctx.attr.jar_sha256:
7072
print("'jar_sha256' is deprecated. Please use 'artifact_sha256'")
7173
sha = repository_ctx.attr.jar_sha256 or repository_ctx.attr.artifact_sha256
72-
path = repository_ctx.name + ".jar"
74+
path = repo_name + ".jar"
7375
for url in urls:
7476
if url.endswith(".jar"):
7577
path = url[url.rindex("/") + 1:]
7678
break
7779
srcurls = repository_ctx.attr.srcjar_urls
7880
srcsha = repository_ctx.attr.srcjar_sha256
79-
srcpath = repository_ctx.name + "-src.jar" if srcurls else ""
81+
srcpath = repo_name + "-src.jar" if srcurls else ""
8082
coordinates = repository_ctx.attr.coordinates
8183
for url in srcurls:
8284
if url.endswith(".jar"):
@@ -136,7 +138,7 @@ def _jvm_import_external(repository_ctx):
136138
"",
137139
"alias(",
138140
" name = \"jar\",",
139-
" actual = \"@%s\"," % repository_ctx.name,
141+
" actual = \"//:%s\"," % repo_name,
140142
")",
141143
"",
142144
]))

scala/scala_toolchain.bzl

+38-13
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
load(
2-
"@io_bazel_rules_scala//scala:providers.bzl",
3-
_DepsInfo = "DepsInfo",
4-
)
1+
load("//scala:providers.bzl", _DepsInfo = "DepsInfo")
52
load(
63
"@io_bazel_rules_scala_config//:config.bzl",
74
"ENABLE_COMPILER_DEPENDENCY_TRACKING",
@@ -108,17 +105,17 @@ def _scala_toolchain_impl(ctx):
108105

109106
def _default_dep_providers():
110107
dep_providers = [
111-
"@io_bazel_rules_scala//scala:scala_xml_provider",
112-
"@io_bazel_rules_scala//scala:parser_combinators_provider",
113-
"@io_bazel_rules_scala//scala:scala_compile_classpath_provider",
114-
"@io_bazel_rules_scala//scala:scala_library_classpath_provider",
115-
"@io_bazel_rules_scala//scala:scala_macro_classpath_provider",
108+
"scala_xml",
109+
"parser_combinators",
110+
"scala_compile_classpath",
111+
"scala_library_classpath",
112+
"scala_macro_classpath",
116113
]
117-
if SCALA_MAJOR_VERSION.startswith("2"):
118-
dep_providers.append("@io_bazel_rules_scala//scala:semanticdb_provider")
119-
return dep_providers
114+
if SCALA_MAJOR_VERSION.startswith("2."):
115+
dep_providers.append("semanticdb")
116+
return [Label("//scala:%s_provider" % p) for p in dep_providers]
120117

121-
scala_toolchain = rule(
118+
_scala_toolchain = rule(
122119
_scala_toolchain_impl,
123120
attrs = {
124121
"scalacopts": attr.string_list(),
@@ -179,3 +176,31 @@ scala_toolchain = rule(
179176
},
180177
fragments = ["java"],
181178
)
179+
180+
def _expand_patterns(patterns):
181+
"""Expands string patterns to match actual Label values."""
182+
result = []
183+
184+
for p in patterns:
185+
exclude = p.startswith("-")
186+
p = p.lstrip("-")
187+
expanded = str(native.package_relative_label(p)) if p else ""
188+
189+
# If the original pattern doesn't contain ":", match any target
190+
# beginning with the pattern prefix.
191+
if expanded and ":" not in p:
192+
expanded = expanded[:expanded.rindex(":")]
193+
194+
result.append(("-" if exclude else "") + expanded)
195+
196+
return result
197+
198+
def scala_toolchain(**kwargs):
199+
"""Creates a Scala toolchain target."""
200+
strict = kwargs.pop("dependency_tracking_strict_deps_patterns", [""])
201+
unused = kwargs.pop("dependency_tracking_unused_deps_patterns", [""])
202+
_scala_toolchain(
203+
dependency_tracking_strict_deps_patterns = _expand_patterns(strict),
204+
dependency_tracking_unused_deps_patterns = _expand_patterns(unused),
205+
**kwargs
206+
)

test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalDepTest.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ class ScalaLibResourcesFromExternalDepTest extends SpecWithJUnit {
88
"allow to load resources" >> {
99

1010
val expectedString = String.format("A resource%n"); //Using platform dependent newline (%n)
11-
get("/external/test_new_local_repo/resource.txt") must beEqualTo(expectedString)
11+
get("/resource.txt") must beEqualTo(expectedString)
1212

1313
}
1414
}

test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalScalaTest.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ class ScalaLibResourcesFromExternalScalaTest extends AnyFunSuite {
66

77
test("Scala library depending on resources from external resource-only jar should allow to load resources") {
88
val expectedString = String.format("A resource%n"); //Using platform dependent newline (%n)
9-
assert(get("/external/test_new_local_repo/resource.txt") === expectedString)
9+
assert(get("/resource.txt") === expectedString)
1010
}
1111

1212
private def get(s: String): String =

third_party/repositories/repositories.bzl

+15-4
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
load("//scala/private:macros/bzlmod.bzl", "apparent_repo_name")
12
load(
23
"//third_party/repositories:scala_2_11.bzl",
34
_artifacts_2_11 = "artifacts",
@@ -102,14 +103,24 @@ def repositories(
102103
default_artifacts = artifacts_by_major_scala_version[major_scala_version]
103104
artifacts = dict(default_artifacts.items() + overriden_artifacts.items())
104105
for id in for_artifact_ids:
106+
if id not in artifacts:
107+
fail("artifact %s not in third_party/repositories/scala_%s.bzl" % (
108+
id,
109+
major_scala_version.replace(".", "_"),
110+
))
111+
112+
artifact_repo_name = id + suffix
105113
_scala_maven_import_external(
106-
name = id + suffix,
114+
name = artifact_repo_name,
107115
artifact = artifacts[id]["artifact"],
108116
artifact_sha256 = artifacts[id]["sha256"],
109117
licenses = ["notice"],
110118
server_urls = maven_servers,
111119
deps = [dep + suffix for dep in artifacts[id].get("deps", [])],
112-
runtime_deps = artifacts[id].get("runtime_deps", []),
120+
runtime_deps = [
121+
dep + suffix
122+
for dep in artifacts[id].get("runtime_deps", [])
123+
],
113124
testonly_ = artifacts[id].get("testonly", False),
114125
fetch_sources = fetch_sources,
115126
)
@@ -118,13 +129,13 @@ def repositories(
118129
# See: https://github.com/bazelbuild/rules_scala/pull/1573
119130
# Hopefully we can deprecate and remove it one day.
120131
if suffix and scala_version == SCALA_VERSION:
121-
_alias_repository(name = id, target = id + suffix)
132+
_alias_repository(name = id, target = artifact_repo_name)
122133

123134
def _alias_repository_impl(rctx):
124135
""" Builds a repository containing just two aliases to the Scala Maven artifacts in the `target` repository. """
125136

126137
format_kwargs = {
127-
"name": rctx.name,
138+
"name": apparent_repo_name(rctx),
128139
"target": rctx.attr.target,
129140
}
130141
rctx.file("BUILD", """alias(

0 commit comments

Comments
 (0)