-
-
Notifications
You must be signed in to change notification settings - Fork 252
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
Extract utility methods for handling bzlmod mangled names into one place #1224
Conversation
…ace. This allows us to cope with `--incompatible_use_plus_in_repo_names` Closes #23418
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still uneasy about these assumptions being used, but better that they work with +
too than if they don't.
.bazelversion
Outdated
@@ -1 +1 @@ | |||
7.1.0 | |||
7.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use 7.3.1 instead! it fixes some bugs with --incompatible_use_plus_in_repo_names
in particular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
examples/bzlmod/.bazelversion
Outdated
@@ -1 +1 @@ | |||
7.1.0 | |||
7.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
private/rules/jvm_import.bzl
Outdated
# With `bzlmod` enabled, workspace names end up being `~` separated. For the | ||
# user-visible workspace name, we need the final part of the name | ||
visible_name = ctx.label.workspace_name.rpartition("~")[2] | ||
visible_name = to_visible_name(ctx.label.workspace_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeum can this usage of to_visible_name
be avoided? the label is added as part of a string arg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can't comment on the lines below)
-progress_message = "Stamping the manifest of %s" % ctx.label,
+progress_message = "Stamping the manifest of %{label}",
-args.add_all(["--manifest-entry", "Target-Label:{target_label}".format(target_label = label)])
+args.add("--manifest-entry", ctx.label, format = "Target-Label:%s")
Both should give you apparent labels with Bazel 7. They are also more memory efficient as the formatted string isn't retained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Is there a better way of doing this? The two tasks we're trying to accomplish are:
|
private/lib/bzlmod.bzl
Outdated
return name | ||
|
||
def get_file_owner_repo_name(name): | ||
"""Convert a name (typically from `File.owner.workspace_name`) to the owning repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't return the name of a repo, it gives you the name of the module that owns the repo that owns the file.
Note that this can have surprising effects: If my better_repo_rules
module offers a better_http_archive
module extension, then the reported owner of files in such archives will always be better_repo_rules
, not the module that specified tags for the extension.
@Wyverald What do you think of exposing a module_name
field on Label
? That wouldn't require any repo unmapping, which is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the method name and comment.
private/rules/jvm_import.bzl
Outdated
# With `bzlmod` enabled, workspace names end up being `~` separated. For the | ||
# user-visible workspace name, we need the final part of the name | ||
visible_name = ctx.label.workspace_name.rpartition("~")[2] | ||
visible_name = to_visible_name(ctx.label.workspace_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can't comment on the lines below)
-progress_message = "Stamping the manifest of %s" % ctx.label,
+progress_message = "Stamping the manifest of %{label}",
-args.add_all(["--manifest-entry", "Target-Label:{target_label}".format(target_label = label)])
+args.add("--manifest-entry", ctx.label, format = "Target-Label:%s")
Both should give you apparent labels with Bazel 7. They are also more memory efficient as the formatted string isn't retained.
private/rules/maven_project_jar.bzl
Outdated
@@ -20,9 +21,7 @@ def _strip_excluded_workspace_jars(jar_files, excluded_workspaces): | |||
workspace_name = owner.workspace_name | |||
|
|||
# bzlmod module names use ~ as a separator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outdated now.
This allows us to cope with
--incompatible_use_plus_in_repo_names
Closes #1219