From 2c076f213a296a2d5f4921dd1ff1d1d35341471f Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Sun, 6 Oct 2024 20:37:52 -0400 Subject: [PATCH 1/8] Add repo name macros for Bzlmod compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: - https://github.com/bazelbuild/bazel/pull/21316 And the maintainers recently replaced the `~` delimiter with `+` in the upcoming Bazel 8 release due to build performance issues on Windows: - https://github.com/bazelbuild/bazel/issues/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 --- scala/private/macros/bzlmod.bzl | 71 +++++++++++++++++++++++ scala/private/phases/phase_dependency.bzl | 9 +-- scala/private/resources.bzl | 10 +++- scala/scala_maven_import_external.bzl | 8 ++- scala/scala_toolchain.bzl | 26 ++++----- third_party/repositories/repositories.bzl | 18 +++++- 6 files changed, 118 insertions(+), 24 deletions(-) create mode 100644 scala/private/macros/bzlmod.bzl diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl new file mode 100644 index 000000000..55470a71d --- /dev/null +++ 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 index d8ae91e7e..a8963bf28 100644 --- 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 index 1db14a822..086cd5a82 100644 --- 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 index e69388ced..fb1d5c090 100644 --- 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 index a09e03f73..eea248eef 100644 --- 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 index 88239c012..b446cb798 100644 --- 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( From 2047e58d0d4e43292f1505bde812f1b33abcd182 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Mon, 14 Oct 2024 18:18:12 -0400 Subject: [PATCH 2/8] 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. --- scala/private/macros/bzlmod.bzl | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl index 55470a71d..30868dd32 100644 --- a/scala/private/macros/bzlmod.bzl +++ b/scala/private/macros/bzlmod.bzl @@ -1,5 +1,23 @@ """Utilities for working with Bazel modules""" +def _repo_name(label_or_name): + """Utility to provide Label compatibility with Bazel 5. + + Under Bazel 5, calls `Label.workspace_name`. Otherwise calls + `Label.repo_name`. + + Args: + label_or_name: a Label or repository name string + + Returns: + The repository name returned directly from the Label API, or the + original string if not a Label + """ + if hasattr(label_or_name, "repo_name"): + return label_or_name.repo_name + + return getattr(label_or_name, "workspace_name", label_or_name) + def apparent_repo_name(label_or_name): """Return a repository's apparent repository name. @@ -12,7 +30,7 @@ def apparent_repo_name(label_or_name): Returns: The apparent repository name """ - repo_name = getattr(label_or_name, "repo_name", label_or_name).lstrip("@") + repo_name = _repo_name(label_or_name).lstrip("@") delimiter_indices = [] # Bazed on this pattern from the Bazel source: @@ -42,13 +60,14 @@ def apparent_repo_label_string(label): str(label) with its canonical repository name replaced with its apparent repository name """ - if len(label.repo_name) == 0: + repo_name = _repo_name(label) + if len(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] +_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. From 97341744719eba0a01913de7940431c51c82aa6e Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Mon, 14 Oct 2024 20:54:13 -0400 Subject: [PATCH 3/8] 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. --- scala/private/macros/bzlmod.bzl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl index 30868dd32..918df1544 100644 --- a/scala/private/macros/bzlmod.bzl +++ b/scala/private/macros/bzlmod.bzl @@ -67,7 +67,9 @@ def apparent_repo_label_string(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] +_IS_BZLMOD_ENABLED = str(Label("//:all")).startswith("@@") + +_MAIN_REPO_PREFIX = "@@//" if _IS_BZLMOD_ENABLED else "@//" def adjust_main_repo_prefix(target_pattern): """Updates the main repo prefix to match the current Bazel version. From 9c6f0049e5ff45c741b9dad334d07e5ca6e107c1 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Tue, 15 Oct 2024 15:59:12 -0400 Subject: [PATCH 4/8] 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. --- scala/private/macros/bzlmod.bzl | 63 ++--------------------- scala/private/phases/phase_dependency.bzl | 3 +- scala/scala_toolchain.bzl | 35 +++++++++++-- 3 files changed, 36 insertions(+), 65 deletions(-) diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl index 918df1544..1816a2aa7 100644 --- a/scala/private/macros/bzlmod.bzl +++ b/scala/private/macros/bzlmod.bzl @@ -1,22 +1,8 @@ """Utilities for working with Bazel modules""" -def _repo_name(label_or_name): - """Utility to provide Label compatibility with Bazel 5. - - Under Bazel 5, calls `Label.workspace_name`. Otherwise calls - `Label.repo_name`. - - Args: - label_or_name: a Label or repository name string - - Returns: - The repository name returned directly from the Label API, or the - original string if not a Label - """ - if hasattr(label_or_name, "repo_name"): - return label_or_name.repo_name - - return getattr(label_or_name, "workspace_name", label_or_name) +_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. @@ -30,7 +16,7 @@ def apparent_repo_name(label_or_name): Returns: The apparent repository name """ - repo_name = _repo_name(label_or_name).lstrip("@") + repo_name = getattr(label_or_name, _repo_attr, label_or_name).lstrip("@") delimiter_indices = [] # Bazed on this pattern from the Bazel source: @@ -49,44 +35,3 @@ def apparent_repo_name(label_or_name): 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 - """ - repo_name = _repo_name(label) - if len(repo_name) == 0: - return str(label) - - label_str = "@" + str(label).lstrip("@") - return label_str.replace(label.repo_name, apparent_repo_name(label)) - -_IS_BZLMOD_ENABLED = str(Label("//:all")).startswith("@@") - -_MAIN_REPO_PREFIX = "@@//" if _IS_BZLMOD_ENABLED else "@//" - -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 index a8963bf28..ccf629270 100644 --- a/scala/private/phases/phase_dependency.bzl +++ b/scala/private/phases/phase_dependency.bzl @@ -6,7 +6,6 @@ load( "get_strict_deps_mode", "new_dependency_info", ) -load("//scala/private:macros/bzlmod.bzl", "apparent_repo_label_string") load( "//scala/private:paths.bzl", _get_files_with_extension = "get_files_with_extension", @@ -38,7 +37,7 @@ def _phase_dependency( strict_deps_always_off): toolchain = ctx.toolchains[Label("//scala:toolchain_type")] - target_label = apparent_repo_label_string(ctx.label) + target_label = str(ctx.label) included_in_strict_deps_analysis = _is_target_included( target_label, diff --git a/scala/scala_toolchain.bzl b/scala/scala_toolchain.bzl index eea248eef..aba46631f 100644 --- a/scala/scala_toolchain.bzl +++ b/scala/scala_toolchain.bzl @@ -1,4 +1,3 @@ -load("//scala/private:macros/bzlmod.bzl", "adjust_main_repo_prefix") load("//scala:providers.bzl", _DepsInfo = "DepsInfo") load( "@io_bazel_rules_scala_config//:config.bzl", @@ -29,12 +28,12 @@ def _compute_dependency_tracking_method( def _partition_patterns(patterns): includes = [ - adjust_main_repo_prefix(pattern) + pattern for pattern in patterns if not pattern.startswith("-") ] excludes = [ - adjust_main_repo_prefix(pattern.lstrip("-")) + pattern.lstrip("-") for pattern in patterns if pattern.startswith("-") ] @@ -116,7 +115,7 @@ def _default_dep_providers(): dep_providers.append("semanticdb") return [Label("//scala:%s_provider" % p) for p in dep_providers] -scala_toolchain = rule( +_scala_toolchain = rule( _scala_toolchain_impl, attrs = { "scalacopts": attr.string_list(), @@ -177,3 +176,31 @@ scala_toolchain = rule( }, fragments = ["java"], ) + +def _expand_patterns(patterns): + """Expands string patterns to match actual Label values.""" + result = [] + + for p in patterns: + exclude = p.startswith("-") + p = p.lstrip("-") + expanded = str(native.package_relative_label(p)) if p else "" + + # If the original pattern doesn't contain ":", match any target + # beginning with the pattern prefix. + if expanded and ":" not in p: + expanded = expanded[:expanded.rindex(":")] + + result.append(("-" if exclude else "") + expanded) + + return result + +def scala_toolchain(**kwargs): + """Creates a Scala toolchain target.""" + strict = kwargs.pop("dependency_tracking_strict_deps_patterns", [""]) + unused = kwargs.pop("dependency_tracking_unused_deps_patterns", [""]) + _scala_toolchain( + dependency_tracking_strict_deps_patterns = _expand_patterns(strict), + dependency_tracking_unused_deps_patterns = _expand_patterns(unused), + **kwargs, + ) From 02f3605288f3f23c1f847dd7ffbd7d188891ccd0 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Tue, 15 Oct 2024 16:23:45 -0400 Subject: [PATCH 5/8] Fix `scala_toolchains` lint error --- scala/scala_toolchain.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scala/scala_toolchain.bzl b/scala/scala_toolchain.bzl index aba46631f..60da0783a 100644 --- a/scala/scala_toolchain.bzl +++ b/scala/scala_toolchain.bzl @@ -202,5 +202,5 @@ def scala_toolchain(**kwargs): _scala_toolchain( dependency_tracking_strict_deps_patterns = _expand_patterns(strict), dependency_tracking_unused_deps_patterns = _expand_patterns(unused), - **kwargs, + **kwargs ) From a3d5165e90c985dc5208273993c45d4fbb592c62 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Tue, 15 Oct 2024 17:12:49 -0400 Subject: [PATCH 6/8] 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 b446cb798..1e0529c1e 100644 --- a/third_party/repositories/repositories.bzl +++ b/third_party/repositories/repositories.bzl @@ -109,10 +109,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"], @@ -130,13 +129,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( From a4b20f0f83560bdc6a05741ff37e6106ed12adcb Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 16 Oct 2024 12:03:04 -0400 Subject: [PATCH 7/8] 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. --- scala/scala_maven_import_external.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scala/scala_maven_import_external.bzl b/scala/scala_maven_import_external.bzl index a6e65a0c6..1a7bf8145 100644 --- a/scala/scala_maven_import_external.bzl +++ b/scala/scala_maven_import_external.bzl @@ -138,7 +138,7 @@ def _jvm_import_external(repository_ctx): "", "alias(", " name = \"jar\",", - " actual = \"@%s\"," % repo_name, + " actual = \"//:%s\"," % repo_name, ")", "", ])) From f6dbab1aa9604b022e0f00c500c4e3af177e1f97 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 23 Oct 2024 09:24:26 -0400 Subject: [PATCH 8/8] 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! --- scala/private/macros/bzlmod.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scala/private/macros/bzlmod.bzl b/scala/private/macros/bzlmod.bzl index 1541170f2..db8e8385f 100644 --- a/scala/private/macros/bzlmod.bzl +++ b/scala/private/macros/bzlmod.bzl @@ -11,7 +11,7 @@ def apparent_repo_name(repository_ctx): """ repo_name = repository_ctx.name - # Bazed on this pattern from the Bazel source: + # Based on this pattern from the Bazel source: # com.google.devtools.build.lib.cmdline.RepositoryName.VALID_REPO_NAME for i in range(len(repo_name) - 1, -1, -1): c = repo_name[i]