Skip to content

Commit bc8092a

Browse files
chrisnovakovicchrisnovakovic
authored
Optionally traverse srcs and deps for run-time dependencies (#3466)
There are circumstances under which a target might want to inherit the run-time dependencies of certain build-time dependencies, because the target's output is built in such a way that its dependencies don't have their own run-time dependencies resolved. An example of this is the shell-rules plugin's `sh_binary` rule: a `sh_binary` is a zip file of its dependencies (`deps`) concatenated onto a shell script preamble that extracts those dependencies into a temporary directory during initialisation. If one of those `deps` happens to have its own run-time dependencies, Please will not resolve them, and they will therefore not be built before the target runs (and will not be sent to the remote worker along with the target's output). Add two parameters to `build_rule` - `runtime_deps_from_srcs` and `runtime_deps_from_deps` - that indicate to Please that run-time dependencies should additionally be collected from the target's `srcs` and `deps` respectively. This allows build definitions to inherit the run-time dependencies of their sources and/or build-time dependencies if they know they will be needed when running the target they generate.
1 parent 7872c83 commit bc8092a

File tree

14 files changed

+406
-80
lines changed

14 files changed

+406
-80
lines changed

‎rules/builtins.build_defs‎

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,64 @@
22

33

44
# Do not change the order of arguments to this function without updating the iota in targets.go to match it.
5-
def build_rule(name:str, cmd:str|dict='', test_cmd:str|dict='', debug_cmd:str='', srcs:list|dict=None, data:list|dict=None,
6-
debug_data:list|dict=None, outs:list|dict=None, deps:list=None, exported_deps:list=None, runtime_deps:list=None,
7-
secrets:list|dict=None, tools:str|list|dict=None, test_tools:str|list|dict=None, debug_tools:str|list|dict=None,
8-
labels:list=None, visibility:list=CONFIG.DEFAULT_VISIBILITY, hashes:list=None, binary:bool=False, test:bool=False,
9-
test_only:bool=CONFIG.DEFAULT_TESTONLY, building_description:str=None, needs_transitive_deps:bool=False,
10-
output_is_complete:bool=False, sandbox:bool=CONFIG.BUILD_SANDBOX, test_sandbox:bool=CONFIG.TEST_SANDBOX,
11-
no_test_output:bool=False, flaky:bool|int=0, build_timeout:int|str=0, test_timeout:int|str=0, pre_build:function=None,
12-
post_build:function=None, requires:list=None, provides:dict=None, licences:list=CONFIG.DEFAULT_LICENCES,
13-
test_outputs:list=None, system_srcs:list=None, stamp:bool=False, tag:str='', optional_outs:list=None, progress:bool=False,
14-
size:str=None, _urls:list=None, internal_deps:list=None, pass_env:list=None, local:bool=False, output_dirs:list=[],
15-
exit_on_error:bool=CONFIG.EXIT_ON_ERROR, entry_points:dict={}, env:dict={}, _file_content:str=None,
16-
_subrepo:bool=False, no_test_coverage:bool=False, src_list_files:bool=False):
5+
def build_rule(
6+
name:str,
7+
cmd:str|dict="",
8+
test_cmd:str|dict="",
9+
debug_cmd:str="",
10+
srcs:list|dict=None,
11+
data:list|dict=None,
12+
debug_data:list|dict=None,
13+
outs:list|dict=None,
14+
deps:list=None,
15+
exported_deps:list=None,
16+
runtime_deps:list=None,
17+
runtime_deps_from_srcs:bool=False,
18+
runtime_deps_from_deps:bool=False,
19+
secrets:list|dict=None,
20+
tools:str|list|dict=None,
21+
test_tools:str|list|dict=None,
22+
debug_tools:str|list|dict=None,
23+
labels:list=None,
24+
visibility:list=CONFIG.DEFAULT_VISIBILITY,
25+
hashes:list=None,
26+
binary:bool=False,
27+
test:bool=False,
28+
test_only:bool=CONFIG.DEFAULT_TESTONLY,
29+
building_description:str=None,
30+
needs_transitive_deps:bool=False,
31+
output_is_complete:bool=False,
32+
sandbox:bool=CONFIG.BUILD_SANDBOX,
33+
test_sandbox:bool=CONFIG.TEST_SANDBOX,
34+
no_test_output:bool=False,
35+
flaky:bool|int=0,
36+
build_timeout:int|str=0,
37+
test_timeout:int|str=0,
38+
pre_build:function=None,
39+
post_build:function=None,
40+
requires:list=None,
41+
provides:dict=None,
42+
licences:list=CONFIG.DEFAULT_LICENCES,
43+
test_outputs:list=None,
44+
system_srcs:list=None,
45+
stamp:bool=False,
46+
tag:str="",
47+
optional_outs:list=None,
48+
progress:bool=False,
49+
size:str=None,
50+
_urls:list=None,
51+
internal_deps:list=None,
52+
pass_env:list=None,
53+
local:bool=False,
54+
output_dirs:list=[],
55+
exit_on_error:bool=CONFIG.EXIT_ON_ERROR,
56+
entry_points:dict={},
57+
env:dict={},
58+
_file_content:str=None,
59+
_subrepo:bool=False,
60+
no_test_coverage:bool=False,
61+
src_list_files:bool=False,
62+
):
1763
pass
1864

1965
def chr(i:int) -> str:

‎rules/misc_rules.build_defs‎

Lines changed: 89 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,42 @@
11
"""Miscellaneous rules that aren't language-specific."""
22

33

4-
def genrule(name:str, cmd:str|list|dict, srcs:list|dict=None, out:str=None, outs:list|dict=None, deps:list=None,
5-
exported_deps:list=None, runtime_deps:list=None, labels:list&features&tags=None, visibility:list=None,
6-
building_description:str='Building...', data:list|dict=None, hashes:list=None, timeout:int=0, binary:bool=False,
7-
sandbox:bool=None, needs_transitive_deps:bool=False, output_is_complete:bool=True,
8-
test_only:bool&testonly=False, secrets:list|dict=None, requires:list=None, provides:dict=None,
9-
pre_build:function=None, post_build:function=None, tools:str|list|dict=None, pass_env:list=None,
10-
local:bool=False, output_dirs:list=[], exit_on_error:bool=CONFIG.EXIT_ON_ERROR, entry_points:dict={},
11-
env:dict={}, optional_outs:list=[]):
4+
def genrule(
5+
name:str,
6+
cmd:str|list|dict,
7+
srcs:list|dict=None,
8+
out:str=None,
9+
outs:list|dict=None,
10+
deps:list=None,
11+
exported_deps:list=None,
12+
runtime_deps:list=None,
13+
runtime_deps_from_srcs:bool=False,
14+
runtime_deps_from_deps:bool=False,
15+
labels:list&features&tags=None,
16+
visibility:list=None,
17+
building_description:str="Building...",
18+
data:list|dict=None,
19+
hashes:list=None,
20+
timeout:int=0,
21+
binary:bool=False,
22+
sandbox:bool=None,
23+
needs_transitive_deps:bool=False,
24+
output_is_complete:bool=True,
25+
test_only:bool&testonly=False,
26+
secrets:list|dict=None,
27+
requires:list=None,
28+
provides:dict=None,
29+
pre_build:function=None,
30+
post_build:function=None,
31+
tools:str|list|dict=None,
32+
pass_env:list=None,
33+
local:bool=False,
34+
output_dirs:list=[],
35+
exit_on_error:bool=CONFIG.EXIT_ON_ERROR,
36+
entry_points:dict={},
37+
env:dict={},
38+
optional_outs:list=[],
39+
):
1240
"""A general build rule which allows the user to specify a command.
1341

1442
Args:
@@ -41,6 +69,14 @@ def genrule(name:str, cmd:str|list|dict, srcs:list|dict=None, out:str=None, outs
4169
the outputs of those rules' transitive run-time dependencies, will exist in
4270
the dependent rule's build environment. Requires the rule to produce a runnable
4371
output (i.e. binary = True).
72+
runtime_deps_from_srcs (bool): If true, additionally collect run-time dependencies from this target's
73+
sources. This is useful if the target's output simply collects its
74+
sources in some way without eliminating their own run-time dependencies.
75+
runtime_deps_from_deps (bool): If true, additionally collect run-time dependencies from this target's
76+
build-time dependencies (and those targets' exported dependencies).
77+
This is useful if the target's output includes its dependencies without
78+
eliminating their own run-time dependencies, e.g. for targets generated
79+
by the shell-rules plugin's sh_binary rule.
4480
tools (str | list | dict): Tools used to build this rule; similar to srcs but are not copied to the
4581
temporary build directory. Should be accessed via $(exe //path/to:tool)
4682
or similar.
@@ -127,6 +163,8 @@ def genrule(name:str, cmd:str|list|dict, srcs:list|dict=None, out:str=None, outs
127163
deps = deps,
128164
exported_deps = exported_deps,
129165
runtime_deps = runtime_deps,
166+
runtime_deps_from_srcs = runtime_deps_from_srcs,
167+
runtime_deps_from_deps = runtime_deps_from_deps,
130168
data = data,
131169
tools = tools,
132170
secrets = secrets,
@@ -154,13 +192,38 @@ def genrule(name:str, cmd:str|list|dict, srcs:list|dict=None, out:str=None, outs
154192
)
155193

156194

157-
def gentest(name:str, test_cmd:str|list|dict, labels:list&features&tags=None, cmd:str|list|dict=None, srcs:list|dict=None,
158-
outs:list=None, deps:list=None, exported_deps:list=None, runtime_deps:list=None, tools:str|list|dict=None,
159-
test_tools:str|list|dict=None, data:list|dict=None, visibility:list=None, timeout:int=0,
160-
needs_transitive_deps:bool=False, flaky:bool|int=0, secrets:list|dict=None, no_test_output:bool=False,
161-
test_outputs:list=None, output_is_complete:bool=True, requires:list=None, sandbox:bool=None, size:str=None,
162-
local:bool=False, pass_env:list=None, env:dict=None, exit_on_error:bool=CONFIG.EXIT_ON_ERROR,
163-
no_test_coverage:bool=False):
195+
def gentest(
196+
name:str,
197+
test_cmd:str|list|dict,
198+
labels:list&features&tags=None,
199+
cmd:str|list|dict=None,
200+
srcs:list|dict=None,
201+
outs:list=None,
202+
deps:list=None,
203+
exported_deps:list=None,
204+
runtime_deps:list=None,
205+
runtime_deps_from_srcs:bool=False,
206+
runtime_deps_from_deps:bool=False,
207+
tools:str|list|dict=None,
208+
test_tools:str|list|dict=None,
209+
data:list|dict=None,
210+
visibility:list=None,
211+
timeout:int=0,
212+
needs_transitive_deps:bool=False,
213+
flaky:bool|int=0,
214+
secrets:list|dict=None,
215+
no_test_output:bool=False,
216+
test_outputs:list=None,
217+
output_is_complete:bool=True,
218+
requires:list=None,
219+
sandbox:bool=None,
220+
size:str=None,
221+
local:bool=False,
222+
pass_env:list=None,
223+
env:dict=None,
224+
exit_on_error:bool=CONFIG.EXIT_ON_ERROR,
225+
no_test_coverage:bool=False,
226+
):
164227
"""A rule which creates a test with an arbitrary command.
165228

166229
The command must return zero on success and nonzero on failure. Test results are written
@@ -185,6 +248,14 @@ def gentest(name:str, test_cmd:str|list|dict, labels:list&features&tags=None, cm
185248
list, as well as those rules' transitive run-time dependencies, will exist in
186249
the test environment. Requires the rule to produce a runnable output (i.e.
187250
binary = True).
251+
runtime_deps_from_srcs (bool): If true, additionally collect run-time dependencies from this target's
252+
sources. This is useful if the target's output simply collects its
253+
sources in some way without eliminating their own run-time dependencies.
254+
runtime_deps_from_deps (bool): If true, additionally collect run-time dependencies from this target's
255+
build-time dependencies (and those targets' exported dependencies).
256+
This is useful if the target's output includes its dependencies without
257+
eliminating their own run-time dependencies, e.g. for targets generated
258+
by the shell-rules plugin's sh_binary rule.
188259
tools (str | list | dict): Tools used to build this rule; similar to srcs but are not copied to the temporary
189260
build directory.
190261
test_tools (str | list | dict): Like tools but available to test_cmd instead.
@@ -224,6 +295,8 @@ def gentest(name:str, test_cmd:str|list|dict, labels:list&features&tags=None, cm
224295
deps = deps,
225296
exported_deps = exported_deps,
226297
runtime_deps = runtime_deps,
298+
runtime_deps_from_srcs = runtime_deps_from_srcs,
299+
runtime_deps_from_deps = runtime_deps_from_deps,
227300
data = data,
228301
tools = tools,
229302
test_tools = test_tools,
@@ -270,7 +343,7 @@ def export_file(name:str, src:str, visibility:list=None, binary:bool=False, test
270343
)
271344

272345

273-
def filegroup(name:str, tag:str='', srcs:list=None, deps:list=None, exported_deps:list=None, runtime_deps:list=None,
346+
def filegroup(name:str, tag:str='', srcs:list=None, deps:list=None, exported_deps:list=None,
274347
visibility:list=None, labels:list&features&tags=None, binary:bool=False, output_is_complete:bool=True,
275348
requires:list=None, provides:dict=None, hashes:list=None, test_only:bool&testonly=False):
276349
"""Defines a collection of files which other rules can depend on.
@@ -285,8 +358,6 @@ def filegroup(name:str, tag:str='', srcs:list=None, deps:list=None, exported_dep
285358
srcs (list): Source files for the rule.
286359
deps (list): Dependencies of the rule.
287360
exported_deps (list): Dependencies that will become visible to any rules that depend on this rule.
288-
runtime_deps (list): Run-time dependencies of this rule. Requires the rule to produce a runnable
289-
output (i.e. binary = True).
290361
visibility (list): Visibility declaration
291362
labels (list): Labels to apply to this rule
292363
binary (bool): True to mark the rule outputs as binary
@@ -305,7 +376,6 @@ def filegroup(name:str, tag:str='', srcs:list=None, deps:list=None, exported_dep
305376
srcs=srcs,
306377
deps=deps,
307378
exported_deps=exported_deps,
308-
runtime_deps=runtime_deps,
309379
visibility=visibility,
310380
building_description='Copying...',
311381
output_is_complete=output_is_complete,

‎src/build/incrementality_test.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,12 @@ var KnownFields = map[string]bool{
8181
"Debug.namedTools": true,
8282

8383
// These only contribute to the runtime hash, not at build time.
84-
"runtimeDependencies": true,
85-
"Data": true,
86-
"NamedData": true,
87-
"ContainerSettings": true,
84+
"runtimeDependencies": true,
85+
"RuntimeDependenciesFromSources": true,
86+
"RuntimeDependenciesFromDependencies": true,
87+
"Data": true,
88+
"NamedData": true,
89+
"ContainerSettings": true,
8890

8991
// These would ideally not contribute to the hash, but we need that at present
9092
// because we don't have a good way to force a recheck of its reverse dependencies.

‎src/core/build_target.go‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,13 @@ type BuildTarget struct {
117117
dependencies []depInfo `name:"deps"`
118118
// The run-time dependencies of this target.
119119
runtimeDependencies []BuildLabel `name:"runtime_deps"`
120+
// Whether to consider the run-time dependencies of this target's sources to be additional
121+
// run-time dependencies of this target.
122+
RuntimeDependenciesFromSources bool `name:"runtime_deps_from_srcs"`
123+
// Whether to consider the run-time dependencies of this target's build-time dependencies
124+
// (and exported dependencies of those targets) to be additional run-time dependencies of
125+
// this target.
126+
RuntimeDependenciesFromDependencies bool `name:"runtime_deps_from_deps"`
120127
// List of build target patterns that can use this build target.
121128
Visibility []BuildLabel
122129
// Source files of this rule. Can refer to build rules themselves.
@@ -732,9 +739,9 @@ func (target *BuildTarget) IterAllRuntimeDependencies(graph *BuildGraph) iter.Se
732739
return true
733740
}
734741
done[t.String()] = true
735-
for _, runDep := range t.runtimeDependencies {
736-
runDepLabel, _ := runDep.Label()
737-
for _, providedDep := range graph.TargetOrDie(runDepLabel).ProvideFor(t) {
742+
for _, dep := range t.runtimeDependencies {
743+
depLabel, _ := dep.Label()
744+
for _, providedDep := range graph.TargetOrDie(depLabel).ProvideFor(t) {
738745
if !yield(providedDep) {
739746
return false
740747
}
@@ -743,6 +750,38 @@ func (target *BuildTarget) IterAllRuntimeDependencies(graph *BuildGraph) iter.Se
743750
}
744751
}
745752
}
753+
if t.RuntimeDependenciesFromSources || t.RuntimeDependenciesFromDependencies {
754+
for _, dep := range t.dependencies {
755+
// If required, include the run-time dependencies of sources, but not the sources themselves.
756+
if t.RuntimeDependenciesFromSources && dep.source {
757+
depLabel, _ := dep.declared.Label()
758+
for _, providedDep := range graph.TargetOrDie(depLabel).ProvideFor(t) {
759+
if !push(graph.TargetOrDie(providedDep), yield) {
760+
return false
761+
}
762+
}
763+
}
764+
// If required, include the run-time dependencies of dependencies, but not the dependencies themselves.
765+
if t.RuntimeDependenciesFromDependencies && !dep.exported && !dep.source && !dep.internal && !dep.runtime {
766+
depLabel, _ := dep.declared.Label()
767+
depTarget := graph.TargetOrDie(depLabel)
768+
for _, providedDep := range depTarget.ProvideFor(t) {
769+
if !push(graph.TargetOrDie(providedDep), yield) {
770+
return false
771+
}
772+
}
773+
// Also include the run-time dependencies of the target's exported dependencies, but not the
774+
// exported dependencies themselves.
775+
for _, exportedDep := range depTarget.ExportedDependencies() {
776+
for _, providedDep := range graph.TargetOrDie(exportedDep).ProvideFor(t) {
777+
if !push(graph.TargetOrDie(providedDep), yield) {
778+
return false
779+
}
780+
}
781+
}
782+
}
783+
}
784+
}
746785
return true
747786
}
748787
return func(yield func(BuildLabel) bool) {

‎src/parse/asp/targets.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ const (
3131
depsBuildRuleArgIdx
3232
exportedDepsBuildRuleArgIdx
3333
runtimeDepsBuildRuleArgIdx
34+
runtimeDepsFromSrcsBuildRuleArgIdx
35+
runtimeDepsFromDepsBuildRuleArgIdx
3436
secretsBuildRuleArgIdx
3537
toolsBuildRuleArgIdx
3638
testToolsBuildRuleArgIdx
@@ -142,6 +144,11 @@ func createTarget(s *scope, args []pyObject) *core.BuildTarget {
142144
if target.IsRemoteFile {
143145
target.AddLabel("remote")
144146
}
147+
// filegroups don't really produce any outputs of their own - they're just the filegroup's own sources.
148+
// The run-time dependencies of a filegroup's sources should therefore be treated as the filegroup's own
149+
// run-time dependencies.
150+
target.RuntimeDependenciesFromSources = target.IsFilegroup || isTruthy(runtimeDepsFromSrcsBuildRuleArgIdx)
151+
target.RuntimeDependenciesFromDependencies = isTruthy(runtimeDepsFromDepsBuildRuleArgIdx)
145152
target.Command, target.Commands = decodeCommands(s, args[cmdBuildRuleArgIdx])
146153
if test {
147154
target.Test = new(core.TestFields)

‎test/runtime_deps/BUILD‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,27 @@ please_repo_e2e_test(
6666
},
6767
repo = "repo",
6868
)
69+
70+
please_repo_e2e_test(
71+
name = "runtime_deps_from_srcs_test",
72+
plz_command = "plz test //from_srcs_test:runtime_deps_from_srcs_test_case",
73+
repo = "repo",
74+
)
75+
76+
please_repo_e2e_test(
77+
name = "runtime_deps_from_deps_test",
78+
plz_command = "plz test //from_deps_test:runtime_deps_from_deps_test_case",
79+
repo = "repo",
80+
)
81+
82+
please_repo_e2e_test(
83+
name = "runtime_deps_from_exported_deps_test",
84+
plz_command = "plz test //from_exported_deps_test:runtime_deps_from_exported_deps_test_case",
85+
repo = "repo",
86+
)
87+
88+
please_repo_e2e_test(
89+
name = "filegroup_test",
90+
plz_command = "plz test //filegroup_test:filegroup_test_case",
91+
repo = "repo",
92+
)

‎test/runtime_deps/repo/build_defs/BUILD_FILE‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
export_file(
2+
name = "test",
3+
src = "test.build_defs",
4+
visibility = ["PUBLIC"],
5+
)

0 commit comments

Comments
 (0)