Move rules_pkg_lib to //pkg #898
Conversation
It is incorrect to put a struct in a depset of files. However, we never use this case, so just delete the branch. Signed-off-by: HONG Yifan <[email protected]>
... 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: bazelbuild#897 Signed-off-by: HONG Yifan <[email protected]>
|
@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? |
| @@ -9,13 +9,10 @@ def _make_starlark_library(ctx): | |||
| 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.
... 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