Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Move rules_pkg_lib to //pkg#898

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
jacky8hyf wants to merge1 commit intobazelbuild:main
base:main
Choose a base branch
Loading
fromjacky8hyf:pkg_install_doc

Conversation

@jacky8hyf
Copy link
Contributor

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

@cgrindel
Copy link
Collaborator

@jacky8hyf Do you want to address the failing tests before we review?

@jacky8hyf
Copy link
ContributorAuthor

@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:

 ERROR: no such package '@@[unknown repo 'mappings_test_external_repo' requested from @@]//pkg': The repository '@@[unknown repo 'mappings_test_external_repo' requested from @@]' could not be resolved: No repository visible as '@mappings_test_external_repo' from main repository. Was the repository introduced in WORKSPACE? The WORKSPACE file is disabled by default in Bazel 8 (late 2024) and will be removed in Bazel 9 (late 2025), please migrate to Bzlmod. See https://bazel.build/external/migration.

mappings_test_external_repo is defined in WORKSPACE file. Perhaps--noenable_workspace is set because it is using Bazel 8: (8.0.0-pre.20240911.1 from the test).

I can't reproduce the same error offline.

@cgrindel
Copy link
Collaborator

@aiuto Any thoughts on the test failures?

@jacky8hyf
Copy link
ContributorAuthor

gentle ping

@aiuto
Copy link
Collaborator

I do think the failing tests are unrelated, but I am dubious about the change.
We are trying to limit taking on new dependencies. Skylib depends on rules_pkg, so the mutual embrace makes compatibility_level updates really hard.

@jacky8hyf
Copy link
ContributorAuthor

AFAIK there are nonew dependencies. See this line:

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

This means for anyone that relies on the@rules_pkg//pkg package, thestarlark_library.bzl is loaded, which relies on skylib because of

load("@bazel_skylib//:bzl_library.bzl","StarlarkLibraryInfo")

In addition, skylib is already anon-dev dependency:

bazel_dep(name="bazel_skylib",version="1.4.2")

(Skylib does depend on rules_pkg as a dev-dependency, on the other hand:
https://github.com/bazelbuild/bazel-skylib/blob/e8e9d218ff563f93e3a7f7547ea532e66b210620/MODULE.bazel#L20
)

I am not changing MODULE.bazel for rules_pkg so there are no dependency changes. Am I missing something?

direct= []
transitive= []
forsrcinctx.attr.srcs:
ifStarlarkLibraryInfoinsrc:
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
ContributorAuthor

@jacky8hyfjacky8hyfOct 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 theStarlarkLibraryInfo, 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 insrcs 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
ContributorAuthor

@jacky8hyfjacky8hyfOct 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 usestarlark_library instead ofbzl_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 bothbzl_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 onbazelbuild/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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

@jacky8hyf
Copy link
ContributorAuthor

I do think the failing tests are unrelated, but I am dubious about the change. We are trying to limit taking on new dependencies. Skylib depends on rules_pkg, so the mutual embrace makes compatibility_level updates really hard.

(combining with your comment in#897 (comment) )

If I understand correctly, are you planning to makerules_pkg depend onskylib with adev_dependency = True in the future? Right now it isn't, and this change is not making the problem worse.

@jacky8hyf
Copy link
ContributorAuthor

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:

load("@bazel_skylib//lib:paths.bzl","paths")

... unless you planning to fork your ownpaths implementation in rules_pkg...

This change updates deps for //pkg:bzl_srcs to include paths andrules_python, which comes from the individual rules.This bzl_library is next to the .bzl files. Users of rules_pkg may buildstardoc for their own extensions by relying on this bzl_librarydirectly.This change deletes the unnecessary starlark_library rule, becausestarlark_library itself already creates the non-dev dependency fromrules_pkg to skylib. A user of rules_pkg will have to have skylibanyways. So let's just use skylib's bzl_library directly. It is okayto 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
Copy link
ContributorAuthor

jacky8hyf commentedDec 17, 2024•
edited
Loading

ping, any updates? I pushed a revision a few days ago. Would someone be able to take a look and comment? Thanks.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@aiutoaiutoaiuto left review comments

@jylinv0jylinv0Awaiting requested review from jylinv0

@cgrindelcgrindelAwaiting requested review from cgrindelcgrindel is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jacky8hyf@cgrindel@aiuto
[8]ページ先頭
©2009-2025 Movatter.jp