From d579e6632a15e5f06c500511ab18b5d4a3d06e35 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Tue, 15 Oct 2024 17:12:49 -0400 Subject: [PATCH] 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/` prefixes. This represents a breaking change for files referencing `external/` 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. --- scala/private/macros/bzlmod.bzl | 32 +++++-------------- scala/private/resources.bzl | 17 +++++----- scala/scala_maven_import_external.bzl | 12 +++---- ...ScalaLibResourcesFromExternalDepTest.scala | 2 +- ...alaLibResourcesFromExternalScalaTest.scala | 2 +- third_party/repositories/repositories.bzl | 9 +++--- 6 files changed, 27 insertions(+), 47 deletions(-) diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl index 1816a2aa7..1541170f2 100644 --- a/scala/private/macros/bzlmod.bzl +++ b/scala/private/macros/bzlmod.bzl @@ -1,37 +1,21 @@ """Utilities for working with Bazel modules""" -_repo_attr = ( - "repo_name" if hasattr(Label("//:all"), "repo_name") else "workspace_name" -) - -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. +def apparent_repo_name(repository_ctx): + """Generates a repository's apparent name from a repository_ctx object. Args: - label_or_name: a Label or repository name string + repository_ctx: a repository_ctx object Returns: - The apparent repository name + An apparent repo name derived from repository_ctx.name """ - repo_name = getattr(label_or_name, _repo_attr, label_or_name).lstrip("@") - delimiter_indices = [] + repo_name = repository_ctx.name # 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)): + for i in range(len(repo_name) - 1, -1, -1): 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[i + 1:] - return repo_name[delimiter_indices[-1] + 1:] + return repo_name diff --git a/scala/private/resources.bzl b/scala/private/resources.bzl index 086cd5a82..eaaf12bcb 100644 --- a/scala/private/resources.bzl +++ b/scala/private/resources.bzl @@ -1,5 +1,3 @@ -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) @@ -15,13 +13,7 @@ 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 _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]) + return _strip_prefix(path, "/") 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 @@ -52,6 +44,13 @@ def _target_path_by_default_prefixes(resource): if rel_path: return rel_path + # Looking inside an external repository. Trim off both the "external/" and + # the repository name components. Especially important under Bzlmod, because + # the canonical repository name may change between versions. + (dir_1, dir_2, rel_path) = path.partition("external/") + if rel_path: + return rel_path[rel_path.index("/"):] + # Both short_path and path have quirks we wish to avoid, in short_path there are times where # it is prefixed by `../` instead of `external/`. And in .path it will instead return the entire # bazel-out/... path, which is also wanting to be avoided. So instead, we return the short-path if diff --git a/scala/scala_maven_import_external.bzl b/scala/scala_maven_import_external.bzl index fb1d5c090..a6e65a0c6 100644 --- a/scala/scala_maven_import_external.bzl +++ b/scala/scala_maven_import_external.bzl @@ -65,22 +65,20 @@ 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 - apparent_repo_name(repository_ctx.name) - ) + repo_name = apparent_repo_name(repository_ctx) + name = repository_ctx.attr.generated_rule_name or repo_name urls = repository_ctx.attr.jar_urls if repository_ctx.attr.jar_sha256: print("'jar_sha256' is deprecated. Please use 'artifact_sha256'") sha = repository_ctx.attr.jar_sha256 or repository_ctx.attr.artifact_sha256 - path = repository_ctx.name + ".jar" + path = repo_name + ".jar" for url in urls: if url.endswith(".jar"): path = url[url.rindex("/") + 1:] break srcurls = repository_ctx.attr.srcjar_urls srcsha = repository_ctx.attr.srcjar_sha256 - srcpath = repository_ctx.name + "-src.jar" if srcurls else "" + srcpath = repo_name + "-src.jar" if srcurls else "" coordinates = repository_ctx.attr.coordinates for url in srcurls: if url.endswith(".jar"): @@ -140,7 +138,7 @@ def _jvm_import_external(repository_ctx): "", "alias(", " name = \"jar\",", - " actual = \"@%s\"," % apparent_repo_name(repository_ctx.name), + " actual = \"@%s\"," % repo_name, ")", "", ])) diff --git a/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalDepTest.scala b/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalDepTest.scala index 10e4564f0..088ec71a3 100644 --- a/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalDepTest.scala +++ b/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalDepTest.scala @@ -8,7 +8,7 @@ class ScalaLibResourcesFromExternalDepTest extends SpecWithJUnit { "allow to load resources" >> { val expectedString = String.format("A resource%n"); //Using platform dependent newline (%n) - get("/external/test_new_local_repo/resource.txt") must beEqualTo(expectedString) + get("/resource.txt") must beEqualTo(expectedString) } } diff --git a/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalScalaTest.scala b/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalScalaTest.scala index 99a9e8e7f..599ca7edf 100644 --- a/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalScalaTest.scala +++ b/test/src/main/scala/scalarules/test/resources/ScalaLibResourcesFromExternalScalaTest.scala @@ -6,7 +6,7 @@ class ScalaLibResourcesFromExternalScalaTest extends AnyFunSuite { test("Scala library depending on resources from external resource-only jar should allow to load resources") { val expectedString = String.format("A resource%n"); //Using platform dependent newline (%n) - assert(get("/external/test_new_local_repo/resource.txt") === expectedString) + assert(get("/resource.txt") === expectedString) } private def get(s: String): String = diff --git a/third_party/repositories/repositories.bzl b/third_party/repositories/repositories.bzl index 505f9fb48..b9e66a8ab 100644 --- a/third_party/repositories/repositories.bzl +++ b/third_party/repositories/repositories.bzl @@ -102,10 +102,9 @@ def repositories( major_scala_version.replace(".", "_"), )) - generated_rule_name = apparent_repo_name(id) + suffix + artifact_repo_name = id + suffix _scala_maven_import_external( - name = id + suffix, - generated_rule_name = generated_rule_name, + name = artifact_repo_name, artifact = artifacts[id]["artifact"], artifact_sha256 = artifacts[id]["sha256"], licenses = ["notice"], @@ -123,13 +122,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 = generated_rule_name) + _alias_repository(name = id, target = artifact_repo_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": apparent_repo_name(rctx.name), + "name": apparent_repo_name(rctx), "target": rctx.attr.target, } rctx.file("BUILD", """alias(