Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move rules_pkg_lib to //pkg #898

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Move rules_pkg_lib to //pkg #898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c0b6dd
starlark_library does not use StarlarkLibraryInfo from srcs.
Sep 30, 2024
6929bf6
Move rules_pkg_lib to //pkg
Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions doc_build/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ How to:
git commit -m 'update docs' docs/latest.md
"""

load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("@rules_python//python:defs.bzl", "py_binary")
load("@stardoc//stardoc:stardoc.bzl", "stardoc")
load("//:version.bzl", "version")
Expand Down Expand Up @@ -111,14 +110,9 @@ stardoc(
],
)

# gather all rules that should be documented
bzl_library(
alias(
name = "rules_pkg_lib",
srcs = [
"//:version.bzl",
"//pkg:bzl_srcs",
"@bazel_skylib//lib:paths",
],
actual = "//pkg:rules_pkg_lib",
visibility = ["//visibility:public"],
)

Expand Down
13 changes: 13 additions & 0 deletions pkg/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("@rules_pkg//pkg/private:make_starlark_library.bzl", "starlark_library")

# -*- coding: utf-8 -*-
Expand Down Expand Up @@ -65,6 +66,18 @@ starlark_library(
visibility = ["//visibility:public"],
)

# gather all rules that should be documented
bzl_library(
name = "rules_pkg_lib",
srcs = [
"//:version.bzl",
"//pkg:bzl_srcs",
"@bazel_skylib//lib:paths",
"@rules_python//python:defs_bzl",
],
visibility = ["//visibility:public"],
)

# Used by pkg_rpm in rpm.bzl.
py_binary(
name = "make_rpm",
Expand Down
11 changes: 4 additions & 7 deletions pkg/private/make_starlark_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,10 @@ def _make_starlark_library(ctx):
direct = []
transitive = []
for src in ctx.attr.srcs:
if StarlarkLibraryInfo in src:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am trying to remember why this was important. Is removing that strictly needed?

Copy link
Contributor Author

@jacky8hyf jacky8hyf Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jacky8hyf jacky8hyf Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

https://github.com/bazelbuild/bazel-skylib/blob/56b235e700ddd6a15b7d9fa1803fa7a84048471e/rules/private/bzl_library.bzl#L37

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...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

transitive.append(src[StarlarkLibraryInfo])
else:
for file in src[DefaultInfo].files.to_list():
if file.path.endswith(".bzl"):
# print(file.path)
direct.append(file)
for file in src[DefaultInfo].files.to_list():
if file.path.endswith(".bzl"):
# print(file.path)
direct.append(file)
all_files = depset(direct, transitive = transitive)
return [
DefaultInfo(files = all_files, runfiles = ctx.runfiles(transitive_files = all_files)),
Expand Down