-
Notifications
You must be signed in to change notification settings - Fork 189
feat: generate doc_extract targets #571
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
base: main
Are you sure you want to change the base?
Conversation
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.
A PR like this would create a lot of additional targets. There's a cost connected with each additional target. Such feature would need to be disabled at Google.
A lot of automatically created targets are potentially unused. Usually to generate documentation of one rules_* repo, you need a couple of starlar_doc_extract rules, while the number of bzl_libraries is much larger.
What do you think of the alternative: making it possible to write an aspect that invokes starlark_doc_extract actions? If someone worked on such a PR to bazelbuild/bazel do you think it might be accepted? |
Allows bzl_library targets to produce documentation without needing to load stardoc or add additional targets. Note, it would be better to do this with an aspect that visits the bzl_library graph, but the starlark_doc_extract rule is in Bazel java core, and so we cannot spawn actions using its Java code. It needs a main() entry point. Use a config_setting to make this opt-in, so we don't generate so many targets all the time.
Label("//:extract_docs_flag"): hasattr(native, "starlark_doc_extract"), | ||
"//conditions:default": False, | ||
}) | ||
if enable_doc_extract: |
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.
selects don't work in a macro
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.
Yeah oops, evaluation is deferred until after the loading phase, at which point both branches were taken and the targets were created anyway. I didn't manage to test this yesterday since it was hard to back-port to 1.7.1.
(Aside: could we get a release of this module? It has been nearly a year)
I don't fully understand how to make such an aspect possible without incurring tech-debt. What's the intent? Automatically extracting all the docs? I think we have some support in query to do that. cc @tetromino |
Yes I'm experimenting with a pipeline to generate docs, similar to readthedocs. @rickeylev and I discussed in a rules authors SIG meeting. https://github.com/alexeagle/doc.bzl is the prototype. |
Another intent here is merely to ensure that rulesets have correct As I'm visiting the 720 modules on BCR I find that many are missing deps, as one example aspect-build/rules_swc#305 You might say those repos ought to include more testing to avoid such a defect, but this is extra burden on those maintainers. It's akin to |
Another approach would be to let starlark_doc_extract take a bzl_library as input? Or multiple files as inputs. If memory serves, it isn't possible today because of the combination of bzl_library not returning a single output, and starlark_doc_extract requiring a single_file_only input. In rules_python, I worked around this by having a custom rule take a bzl_library as input and produce a single output to then feed into starlark_doc_extract, and just forcing the convention of every bzl_library having a single srcs. |
What I'm looking for in this PR is just a more ergonomic way to run the action behind |
Allows bzl_library targets to produce documentation without needing to load stardoc or add additional targets. Note, it would be better to do this with an aspect that visits the bzl_library graph, but the starlark_doc_extract rule is in Bazel java core, and so we cannot spawn actions using its Java code. It needs a main() entry point.
Fixes #568