-
Notifications
You must be signed in to change notification settings - Fork 90
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
Update stardoc for bzlmod #776
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("@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
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.
docs/vfs_overlay.doc_doc.md
Outdated
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
"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
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 stricterdeps
changes.This updates our documentation (i.e.
stardoc
andbzl_library
targets) 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_library
with it's own set ofdeps
. These target names are then used within thestardoc
rule to generate documentation for each of them.The main change is going from one giant
bzl_library
to multiplebzl_library
.Depends on the following stack:
write_file
from Bazel Skylib #778