Update repo name handling for Bzlmod compatibility #1621
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbland
added a commit
to mbland/bazel-skylib
that referenced
this pull request
Oct 14, 2024
Adds the following macros to work with apparent repo names when running under Bzlmod. - `adjust_main_repo_prefix` - `apparent_repo_label_string` - `apparent_repo_name` Originally developed while updating rules_scala to support Bzlmod as part of bazelbuild/rules_scala#1482. For examples of their use, see bazelbuild/rules_scala#1621.
|
And here's the new bazel-skylib pull request: bazelbuild/bazel-skylib#548 |
|
Based on @fmeum's review of bazelbuild/bazel-skylib#548, I'm rethinking all of the problems solved in this PR. I'll ping again once I've tried some different things. |
mbland
added a commit
to mbland/bazel-skylib
that referenced
this pull request
Oct 16, 2024
Repurposed `apparent_repo_name` to only work with `repository_ctx` objects after finding solutions to other repository name related problems in bazelbuild/rules_scala#1621. The one problem I couldn't solve (elegantly) without this function was setting a default target name for a generated repo. Technically, such repos could require an attribute to mirror `name`, but that seems like a terrible user experience.
|
OK, this is ready for review again. I think it's much improved now, thanks to @fmeum's feedback on bazelbuild/bazel-skylib#548. There is one potential compatibility breaking change, noted below. CI is showing as failed, even though it's still only failing on the "optional" Bazel green head build. If you'd like me to break this into separate pull requests, please ask and I'd be happy to do so. Either way, this set of changes addresses all the repository name related breakages under Bzlmod, while remaining Resource paths - breaking changeI've removed my earlier This will break any existing Scala code referencing
|
mbland
changed the title
Part of bazelbuild#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
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.
Backported from bazelbuild/bazel-skylib#548, whereby I realized `Label(//:all)` would not produce the main repo prefix when imported into other repos.
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.
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.
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.
mbland
force-pushed
the
bzlmod-apparent-repo-name
branch
from
4a83d81 to
a4b20f0
Compare
|
Rebased and passing after #1627, including latest Bazel 7.3.2. |
simuons
requested changes
Oct 23, 2024
There was a problem hiding this comment.
Hi, @mbland, thanks again for such a detailed PR.
To be fair that breaking change part was a bit concerning. But honestly current behaviour looks like a mistake to me. Bazel details are leaking into code. So I'm fine with such breaking change. I hope others won't object as well.
All looks good except small comment I made. I'll merge once you will resolve that.
scala/private/macros/bzlmod.bzl
Outdated
| """ | ||
| repo_name = repository_ctx.name | ||
|
|
||
| # Bazed on this pattern from the Bazel source: |
There was a problem hiding this comment.
D'oh! Fixed, thanks! Like I say in the new commit message, I hate sloppy comments, and hate it more when I write them!
Caught by @simuons in bazelbuild#1621, thankfully. I hate sloppy comments, and hate it more when I write them!
simuons
approved these changes
Oct 24, 2024
mbland
added a commit
to mbland/bazel-skylib
that referenced
this pull request
Nov 6, 2024
Adds the following macros to work with apparent repo names when running under Bzlmod. - `adjust_main_repo_prefix` - `apparent_repo_label_string` - `apparent_repo_name` Originally developed while updating rules_scala to support Bzlmod as part of bazelbuild/rules_scala#1482. For examples of their use, see bazelbuild/rules_scala#1621.
mbland
added a commit
to mbland/bazel-skylib
that referenced
this pull request
Nov 6, 2024
Repurposed `apparent_repo_name` to only work with `repository_ctx` objects after finding solutions to other repository name related problems in bazelbuild/rules_scala#1621. The one problem I couldn't solve (elegantly) without this function was setting a default target name for a generated repo. Technically, such repos could require an attribute to mirror `name`, but that seems like a terrible user experience.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Nov 15, 2024
This presents the same API, remains compatible with both `WORKSPACE` and Bzlmod, and avoids relying on the canonical repository name format at all. Part of bazelbuild#1482, and supersedes the addition of `apparent_repo_name` from bazelbuild#1621. Also adds the `_SCALA_IMPORT_RULE_LOAD` constant in `scala/scala_maven_import_external.bzl`, which also removes `@io_bazel_rules_scala` from the `load`ed file path. It now generates the correct path with a `Label` instead. --- With bazelbuild/bazel-skylib#548 going nowhere, I eventually realized a macro wrapper could generate a default target name for a repository_rule by duplicating the `name` attr. This isn't as easy as adding `apparent_repo_name(repository_ctx.name)` to a repo rule's implementation, but it's also not that much more work. One small downside is that the macro can't be imported into a `MODULE.bazel` file directly via `use_repo_rule`, if one wanted to do that. If you did, you'd have to add a module extension to call it, or use `modules.as_extension` from the `bazel_skylib` module. I suppose that's a small price to pay for squashing a canonical repo name format dependency forever. The other small downside may be that the documentation from the original `repository_rule` doesn't automatically convey to the repo wrapper. In this case, `_alias_repository` is already internal, and the original `jvm_import_external` didn't have docstrings to begin with.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Nov 15, 2024
This presents the same API, remains compatible with both `WORKSPACE` and Bzlmod, and avoids relying on the canonical repository name format at all. Part of bazelbuild#1482, and supersedes the addition of `apparent_repo_name` from bazelbuild#1621. Also adds the `_SCALA_IMPORT_RULE_LOAD` constant in `scala/scala_maven_import_external.bzl`, which also removes `@io_bazel_rules_scala` from the `load`ed file path. It now generates the correct path with a `Label` instead. --- The goal is to maintain the ability to generate default target names from a repository name under Bzlmod. While not documented on https://bazel.build, it is documented in the Bazel source itself, even under the current 9.0.0-pre.20241105.2 prerelease: - https://github.com/bazelbuild/bazel/blob/9.0.0-pre.20241105.2/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java#L99 Under `WORKSPACE`, the `repository_ctx.name` value served this purpose directly, as it would reflect the `name` value as provided by the caller. Under Bzlmod, this value now contains the canonical repo name, which is different than that provided by the `name` parameter. The `apparent_repo_name` utility was my first attempt to resolve this, by parsing the original `name` parameter from the canonicalized `repository_ctx.name`. I also created bazelbuild/bazel-skylib#548 to contribute `apparent_repo_name` to `bazel_skylib`, along with other proposed utilities. I quickly found better solutions to obviate the other utilities, but hung onto `apparent_repo_name`. After that pull request stagnated, I eventually realized a macro wrapper could generate a default target name for a repository_rule by duplicating the `name` attr. This isn't as easy as adding `apparent_repo_name(repository_ctx.name)` to a repo rule's implementation, but it's also not that much more work. See also this thread from the #bzlmod channel in the Bazel Slack workspace about generating default repo names: - https://bazelbuild.slack.com/archives/C014RARENH0/p1730909824656159 --- One small downside of this technique is that the macro can't be imported into a `MODULE.bazel` file directly via `use_repo_rule`. If you did want to use it in `MODULE.bazel`, you'd have to write a module extension to call it, or use `modules.as_extension` from `bazel_skylib`. I suppose that's a small price to pay for squashing a canonical repo name format dependency forever. The other small downside may be that the documentation from the original `repository_rule` doesn't automatically convey to the repo wrapper. In this case, `_alias_repository` is already internal, and the original `jvm_import_external` didn't have docstrings to begin with.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Nov 15, 2024
This presents the same API, remains compatible with both `WORKSPACE` and Bzlmod, and avoids relying on the canonical repository name format at all. Part of bazelbuild#1482, and supersedes the addition of `apparent_repo_name` from bazelbuild#1621. Also adds the `_SCALA_IMPORT_RULE_LOAD` constant in `scala/scala_maven_import_external.bzl`, which also removes `@io_bazel_rules_scala` from the `load`ed file path. It now generates the correct path with a `Label` instead. --- The goal is to maintain the ability to generate default target names from a repository name under Bzlmod. While not documented on https://bazel.build, it is documented in the Bazel source itself, even under the current 9.0.0-pre.20241105.2 prerelease: - https://github.com/bazelbuild/bazel/blob/9.0.0-pre.20241105.2/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java#L99 Under `WORKSPACE`, the `repository_ctx.name` value served this purpose directly, as it would reflect the `name` value as provided by the caller. Under Bzlmod, this value now contains the canonical repo name, which is different than that provided by the `name` parameter. The `apparent_repo_name` utility was my first attempt to resolve this, by parsing the original `name` parameter from the canonicalized `repository_ctx.name`. I also created bazelbuild/bazel-skylib#548 to contribute `apparent_repo_name` to `bazel_skylib`, along with other proposed utilities. I quickly found better solutions to obviate the other utilities, but hung onto `apparent_repo_name`. After that pull request stagnated, I eventually realized a macro wrapper could generate a default target name for a repository_rule by duplicating the `name` attr. This isn't as easy as adding `apparent_repo_name(repository_ctx.name)` to a repo rule's implementation, but it's also not that much more work. See also this thread from the #bzlmod channel in the Bazel Slack workspace about generating default repo names: - https://bazelbuild.slack.com/archives/C014RARENH0/p1730909824656159 --- One small downside of this technique is that the macro can't be imported into a `MODULE.bazel` file directly via `use_repo_rule`. If you did want to use it in `MODULE.bazel`, you'd have to write a module extension to call it, or use `modules.as_extension` from `bazel_skylib`. I suppose that's a small price to pay for squashing a canonical repo name format dependency forever. The other small downside may be that the documentation from the original `repository_rule` doesn't automatically convey to the repo wrapper. In this case, `_alias_repository` is already internal, and the original `jvm_import_external` didn't have docstrings to begin with.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Nov 15, 2024
This presents the same API, remains compatible with both `WORKSPACE` and Bzlmod, and avoids relying on the canonical repository name format at all. Part of bazelbuild#1482, and supersedes the addition of `apparent_repo_name` from bazelbuild#1621. Also adds the `_SCALA_IMPORT_RULE_LOAD` constant in `scala/scala_maven_import_external.bzl`, which also removes `@io_bazel_rules_scala` from the `load`ed file path. It now generates the correct path with a `Label` instead. --- The goal is to maintain the ability to generate default target names from a repository name under Bzlmod. While not documented on https://bazel.build, it is documented in the Bazel source itself, even under the current 9.0.0-pre.20241105.2 prerelease: - https://github.com/bazelbuild/bazel/blob/9.0.0-pre.20241105.2/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java#L99 Under `WORKSPACE`, the `repository_ctx.name` value served this purpose directly, as it would reflect the `name` value as provided by the caller. Under Bzlmod, this value now contains the canonical repo name, which is different than that provided by the `name` parameter. The `apparent_repo_name` utility was my first attempt to resolve this, by parsing the original `name` parameter from the canonicalized `repository_ctx.name`. I also created bazelbuild/bazel-skylib#548 to contribute `apparent_repo_name` to `bazel_skylib`, along with other proposed utilities. I quickly found better solutions to obviate the other utilities, but hung onto `apparent_repo_name`. After that pull request stagnated, I eventually realized a macro wrapper could generate a default target name for a repository_rule by duplicating the `name` attr. This isn't as easy as adding `apparent_repo_name(repository_ctx.name)` to a repo rule's implementation, but it's also not that much more work. See also this thread from the #bzlmod channel in the Bazel Slack workspace about generating default repo names: - https://bazelbuild.slack.com/archives/C014RARENH0/p1730909824656159 --- One small downside of this technique is that the macro can't be imported into a `MODULE.bazel` file directly via `use_repo_rule`. If you did want to use it in `MODULE.bazel`, you'd have to write a module extension to call it, or use `modules.as_extension` from `bazel_skylib`. I suppose that's a small price to pay for squashing a canonical repo name format dependency forever. The other small downside may be that the documentation from the original `repository_rule` doesn't automatically convey to the repo wrapper. In this case, `_alias_repository` is already internal, and the original `jvm_import_external` didn't have docstrings to begin with.
liucijus
pushed a commit
that referenced
this pull request
Nov 27, 2024
This presents the same API, remains compatible with both `WORKSPACE` and Bzlmod, and avoids relying on the canonical repository name format at all. Part of #1482, and supersedes the addition of `apparent_repo_name` from #1621. Also adds the `_SCALA_IMPORT_RULE_LOAD` constant in `scala/scala_maven_import_external.bzl`, which also removes `@io_bazel_rules_scala` from the `load`ed file path. It now generates the correct path with a `Label` instead. --- The goal is to maintain the ability to generate default target names from a repository name under Bzlmod. While not documented on https://bazel.build, it is documented in the Bazel source itself, even under the current 9.0.0-pre.20241105.2 prerelease: - https://github.com/bazelbuild/bazel/blob/9.0.0-pre.20241105.2/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java#L99 Under `WORKSPACE`, the `repository_ctx.name` value served this purpose directly, as it would reflect the `name` value as provided by the caller. Under Bzlmod, this value now contains the canonical repo name, which is different than that provided by the `name` parameter. The `apparent_repo_name` utility was my first attempt to resolve this, by parsing the original `name` parameter from the canonicalized `repository_ctx.name`. I also created bazelbuild/bazel-skylib#548 to contribute `apparent_repo_name` to `bazel_skylib`, along with other proposed utilities. I quickly found better solutions to obviate the other utilities, but hung onto `apparent_repo_name`. After that pull request stagnated, I eventually realized a macro wrapper could generate a default target name for a repository_rule by duplicating the `name` attr. This isn't as easy as adding `apparent_repo_name(repository_ctx.name)` to a repo rule's implementation, but it's also not that much more work. See also this thread from the #bzlmod channel in the Bazel Slack workspace about generating default repo names: - https://bazelbuild.slack.com/archives/C014RARENH0/p1730909824656159 --- One small downside of this technique is that the macro can't be imported into a `MODULE.bazel` file directly via `use_repo_rule`. If you did want to use it in `MODULE.bazel`, you'd have to write a module extension to call it, or use `modules.as_extension` from `bazel_skylib`. I suppose that's a small price to pay for squashing a canonical repo name format dependency forever. The other small downside may be that the documentation from the original `repository_rule` doesn't automatically convey to the repo wrapper. In this case, `_alias_repository` is already internal, and the original `jvm_import_external` didn't have docstrings to begin with.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes various repository name related errors when building under Bzlmod, while remaining backwards compatible with
WORKSPACE. See the first commit message for details on the problems solved by this change.This change also includes a couple of other minor touch-ups:
Updated the
runtime_depsattribute inrepositories()to add the Scala version suffix, just likedeps.Added a
fail()message torepositories()to make it more clear which Scala version dictionary is missing an artifact.Removed unnecessary internal uses of the
@io_bazel_rules_scalarepo name, applyingLabel()where necessary.Updated the construction of
dep_providersin_default_dep_providersto remove the repo name, reduce duplication, and make the upcoming toolchain update slightly cleaner.Motivation
Part of adding Bzlmod support per #1482.
I've created bazelbuild/bazel-skylib#548 containing the
apparent_repo_namemacro with full unit tests. If the maintainers accept that change, we can bump our bazel_skylib version to use the macro from there, and remove thebzlmod.bzlfile.