Conversation
|
@jacky8hyf Do you want to address the failing tests before we review? |
I don't think the test error sare related to my change. The errors are:
I can't reproduce the same error offline. |
|
@aiuto Any thoughts on the test failures? |
|
gentle ping |
|
I do think the failing tests are unrelated, but I am dubious about the change. |
|
AFAIK there are no new dependencies. See this line: Line 15 in 5c6aec6 This means for anyone that relies on the In addition, skylib is already a non-dev dependency: Line 11 in 5c6aec6 (Skylib does depend on rules_pkg as a dev-dependency, on the other hand: I am not changing MODULE.bazel for rules_pkg so there are no dependency changes. Am I missing something? |
| direct = [] | ||
| transitive = [] | ||
| for src in ctx.attr.srcs: | ||
| if StarlarkLibraryInfo in src: |
There was a problem hiding this comment.
Now I am trying to remember why this was important. Is removing that strictly needed?
There was a problem hiding this comment.
No; in fact I can drop this commit from this particular pull request.
I just find that it is incorrect here because
transitive.append(src[StarlarkLibraryInfo])
This is appending the StarlarkLibraryInfo, not the inner list of files, to the transitive list, causing the depset creation below to fail when there is a bzl_library/starlark_library in srcs because of unmatching types. I found this bug when I was trying to add rules_python/skylib to //pkg:bzl_srcs directly. But later I didn't actually add rules_python/skylib to //pkg:bzl_srcs.
Do you want me to drop this commit from this pull request and create a separate pull request?
There was a problem hiding this comment.
In other words, if I were to use starlark_library instead of bzl_library for the newly added //pkg:rules_pkg_lib target, then I would need this patch (or a variation of it that makes sure the StarlarkLibraryInfo doesn't go into the depset directly).
That being said, I think @bazel_skylib//lib:paths and @rules_python//python:defs_bzl are both bzl_librarys, and due to this:
It is actually okay to just use the default files from these libraries without poking into StarlarkLibraryInfo, so this contional branch could be deleted, as I am doing in this patch.
Though, a pedantic person may also argue that, for this to be 100% correct, we should use the fields in StarlarkLibraryInfo from dependencies to construct StarlarkLibraryInfo of this starlark_library...
There was a problem hiding this comment.
Based on bazelbuild/bazel#18391 (comment)
it seems we can drop the use of starlarklibrary and bzl_library.
It seem to work for me by changing pkg/BUILD:bzl_srcs to a filegroup(name ="rules_pkg_lib"
and referring to that in doc_build/BUILD, deleting the existing rules_pkg_lib there.
There was a problem hiding this comment.
changing pkg/BUILD:bzl_srcs to a filegroup(name ="rules_pkg_lib"
bzl_srcs has visibility public. Wouldn't changing it to a filegroup breaks an API?
I can push a change that does this.
There was a problem hiding this comment.
Also, as I said in the original commit message:
This is because the
//doc_build package is not loadable when rules_pkg itself
is not the root Bazel module, because stardoc is a
dev_dependency.
So how should users of rules_pkg build stardoc for a .bzl that relies on a .bzl from rules_pkg? It seems to me that the bzl_library must be outside of //docs_build. I choose //pkg because it is next to the .bzl files, which is what the other projects usually do as well, e.g. rules_cc does this:
https://github.com/bazelbuild/rules_cc/blob/34f0e1f038cff9bf28d65d101c806b48f35bf92e/cc/BUILD#L85
https://github.com/bazelbuild/rules_cc/blob/34f0e1f038cff9bf28d65d101c806b48f35bf92e/docs/BUILD#L30
(combining with your comment in #897 (comment) ) If I understand correctly, are you planning to make |
|
Actually, I don't think it would be possible for rules_pkg to depend on skylib as a dev_dependency, because of this usage here: Line 30 in 5c6aec6 ... unless you planning to fork your own |
This change updates deps for //pkg:bzl_srcs to include paths and rules_python, which comes from the individual rules. This bzl_library is next to the .bzl files. Users of rules_pkg may build stardoc for their own extensions by relying on this bzl_library directly. This change deletes the unnecessary starlark_library rule, because starlark_library itself already creates the non-dev dependency from rules_pkg to skylib. A user of rules_pkg will have to have skylib anyways. So let's just use skylib's bzl_library directly. It is okay to put non .bzl files in bzl_library. Test: locally update latest.md, no significant difference other than small changes due to the file being outdated in the first place. Link: bazelbuild#897
jacky8hyf
force-pushed
the
pkg_install_doc
branch
from
6929bf6 to
a6a12e9
Compare
|
ping, any updates? I pushed a revision a few days ago. Would someone be able to take a look and comment? Thanks. |
3 participants
... next to the .bzl files. This is because the
//doc_build package is not loadable when rules_pkg itself
is not the root Bazel module, because stardoc is a
dev_dependency. So, //doc_build:rules_pkg_lib is not
usable by clients of rules_pkg.
Move the target to //pkg, next to the .bzl files.
Also add @rules_python//python:defs_bzl used by pkg_install.
Link: #897