Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace apparent_repo_name with repo rule wrappers #1650

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Nov 15, 2024

Description

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 loaded file path. It now generates the correct path with a Label instead.

Motivation

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.

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.

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.

Regarding _SCALA_IMPORT_RULE_LOAD, on the one hand, it eliminates string constant duplication. On the other, it's part of my effort to opportunistically remove or replace internal uses of @io_bazel_rules_scala. This will allow future Bzlmod users to import rules_scala without having to set repo_name = "io_bazel_rules_scala" in the bazel_dep call.

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 liucijus merged commit d35448f into bazelbuild:master Nov 27, 2024
2 checks passed
@mbland mbland deleted the bzlmod-replace-apparent-repo-name-with-repo-rule-wrappers branch November 27, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants