Skip to content

Commit

Permalink
Use apparent_repo_name only on repository_ctx
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mbland committed Oct 16, 2024
1 parent 6bcedbf commit d579e66
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 47 deletions.
32 changes: 8 additions & 24 deletions scala/private/macros/bzlmod.bzl
Original file line number Diff line number Diff line change
@@ -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
17 changes: 8 additions & 9 deletions scala/private/resources.bzl
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 5 additions & 7 deletions scala/scala_maven_import_external.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Expand Down Expand Up @@ -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,
")",
"",
]))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
9 changes: 4 additions & 5 deletions third_party/repositories/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand All @@ -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(
Expand Down

0 comments on commit d579e66

Please sign in to comment.