Support buildfiles function for external repositories in Bazel query by linzhp · Pull Request #14497 · bazelbuild/bazel

linzhp · 2021-12-28T23:16:24Z

All repository rules are in the package //external, which is different from other packages. Calling Package.getBuildFile() returns the WORKSPACE file and Package.getStarlarkFileDependencies() returns empty. A possible fix is to have the graph keep track of all Starlark files loaded by WORKSPACE, so we can treat the package //external in the same way as other packages during query execution. However, this means that evaluating buildfiles(//external:foo) is the same as evaluating buildfiles(//external:*), which would include Starlark files not related to the repository @foo.

This PR takes a different approach by using the call stack of an external repository to get the precise set of Starlark files needed by //external:foo.

Fixed #14280

aiuto · 2022-01-20T01:53:40Z

@zhengwei143 You've been in this code more than most other people. Can you take a look.

janakdr

Leaving comments on overall design and utility to @zhengwei143, just commenting on performance and bugs.

janakdr · 2022-01-20T15:47:31Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+      List<Label> extensions = new ArrayList<>();
      Package pkg = x.getPackage();
-      if (seenPackages.add(pkg.getPackageIdentifier())) {
+      if (x instanceof Rule && pkg.getPackageIdentifier().getCanonicalForm().equals("//external")) {


Please use LabelConstants#EXTERNAL_PACKAGE_IDENTIFIER. PackageIdentifiers are interned, so this check becomes instant, as opposed to potentially a character-by-character String equality check (depending on how the JVM canonicalizes string constants).

janakdr · 2022-01-20T15:48:38Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+              .toList();
+          for (CallStackEntry entry : callStackEntries) {
+            PathFragment extFile = PathFragment.create(entry.location.file());
+            PathFragment workspaceRoot = pkg.getPackageDirectory().asFragment();


Please pull this out of the for loop so you're not doing the same work on each iteration.

janakdr · 2022-01-20T15:52:09Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+        // External repository rules
+        if (seenRepos.add(x.getName())) {
+          ImmutableList<CallStackEntry> callStackEntries = x.getAssociatedRule().getCallStack()
+              .toList();


nit: if this is a performance issue, could provide an iterator from CallStack, since you don't need anything else about the immutable list here.

janakdr · 2022-01-20T15:52:54Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+          for (CallStackEntry entry : callStackEntries) {
+            PathFragment extFile = PathFragment.create(entry.location.file());
+            PathFragment workspaceRoot = pkg.getPackageDirectory().asFragment();
+            if (extFile.startsWith(workspaceRoot)) {


when does this check fail?

If the extension file is in an external repo, then this check will fail. For example, if we have this in WORKSPACE:

load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies") go_rules_dependencies()

Then the location of the extension file for the target //external:platforms (which is one of the repos loaded by go_rule_dependencies()) will be in somewhere like /private/var/tmp/_bazel_zplin/db67431f618bf23018777556a2edc2eb/external/io_bazel_rules_go/go/private/repositories.bzl, which is outside the workspace root.

Wouldn't that external bzl file be a valid result if buildfiles returned it, though? I can imagine a user being surprised that only bzl files in the main repository are returned.

janakdr · 2022-01-20T15:53:14Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+          ImmutableList<CallStackEntry> callStackEntries = x.getAssociatedRule().getCallStack()
+              .toList();
+          for (CallStackEntry entry : callStackEntries) {
+            PathFragment extFile = PathFragment.create(entry.location.file());


couldn't there be duplicates here? Please add a test that covers this situation (the bzl file calls another function in the same file)

janakdr · 2022-01-20T15:53:53Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+            if (extFile.startsWith(workspaceRoot)) {
+              try {
+                Label extension = Label.create(
+                    extFile.relativeTo(workspaceRoot).getParentDirectory().getPathString(),


is this construction true? A bzl file can be in a subdirectory of a package. Please add a test for this situation.

janakdr · 2022-01-20T15:56:07Z

src/test/py/bazel/query_test.py

+      '        name = "io_bazel_rules_go",',
+      '        sha256 = "2b1641428dff9018f9e85c0384f03ec6c10660d935b750e3fa1492a281a53b0f",',
+      '        urls = [',
+      '            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.29.0/rules_go-v0.29.0.zip",',


Please change test to not require network access.

janakdr · 2022-01-20T16:05:20Z

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

+      List<Label> extensions = new ArrayList<>();
      Package pkg = x.getPackage();
-      if (seenPackages.add(pkg.getPackageIdentifier())) {
+      if (x instanceof Rule && pkg.getPackageIdentifier().getCanonicalForm().equals("//external")) {


This will still be incomplete if you have a non-Rule target in a Package, right? You can get a situation with exports_files([...]) or just if the query restricts the output to source files or output files. The package will still be loaded and require those bzl files to work correctly (and since buildfiles is often used to check if a package needs to be reloaded, that's important), but those bzl files won't be listed here. That makes me think you're doing this at a fundamentally wrong place, although I don't have a better one to suggest since I'm not familiar with external repos. In any case, please add a test for these situations: querying for a source file exported via exports_files([...]); querying for an output file (the output of a genrule, not the genrule itself); and a query that filters for source files (kind('source_file',...)).

This is a special case for //external. The only non-rule target there is WORKSPACE. For example, from Bazel's own repo:

bazel query 'kind("source file", //external:all-targets)' //external:WORKSPACE

For genrule and exports_files in external repos, their labels are like @some_repo//:data.txt, which is not in the //external package, and out of scope of this pull request. I can write tests to cover them, but their behavior in buildfiles is not affected by this pull request.

Oh, I see. Thanks, you can disregard this comment in that case.

linzhp · 2022-01-24T00:09:34Z

That makes me think you're doing this at a fundamentally wrong place

@janakdr I started to think that you may be right about this. So I implement another approach in #14630. Can you take a look?

linzhp · 2022-01-24T04:46:33Z

Closing in favor of #14630

This is an alternative approach to fix #14280. It adds transitive closure of Starlark dependencies to `//external` package when loading `WORKSPACE` file, so it can be processed in the same way as `BUILD` files during query execution. Comparing to the approach taken in #14497, this approach is less intrusive, but not able to distinguish the extension files needed by `//external:foo` and `//external:bar`, meaning `buildfiles(//external:foo)` returns the same result as `buildfiles(//external:*)`. However, this behavior is consistent with other packages. For example, `buildfiles(//foo:bar)` has the same result as `buildfiles(//foo:*)`. Closes #14630. PiperOrigin-RevId: 424092916

This is an alternative approach to fix bazelbuild#14280. It adds transitive closure of Starlark dependencies to `//external` package when loading `WORKSPACE` file, so it can be processed in the same way as `BUILD` files during query execution. Comparing to the approach taken in bazelbuild#14497, this approach is less intrusive, but not able to distinguish the extension files needed by `//external:foo` and `//external:bar`, meaning `buildfiles(//external:foo)` returns the same result as `buildfiles(//external:*)`. However, this behavior is consistent with other packages. For example, `buildfiles(//foo:bar)` has the same result as `buildfiles(//foo:*)`. Closes bazelbuild#14630. PiperOrigin-RevId: 424092916 (cherry picked from commit a6c3f23)

* Adding Starlark dependencies to the package //external This is an alternative approach to fix #14280. It adds transitive closure of Starlark dependencies to `//external` package when loading `WORKSPACE` file, so it can be processed in the same way as `BUILD` files during query execution. Comparing to the approach taken in #14497, this approach is less intrusive, but not able to distinguish the extension files needed by `//external:foo` and `//external:bar`, meaning `buildfiles(//external:foo)` returns the same result as `buildfiles(//external:*)`. However, this behavior is consistent with other packages. For example, `buildfiles(//foo:bar)` has the same result as `buildfiles(//foo:*)`. Closes #14630. PiperOrigin-RevId: 424092916 (cherry picked from commit a6c3f23) * Avoid bazel_module_test timeout on macos https://buildkite.com/bazel/bazel-bazel/builds/17785#9a1fb564-c6f9-4e69-ac8f-87c422a002b0 By setting the test size to "large". RELNOTES:None PiperOrigin-RevId: 409114345 (cherry picked from commit 1d99391) Co-authored-by: Zhongpeng Lin <zplin@uber.com> Co-authored-by: pcloudy <pcloudy@google.com>

Support buildfiles function for external repositories in Bazel query

755953b

aiuto requested a review from zhengwei143 January 20, 2022 01:52

aiuto assigned zhengwei143 Jan 20, 2022

aiuto requested a review from janakdr January 20, 2022 01:53

janakdr reviewed Jan 20, 2022

View reviewed changes

linzhp mentioned this pull request Jan 23, 2022

Adding Starlark dependencies to the package //external #14630

Closed

linzhp marked this pull request as draft January 24, 2022 00:05

linzhp closed this Jan 24, 2022

linzhp deleted the buildfiles branch January 25, 2022 18:10

brentleyjones mentioned this pull request Mar 7, 2022

[5.1] Adding Starlark dependencies to the package //external #14991

Merged

Conversation

linzhp commented Dec 28, 2021

Uh oh!

aiuto commented Jan 20, 2022

Uh oh!

janakdr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linzhp Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linzhp commented Jan 24, 2022

Uh oh!

linzhp commented Jan 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linzhp Jan 22, 2022 •

edited

Loading