Add repo name macros for Bzlmod compatibility

mbland · mbland · commit 2c076f213a29 · 2024-10-21T10:49:10.000-04:00
Part of bazel-contrib#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
diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl
@@ -0,0 +1,71 @@
+"""Utilities for working with Bazel modules"""
+
+def apparent_repo_name(label_or_name):
+    """Return a repository's apparent repository name.
+
+    Can be replaced with a future bazel-skylib implementation, if accepted into
+    that repo.
+
+    Args:
+        label_or_name: a Label or repository name string
+
+    Returns:
+        The apparent repository name
+    """
+    repo_name = getattr(label_or_name, "repo_name", label_or_name).lstrip("@")
+    delimiter_indices = []
+
+    # Bazed on this pattern from the Bazel source:
+    # com.google.devtools.build.lib.cmdline.RepositoryName.VALID_REPO_NAME
+    for i in range(len(repo_name)):
+        c = repo_name[i]
+        if not (c.isalnum() or c in "_-."):
+            delimiter_indices.append(i)
+
+    if len(delimiter_indices) == 0:
+        # Already an apparent repo name, apparently.
+        return repo_name
+
+    if len(delimiter_indices) == 1:
+        # The name is for a top level module, possibly containing a version ID.
+        return repo_name[:delimiter_indices[0]]
+
+    return repo_name[delimiter_indices[-1] + 1:]
+
+def apparent_repo_label_string(label):
+    """Return a Label string starting with its apparent repo name.
+
+    Args:
+        label: a Label instance
+
+    Returns:
+        str(label) with its canonical repository name replaced with its apparent
+            repository name
+    """
+    if len(label.repo_name) == 0:
+        return str(label)
+
+    label_str = "@" + str(label).lstrip("@")
+    return label_str.replace(label.repo_name, apparent_repo_name(label))
+
+_MAIN_REPO_PREFIX = str(Label("@@//:all")).split(":")[0]
+
+def adjust_main_repo_prefix(target_pattern):
+    """Updates the main repo prefix to match the current Bazel version.
+
+    The main repo prefix will be "@//" for Bazel < 7.1.0, and "@@//" for Bazel
+    >= 7.1.0 under Bzlmod. This macro automatically updates strings representing
+    include/exclude target patterns so that they match actual main repository
+    target Labels correctly.
+
+    Args:
+        target_pattern: a string used to match a BUILD target pattern
+
+    Returns:
+        the string with any main repository prefix updated to match the current
+            Bazel version
+    """
+    if target_pattern.startswith("@//") or target_pattern.startswith("@@//"):
+        return _MAIN_REPO_PREFIX + target_pattern.lstrip("@/")
+
+    return target_pattern
diff --git a/scala/private/phases/phase_dependency.bzl b/scala/private/phases/phase_dependency.bzl
@@ -1,13 +1,14 @@
 # Gathers information about dependency mode and analysis
 
 load(
-    "@io_bazel_rules_scala//scala/private:dependency.bzl",
+    "//scala/private:dependency.bzl",
     "get_compiler_deps_mode",
     "get_strict_deps_mode",
     "new_dependency_info",
 )
+load("//scala/private:macros/bzlmod.bzl", "apparent_repo_label_string")
 load(
-    "@io_bazel_rules_scala//scala/private:paths.bzl",
+    "//scala/private:paths.bzl",
     _get_files_with_extension = "get_files_with_extension",
     _java_extension = "java_extension",
 )
@@ -35,9 +36,9 @@ def _phase_dependency(
         p,
         unused_deps_always_off,
         strict_deps_always_off):
-    toolchain = ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"]
+    toolchain = ctx.toolchains[Label("//scala:toolchain_type")]
 
-    target_label = str(ctx.label)
+    target_label = apparent_repo_label_string(ctx.label)
 
     included_in_strict_deps_analysis = _is_target_included(
         target_label,
diff --git a/scala/private/resources.bzl b/scala/private/resources.bzl
@@ -1,3 +1,5 @@
+load(":macros/bzlmod.bzl", "apparent_repo_name")
+
 def paths(resources, resource_strip_prefix):
     """Return a list of path tuples (target, source) where:
         target - is a path in the archive (with given prefix stripped off)
@@ -13,7 +15,13 @@ def paths(resources, resource_strip_prefix):
 
 def _target_path(resource, resource_strip_prefix):
     path = _target_path_by_strip_prefix(resource, resource_strip_prefix) if resource_strip_prefix else _target_path_by_default_prefixes(resource)
-    return _strip_prefix(path, "/")
+    return _update_external_target_path(_strip_prefix(path, "/"))
+
+def _update_external_target_path(target_path):
+    if not target_path.startswith("external/"):
+        return target_path
+    prefix, repo_name, rest = target_path.split("/")
+    return "/".join([prefix, apparent_repo_name(repo_name), rest])
 
 def _target_path_by_strip_prefix(resource, resource_strip_prefix):
     # Start from absolute resource path and then strip roots so we get to correct short path
diff --git a/scala/scala_maven_import_external.bzl b/scala/scala_maven_import_external.bzl
@@ -35,6 +35,7 @@ the following macros are defined below that utilize jvm_import_external:
 - java_import_external - to demonstrate that the original functionality of `java_import_external` stayed intact.
 """
 
+load("//scala/private:macros/bzlmod.bzl", "apparent_repo_name")
 load("@bazel_tools//tools/build_defs/repo:utils.bzl", "read_netrc", "read_user_netrc", "use_netrc")
 
 # https://github.com/bazelbuild/bazel/issues/13709#issuecomment-1336699672
@@ -64,7 +65,10 @@ def _jvm_import_external(repository_ctx):
     if (repository_ctx.attr.generated_linkable_rule_name and
         not repository_ctx.attr.neverlink):
         fail("Only use generated_linkable_rule_name if neverlink is set")
-    name = repository_ctx.attr.generated_rule_name or repository_ctx.name
+    name = (
+        repository_ctx.attr.generated_rule_name or
+        apparent_repo_name(repository_ctx.name)
+    )
     urls = repository_ctx.attr.jar_urls
     if repository_ctx.attr.jar_sha256:
         print("'jar_sha256' is deprecated. Please use 'artifact_sha256'")
@@ -136,7 +140,7 @@ def _jvm_import_external(repository_ctx):
         "",
         "alias(",
         "    name = \"jar\",",
-        "    actual = \"@%s\"," % repository_ctx.name,
+        "    actual = \"@%s\"," % apparent_repo_name(repository_ctx.name),
         ")",
         "",
     ]))
diff --git a/scala/scala_toolchain.bzl b/scala/scala_toolchain.bzl
@@ -1,7 +1,5 @@
-load(
-    "@io_bazel_rules_scala//scala:providers.bzl",
-    _DepsInfo = "DepsInfo",
-)
+load("//scala/private:macros/bzlmod.bzl", "adjust_main_repo_prefix")
+load("//scala:providers.bzl", _DepsInfo = "DepsInfo")
 load(
     "@io_bazel_rules_scala_config//:config.bzl",
     "ENABLE_COMPILER_DEPENDENCY_TRACKING",
@@ -31,12 +29,12 @@ def _compute_dependency_tracking_method(
 
 def _partition_patterns(patterns):
     includes = [
-        pattern
+        adjust_main_repo_prefix(pattern)
         for pattern in patterns
         if not pattern.startswith("-")
     ]
     excludes = [
-        pattern.lstrip("-")
+        adjust_main_repo_prefix(pattern.lstrip("-"))
         for pattern in patterns
         if pattern.startswith("-")
     ]
@@ -108,15 +106,15 @@ def _scala_toolchain_impl(ctx):
 
 def _default_dep_providers():
     dep_providers = [
-        "@io_bazel_rules_scala//scala:scala_xml_provider",
-        "@io_bazel_rules_scala//scala:parser_combinators_provider",
-        "@io_bazel_rules_scala//scala:scala_compile_classpath_provider",
-        "@io_bazel_rules_scala//scala:scala_library_classpath_provider",
-        "@io_bazel_rules_scala//scala:scala_macro_classpath_provider",
+        "scala_xml",
+        "parser_combinators",
+        "scala_compile_classpath",
+        "scala_library_classpath",
+        "scala_macro_classpath",
     ]
-    if SCALA_MAJOR_VERSION.startswith("2"):
-        dep_providers.append("@io_bazel_rules_scala//scala:semanticdb_provider")
-    return dep_providers
+    if SCALA_MAJOR_VERSION.startswith("2."):
+        dep_providers.append("semanticdb")
+    return [Label("//scala:%s_provider" % p) for p in dep_providers]
 
 scala_toolchain = rule(
     _scala_toolchain_impl,
diff --git a/third_party/repositories/repositories.bzl b/third_party/repositories/repositories.bzl
@@ -1,3 +1,4 @@
+load("//scala/private:macros/bzlmod.bzl", "apparent_repo_name")
 load(
     "//third_party/repositories:scala_2_11.bzl",
     _artifacts_2_11 = "artifacts",
@@ -102,14 +103,25 @@ def repositories(
     default_artifacts = artifacts_by_major_scala_version[major_scala_version]
     artifacts = dict(default_artifacts.items() + overriden_artifacts.items())
     for id in for_artifact_ids:
+        if id not in artifacts:
+            fail("artifact %s not in third_party/repositories/scala_%s.bzl" % (
+                id,
+                major_scala_version.replace(".", "_"),
+            ))
+
+        generated_rule_name = apparent_repo_name(id) + suffix
         _scala_maven_import_external(
             name = id + suffix,
+            generated_rule_name = generated_rule_name,
             artifact = artifacts[id]["artifact"],
             artifact_sha256 = artifacts[id]["sha256"],
             licenses = ["notice"],
             server_urls = maven_servers,
             deps = [dep + suffix for dep in artifacts[id].get("deps", [])],
-            runtime_deps = artifacts[id].get("runtime_deps", []),
+            runtime_deps = [
+                dep + suffix
+                for dep in artifacts[id].get("runtime_deps", [])
+            ],
             testonly_ = artifacts[id].get("testonly", False),
             fetch_sources = fetch_sources,
         )
@@ -118,13 +130,13 @@ def repositories(
         # See: https://github.com/bazelbuild/rules_scala/pull/1573
         # Hopefully we can deprecate and remove it one day.
         if suffix and scala_version == SCALA_VERSION:
-            _alias_repository(name = id, target = id + suffix)
+            _alias_repository(name = id, target = generated_rule_name)
 
 def _alias_repository_impl(rctx):
     """ Builds a repository containing just two aliases to the Scala Maven artifacts in the `target` repository. """
 
     format_kwargs = {
-        "name": rctx.name,
+        "name": apparent_repo_name(rctx.name),
         "target": rctx.attr.target,
     }
     rctx.file("BUILD", """alias(
