Skip to content

Commit 7937dd1

Browse files
brentleyjoneslinzhpmeteorcloudy
authored
[5.1] Adding Starlark dependencies to the package //external (bazelbuild#14991)
* Adding Starlark dependencies to the package //external 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) * 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>
1 parent 78f0311 commit 7937dd1

5 files changed

Lines changed: 101 additions & 2 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/packages/Package.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1358,7 +1358,7 @@ FailureDetail getFailureDetail() {
13581358
return null;
13591359
}
13601360

1361-
Builder setStarlarkFileDependencies(ImmutableList<Label> starlarkFileDependencies) {
1361+
public Builder setStarlarkFileDependencies(ImmutableList<Label> starlarkFileDependencies) {
13621362
this.starlarkFileDependencies = starlarkFileDependencies;
13631363
return this;
13641364
}

‎src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ private static ImmutableList<Label> transitiveClosureOfLabels(
706706
return ImmutableList.copyOf(set);
707707
}
708708

709-
private static void transitiveClosureOfLabelsRec(
709+
public static void transitiveClosureOfLabelsRec(
710710
Set<Label> set, ImmutableMap<String, Module> loads) {
711711
for (Module m : loads.values()) {
712712
BazelModuleContext ctx = BazelModuleContext.of(m);

‎src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.common.collect.ImmutableList;
2525
import com.google.common.collect.ImmutableMap;
2626
import com.google.common.collect.ImmutableSortedSet;
27+
import com.google.common.collect.Sets;
2728
import com.google.devtools.build.lib.actions.FileValue;
2829
import com.google.devtools.build.lib.analysis.BlazeDirectories;
2930
import com.google.devtools.build.lib.cmdline.Label;
@@ -58,6 +59,7 @@
5859
import java.util.List;
5960
import java.util.Map;
6061
import java.util.Optional;
62+
import java.util.Set;
6163
import net.starlark.java.eval.Module;
6264
import net.starlark.java.eval.Mutability;
6365
import net.starlark.java.eval.Starlark;
@@ -325,14 +327,21 @@ public SkyValue compute(SkyKey skyKey, Environment env)
325327
directories.getWorkspace(),
326328
directories.getLocalJavabase(),
327329
starlarkSemantics);
330+
Set<Label> starlarkFileDependencies;
328331
if (prevValue != null) {
332+
starlarkFileDependencies =
333+
Sets.newLinkedHashSet(prevValue.getPackage().getStarlarkFileDependencies());
329334
try {
330335
parser.setParent(
331336
prevValue.getPackage(), prevValue.getLoadedModules(), prevValue.getBindings());
332337
} catch (NameConflictException e) {
333338
throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
334339
}
340+
} else {
341+
starlarkFileDependencies = Sets.newLinkedHashSet();
335342
}
343+
PackageFactory.transitiveClosureOfLabelsRec(starlarkFileDependencies, loadedModules);
344+
builder.setStarlarkFileDependencies(ImmutableList.copyOf(starlarkFileDependencies));
336345
// Execute the partial files that comprise this chunk.
337346
for (StarlarkFile partialFile : chunk) {
338347
parser.execute(partialFile, loadedModules, key);

‎src/test/py/bazel/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ py_library(
259259

260260
py_test(
261261
name = "bazel_module_test",
262+
size = "large",
262263
srcs = ["bzlmod/bazel_module_test.py"],
263264
deps = [
264265
":bzlmod_test_utils",

‎src/test/py/bazel/query_test.py‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,87 @@ def testSimpleQuery(self):
3838
self._AssertQueryOutput('deps(//foo:top-rule, 1)', '//foo:top-rule',
3939
'//foo:dep-rule')
4040

41+
def testBuildFilesForExternalRepos_Simple(self):
42+
self.ScratchFile('WORKSPACE', [
43+
'load("//:deps.bzl", "repos")',
44+
'repos()',
45+
])
46+
self.ScratchFile('BUILD.bazel')
47+
self.ScratchFile('deps.bzl', [
48+
'def repos():',
49+
' native.new_local_repository(',
50+
' name = "io_bazel_rules_go",',
51+
' path = ".",',
52+
""" build_file_content = "exports_files(glob(['*.go']))",""",
53+
' )',
54+
])
55+
self._AssertQueryOutputContains('buildfiles(//external:io_bazel_rules_go)',
56+
'//external:WORKSPACE', '//:deps.bzl',
57+
'//:BUILD.bazel')
58+
59+
def testBuildFilesForExternalRepos_IndirectLoads(self):
60+
self.ScratchFile('WORKSPACE', [
61+
'load("//:deps.bzl", "repos")',
62+
'repos()',
63+
])
64+
self.ScratchFile('BUILD.bazel')
65+
self.ScratchFile('deps.bzl', [
66+
'load("//:private_deps.bzl", "other_repos")',
67+
'def repos():',
68+
' native.new_local_repository(',
69+
' name = "io_bazel_rules_go",',
70+
' path = ".",',
71+
""" build_file_content = "exports_files(glob(['*.go']))",""",
72+
' )',
73+
' other_repos()',
74+
'',
75+
])
76+
self.ScratchFile('private_deps.bzl', [
77+
'def other_repos():',
78+
' native.new_local_repository(',
79+
' name = "io_bazel_rules_python",',
80+
' path = ".",',
81+
""" build_file_content = "exports_files(glob(['*.py']))",""",
82+
' )',
83+
])
84+
85+
self._AssertQueryOutputContains(
86+
'buildfiles(//external:io_bazel_rules_python)', '//external:WORKSPACE',
87+
'//:deps.bzl', '//:private_deps.bzl', '//:BUILD.bazel')
88+
89+
def testBuildFilesForExternalRepos_NoDuplicates(self):
90+
self.ScratchFile('WORKSPACE', [
91+
'load("//:deps.bzl", "repos")',
92+
'repos()',
93+
])
94+
self.ScratchFile('BUILD.bazel')
95+
self.ScratchFile('deps.bzl', [
96+
'def repos():',
97+
' native.new_local_repository(',
98+
' name = "io_bazel_rules_go",',
99+
' path = ".",',
100+
""" build_file_content = "exports_files(glob(['*.go']))",""",
101+
' )',
102+
' other_repos()',
103+
'',
104+
'def other_repos():',
105+
' native.new_local_repository(',
106+
' name = "io_bazel_rules_python",',
107+
' path = ".",',
108+
""" build_file_content = "exports_files(glob(['*.py']))",""",
109+
' )',
110+
])
111+
112+
exit_code, stdout, stderr = self.RunBazel(
113+
['query', 'buildfiles(//external:io_bazel_rules_python)'])
114+
self.AssertExitCode(exit_code, 0, stderr)
115+
result = set()
116+
for item in stdout:
117+
if not item:
118+
continue
119+
self.assertNotIn(item, result)
120+
result.add(item)
121+
41122
def _AssertQueryOutput(self, query_expr, *expected_results):
42123
exit_code, stdout, stderr = self.RunBazel(['query', query_expr])
43124
self.AssertExitCode(exit_code, 0, stderr)
@@ -46,6 +127,14 @@ def _AssertQueryOutput(self, query_expr, *expected_results):
46127
self.assertEqual(len(stdout), len(expected_results))
47128
self.assertListEqual(stdout, sorted(expected_results))
48129

130+
def _AssertQueryOutputContains(self, query_expr, *expected_content):
131+
exit_code, stdout, stderr = self.RunBazel(['query', query_expr])
132+
self.AssertExitCode(exit_code, 0, stderr)
133+
134+
stdout = {x for x in stdout if x}
135+
for item in expected_content:
136+
self.assertIn(item, stdout)
137+
49138

50139
if __name__ == '__main__':
51140
unittest.main()

0 commit comments

Comments
 (0)