Add support for Bzlmod #141
Conversation
fmeum
force-pushed
the
support-bzlmod
branch
from
6af3cdb
to
d09735f
Compare
fmeum
force-pushed
the
support-bzlmod
branch
from
d09735f
to
8d01c6a
Compare
|
@comius Should be ready for review, the failing test seems to require updating the stardoc parts in Bazel first. |
comius
requested review from
comius
and removed request for
tetromino and
brandjon
|
@comius I pushed a new commit to fix the buildifier warnings. |
|
@comius Friendly ping. Tests are expected to fail until we carry out bazelbuild/bazel#16775 (comment). |
|
Friendly ping. This is a blocker for downstream projects to fully migrate away from WORKSPACEs. |
|
@fmeum - am I correct in understanding that this PR requires using a Stardoc jar built after merging bazelbuild/bazel#16775 or similar? And without changing the doc extractor, we cannot get support for bzlmod? |
|
@tetromino That's right. I outlined a process that I think should keep CI green at any time at bazelbuild/bazel#16775 (comment), but it's quite possible I missed something or there is a simpler way. |
stardoc/private/stardoc.bzl
Outdated
| files = [ctx.file.input], | ||
| transitive_files = depset(transitive = [ | ||
| dep[StarlarkLibraryInfo].transitive_srcs | ||
| for dep in ctx.attr.deps |
There was a problem hiding this comment.
Looking at the implementation of bzl_library, it seems it is already returning transitive files within its runfiles. It would be nicer if you could just drop this additional rule and target and add input,deps directly stardoc_runfiles target. Is there anything preventing that?
There was a problem hiding this comment.
Thanks for the suggestion, that is indeed much nicer. Removed the rule.
There was a problem hiding this comment.
Also removed the target, please take a look as I may have missed an edge case in the data = [input] + deps logic.
fmeum
force-pushed
the
support-bzlmod
branch
3 times, most recently
from
901ed85
to
7689bf2
Compare
|
I pushed a TMP commit that should make the pipeline green again, please ignore it during import. |
Before Stardoc implementation was tested using stardoc.bzl from Stardoc repository. This makes it hard to change the implementation and bzl file at the same time. The change is preparation to import #16775 and bazelbuild/stardoc#141. Without it we'd need to do a half-release of Stardoc PR to make Bazel's CI pass. There's still a dependency on Stardoc repo/module from Bazel, which is used to generate the docs. PiperOrigin-RevId: 525736214 Change-Id: I291838a9dd9bbb9482a47011e6e36fc6dd67eff1
Input .bzl files are turned into runfiles of a custom Stardoc binary, which allows reusing the Java runfiles library to load the files while taking repository mappings into account.
|
I copied over the entire list of Maven deps from Bazel. It is long, but my attempts at doing this piece by piece resulted in a seemingly never-ending game of whack-a-mole. |
|
The buildifier failure is due to bazelbuild/buildtools#1098, which is unrelated (except that I caused it, sorry :-)) |
I don't mind using bzlmod by default, but I'm not sure it helps. You'll need bazel, which is not on BCR. Adding it seems the easiest (cc @meteorcloudy @Wyverald ), but not sure if there are any objections to that. Depending on Bazel via bzlmod via repo rules probably has the same problem. Is it possible to depend on slightly older commit or disable some transitive tests to just get skydoc? |
|
We could get it via a The commit with the changes for Stardoc landed after the switch to rules_jvm_external, so I don't see a way to depend on an older commit. I could manually prune the list based on |
This reverts commit bf6632e.
|
@tetromino this is now ready for review and/or merge |
Adding Bazel to BCR is a bit weird since Bazel is not exactly a library people should depend on. (Granted, it's hella weird that Stardoc depends on Bazel, but that ship sailed before I even knew what Bazel was.) I think we should probably just do the git_override plus patch thing. And hope @tetromino's rewrite ships soon (lol). |
|
Yeah, the prod version of Stardoc doesn't depend on Bazel, so even if we want to check in Stardoc in BCR, we don't need Bazel there. |
There was a problem hiding this comment.
Thank you for this work. It's a hack which raises several questions, but every time upon careful reading of the code, one comes to the conclusion that this really is the least bad solution available at the moment :)
I regenerated the bundled jars on an internal machine - I hope you do not mind.
|
@tetromino I found a small issue. I will fix it and submit a PR with a proper BCR test and publishing setup. |
We need to ignore the _stardoc java_binary outputs introduced by bazelbuild#141.
In preparation for switching to starlark_doc_extract, we need to fix Stardoc tests to use .bzl files that can really be loaded by Bazel. This means: * getting rid of misuses of aspect api * getting rid of old Android and Java symbols * getting rid of generated_bzl_test entirely, since Bazel cannot load dynamically generated .bzl modules. Also fix update-stardoc-tests.sh script - we need to ignore the _stardoc java_binary outputs introduced by #141.
Before Stardoc implementation was tested using stardoc.bzl from Stardoc repository. This makes it hard to change the implementation and bzl file at the same time. The change is preparation to import bazelbuild#16775 and bazelbuild/stardoc#141. Without it we'd need to do a half-release of Stardoc PR to make Bazel's CI pass. There's still a dependency on Stardoc repo/module from Bazel, which is used to generate the docs. PiperOrigin-RevId: 525736214 Change-Id: I291838a9dd9bbb9482a47011e6e36fc6dd67eff1
Input .bzl files are turned into runfiles of a custom Stardoc binary, which allows reusing the Java runfiles library to load the files while taking repository mappings into account.