-
Notifications
You must be signed in to change notification settings - Fork 94
Update stardoc for bzlmod #776
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
Conversation
|
I plan to PR two things before this PR is merged but that it depends on:
|
3075874 to
c5eb90b
Compare
55a090a to
8c87c5d
Compare
8c87c5d to
52c657a
Compare
| load("@bazel_skylib//lib:selects.bzl", "selects") | ||
| load("@bazel_skylib//rules:write_file.bzl", "write_file") | ||
| load("@build_bazel_rules_apple//apple:apple.bzl", "apple_dynamic_framework_import", "apple_static_framework_import") | ||
| load("@build_bazel_rules_apple//apple/internal:apple_framework_import.bzl", "apple_dynamic_framework_import", "apple_static_framework_import") |
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.
Changing from apple.bzl umbrella import to specific internal import works around an issue i reported in stardoc + bzlmod: bazelbuild/stardoc#192
| _maybe( | ||
| http_archive, | ||
| name = "io_bazel_stardoc", | ||
| sha256 = "62bd2e60216b7a6fec3ac79341aa201e0956477e7c8f6ccc286f279ad1d96432", | ||
| urls = [ | ||
| "https://mirror.bazel.build/github.com/bazelbuild/stardoc/releases/download/0.6.2/stardoc-0.6.2.tar.gz", | ||
| "https://github.com/bazelbuild/stardoc/releases/download/0.6.2/stardoc-0.6.2.tar.gz", | ||
| ], | ||
| ) |
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.
Moved this from WORKSPACE to the dev_dependencies section so that it matches the other rules_ios deps.
| load("@rules_jvm_external//:repositories.bzl", "rules_jvm_external_deps") | ||
|
|
||
| rules_jvm_external_deps() | ||
|
|
||
| load("@rules_jvm_external//:setup.bzl", "rules_jvm_external_setup") | ||
|
|
||
| rules_jvm_external_setup() | ||
|
|
||
| load("@io_bazel_stardoc//:deps.bzl", "stardoc_external_deps") | ||
|
|
||
| stardoc_external_deps() | ||
|
|
||
| load("@stardoc_maven//:defs.bzl", stardoc_pinned_maven_install = "pinned_maven_install") | ||
|
|
||
| stardoc_pinned_maven_install() |
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 required by new stardoc versions
| workspace(name = "build_bazel_rules_ios") | ||
|
|
||
| # TODO: stardoc doesn't currently support bzlmod: https://github.com/bazelbuild/stardoc/issues/117 | ||
| # Once that closes we should depend on it via the WORKSPACE file. | ||
| # In the meantime, this is a workaround to at least allow building. | ||
| # Note however that updating docs with bzlmod will not work until the above is resolved. | ||
| # Until then use: --noenable_bzlmod | ||
| load( | ||
| "@bazel_tools//tools/build_defs/repo:git.bzl", | ||
| "git_repository", | ||
| ) | ||
|
|
||
| git_repository( | ||
| name = "io_bazel_stardoc", | ||
| commit = "6f274e903009158504a9d9130d7f7d5f3e9421ed", | ||
| remote = "https://github.com/bazelbuild/stardoc.git", | ||
| shallow_since = "1667581897 -0400", | ||
| ) | ||
|
|
||
| load("@io_bazel_stardoc//:setup.bzl", "stardoc_repositories") | ||
|
|
||
| stardoc_repositories() |
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.
Now fully bzlmod 🎉
52c657a to
f18dc96
Compare
f18dc96 to
890aa9a
Compare
justinseanmartin
left a comment
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.
Take comments with a grain of salt and nothing that I'd consider a blocker to landing this.
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.
The file name here and xcconfig.doc_doc.md seem odd
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.
Just fixed this, should be generated correctly now
| @@ -0,0 +1,10 @@ | |||
| load( | |||
| "//rules/library:xcconfig.bzl", | |||
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.
Should this live under rules/library/xcconfig.doc.bzl to be next to its associated rule? I presume we don't want all of //rules/library:xcconfig.bzl to be considered public API, so we're cherry-picking a subset of it here? Should we mark the rest as internal symbols with an underscore prefix or something, if that prevents it from being documented?
Same feedback for vfs_overlay.
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.
The use of .doc.bzl matches what rules_apple does, it's exporting the public api of xcconfig.bzl in the //rules package specifically. This makes it so we can create the stardoc target for these in one list vs. conditionalizing the path to the bzl_library
See this in docs/BUILD.bazel:
stardoc(
...
input = "//rules:%s.bzl" % file,
)
61e7273 to
34ac987
Compare
34ac987 to
412a316
Compare
| "apple_patched", | ||
| "extension", | ||
| "features", | ||
| "force_load_direct_deps", |
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 number of the files are internal ones we don't consider upholding an API contract for or have quality starlark doc comments about said internal components; though if other people really want to have this I'm not opposed - nor do I think it will change the lack of some API contract beyond apple_framework
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.
Features seems useful and imo more of the public API than some of the other things we've said are.
Do you have a specific list you'd want me to remove?
412a316 to
4fdb438
Compare
thiagohmcruz
left a comment
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.
Thx so much for tackling this!
Co-authored-by: Thiago Cruz <thiago.hmcruz@gmail.com>
This PR updates stardoc for use with
bzlmod. This is required as the versions of stardoc on the registry require these stricterdepschanges.This updates our documentation (i.e.
stardocandbzl_librarytargets) to work in a similar way to rules_apple: https://github.com/bazelbuild/rules_apple/blob/master/apple/BUILD#L33Each file we want documentation generated for defines it's own
bzl_librarywith it's own set ofdeps. These target names are then used within thestardocrule to generate documentation for each of them.The main change is going from one giant
bzl_libraryto multiplebzl_library.Depends on the following stack:
write_filefrom Bazel Skylib #778