Add render_main_repo_name attribute to starlark_doc_extract#19009
Add render_main_repo_name attribute to starlark_doc_extract#19009tetromino wants to merge 3 commits intobazelbuild:masterfrom
render_main_repo_name attribute to starlark_doc_extract#19009Conversation
|
I think that the current PR state will not result in the correct labels being emitted for modules that specify a custom With this patch applied: diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
index 11cf20b1aa..d87f8351a1 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
@@ -957,7 +957,8 @@ public final class StarlarkDocExtractTest extends BuildViewTestCase {
public void labels_whenStarlarkDocExtractInMainBzlmodModule_respectRenderMainRepoNameAttr()
throws Exception {
setBuildLanguageOptions("--enable_bzlmod");
- scratch.overwriteFile("MODULE.bazel", "module(name = 'my_module')");
+ scratch.overwriteFile("MODULE.bazel", "module(name = 'my_module', repo_name = " +
+ "'my_module_name')");
scratch.file(
"foo.bzl", //
"my_rule = rule(",the test fails with: Here, |
For labels in the main repo, users may wish one of 2 behaviors: * render labels in display form (without the repo component) - when documenting rules intended for other users of the same repo. (This is the expected use case in an organization's internal monorepo). * render labels with the @repo component included - when documenting rules intended for use from other repos. (This is the expected use case for public rule sets.) Formerly, starlark_doc_extract only supported case 1. We add support for case 2 via the new `render_main_repo_name` attribute. Supporting it means we can't simply use Label.getDisplayForm() any more - we need to post-process the string via a new label renderer. It also turned out that Label.getDisplayForm() fails an assert when using a repo map owned by the non-main repo. This means we always have to use the main repo's repo map for rendering, which in turn means that starlark_doc_extract's label stringification is different depending on whether the starlark_doc_extract target lives in the main repo or a dependency. Arguably, this is the correct thing to do: the owner of the main repo presumably wants to generate documentation with consistent repo names regardless of whether the target lives in the main repo or in a dep.
The change to RepositoryMapping addresses a strange corner case: RepositoryMappingFunction.compute returns the bzlmod repo mapping and calls withAdditionalMappings() on it to add a mapping from the legacy workspace name to the main repo; the old implementation of withAdditionalMappings() changed key iteration order, putting new mapping ahead of existing ones, and the legacy name took precedence over the expected MODULE.bazel name. We obviously don't want that.
Interesting point. I thought that behavior is what we want - e.g. https://github.com/bazelbuild/rules_go/blob/master/README.rst uses the repo name ("io_bazel_rules_go"), not the module name everywhere, so I think we ought to have "io_bazel_rules_go" in Stardoc output too for consistency. |
|
No, the module rules_go uses The role of Edit: The rules_go README is still written from the WORKSPACE perspective. But the guide at https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/bzlmod.md uses the new name. |
That is a good point. I've switched to preferring the main module's name instead. |
For labels in the main repo, users may wish one of 2 behaviors:
Formerly, starlark_doc_extract only supported case 1. We add support for case 2 via the new
render_main_repo_nameattribute. Supporting it means we can't simply use Label.getDisplayForm() any more - we need to post-process the string via a new label renderer.It also turned out that Label.getDisplayForm() fails an assert when using a repo map owned by the non-main repo. This means we always have to use the main repo's repo map for rendering, which in turn means that starlark_doc_extract's label stringification is different depending on whether the starlark_doc_extract target lives in the main repo or a dependency. Arguably, this is the correct thing to do: the owner of the main repo presumably wants to generate documentation with consistent repo names regardless of whether the target lives in the main repo or in a dep.
Inspired by @fmeum's work in #19002