diff --git a/src/core/build_target.go b/src/core/build_target.go index fa387b35f..3a909c994 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -1451,8 +1451,7 @@ func (target *BuildTarget) AllDebugTools() []BuildInput { func (target *BuildTarget) AddDatum(datum BuildInput) { target.Data = append(target.Data, datum) if label, ok := datum.Label(); ok { - target.AddDependency(label) - target.dependencyInfo(label).Data = true + target.addDataDependency(label) } } @@ -1463,8 +1462,7 @@ func (target *BuildTarget) AddNamedDatum(name string, datum BuildInput) { } target.NamedData[name] = append(target.NamedData[name], datum) if label, ok := datum.Label(); ok { - target.AddDependency(label) - target.dependencyInfo(label).Data = true + target.addDataDependency(label) } } @@ -1475,8 +1473,7 @@ func (target *BuildTarget) AddDebugDatum(datum BuildInput) { } target.Debug.data = append(target.Debug.data, datum) if label, ok := datum.Label(); ok { - target.AddDependency(label) - target.dependencyInfo(label).Data = true + target.addDataDependency(label) } } @@ -1490,6 +1487,14 @@ func (target *BuildTarget) AddDebugNamedDatum(name string, datum BuildInput) { } target.Debug.namedData[name] = append(target.Debug.namedData[name], datum) if label, ok := datum.Label(); ok { + target.addDataDependency(label) + } +} + +// addDataDependency adds a dependency for a data label. It's only marked as data if the target doesn't +// already depend on it in some other way, otherwise it'd stop being a build-time dependency. +func (target *BuildTarget) addDataDependency(label BuildLabel) { + if target.dependencyInfo(label) == nil { target.AddDependency(label) target.dependencyInfo(label).Data = true } diff --git a/src/core/build_target_test.go b/src/core/build_target_test.go index 311c8250e..4d6182e6f 100644 --- a/src/core/build_target_test.go +++ b/src/core/build_target_test.go @@ -451,6 +451,22 @@ func TestBuildDependencies(t *testing.T) { assert.Empty(t, slices.Collect(target5.BuildDependencies())) } +func TestBuildDependenciesDataAndDep(t *testing.T) { + // Something that's both a dependency and data should remain a build dependency, whichever order they're added in. + target1 := makeTarget1("//src/core:target1", "") + target2 := makeTarget1("//src/core:target2", "", target1) + target2.AddDatum(target1.Label) + target2.AddNamedDatum("data", target1.Label) + target3 := makeTarget1("//src/core:target3", "") + target3.AddDatum(target1.Label) + target3.AddDependency(target1.Label) + target4 := makeTarget1("//src/core:target4", "") + target4.AddDatum(target1.Label) + assert.Equal(t, []BuildLabel{target1.Label}, slices.Collect(target2.BuildDependencies())) + assert.Equal(t, []BuildLabel{target1.Label}, slices.Collect(target3.BuildDependencies())) + assert.Empty(t, slices.Collect(target4.BuildDependencies())) +} + func TestDeclaredDependenciesStrict(t *testing.T) { target1 := makeTarget1("//src/core:target1", "") target2 := makeTarget1("//src/core:target2", "", target1) diff --git a/src/plz/plz.go b/src/plz/plz.go index 081f5d55a..c63e1a06b 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -6,7 +6,6 @@ import ( "fmt" "iter" "path/filepath" - "slices" "strings" "sync" "sync/atomic" @@ -572,51 +571,21 @@ func (r *runner) buildTargetAndDeps(ctx context.Context, target *core.BuildTarge } } - // The target's own build command can refer to its run-time & data dependencies via shell replacements, so we must at - // least have them resolved here (they don't have to be built yet though) - deps := slices.Collect(target.RuntimeAndDataDependencies()) - seen := map[core.BuildLabel]struct{}{} - for _, dep := range deps { - if err := r.resolveForOutputs(ctx, dep, target, seen); err != nil { - return err - } - } - - // Okay, now the runtime dependencies can happen in parallel with the target itself. - // N.B. Even when there are none we can't just build the target and return; its own callbacks - // can add some, which we won't know about until it's built. - if len(deps) == 0 { - if err := r.runBuildAction(ctx, target); err != nil { - return err - } - } else if target.PreBuildFunction != nil { - // Pre-build functions can walk the build graph to arbitrary depth (e.g. get_labels()), so - // we have to make sure all targets are available first. - g, gctx = r.group(ctx) - for _, dep := range deps { - g.Go(func() error { - return r.buildDep(gctx, dep, target) - }) - } - if err := g.Wait(); err != nil { - return err - } - if err := r.runBuildAction(ctx, target); err != nil { - return err - } - } else { - g, gctx = r.group(ctx) + // The target's own build command can refer to its run-time & data dependencies via shell replacements, which need + // their outputs to be known (and potentially their contents, e.g. $(hash ...)), so they have to be built first. + g, gctx = r.group(ctx) + for dep := range target.RuntimeAndDataDependencies() { g.Go(func() error { - return r.runBuildAction(gctx, target) + return r.buildDep(gctx, dep, target) }) - for _, dep := range deps { - g.Go(func() error { - return r.buildDep(gctx, dep, target) - }) - } - if err := g.Wait(); err != nil { - return err - } + } + if err := g.Wait(); err != nil { + return err + } + // N.B. Even when there are no runtime dependencies we can't just build the target and return; its own callbacks + // can add some, which we won't know about until it's built. + if err := r.runBuildAction(ctx, target); err != nil { + return err } if !target.ModifiedByCallback { @@ -633,31 +602,6 @@ func (r *runner) buildTargetAndDeps(ctx context.Context, target *core.BuildTarge return g.Wait() } -// resolveForOutputs resolves a dependency such that its outputs can be calculated. For most targets that's just -// the target itself, but filegroups re-output their sources so those must be resolved as well (recursively, -// since they can be filegroups themselves). -func (r *runner) resolveForOutputs(ctx context.Context, dep core.BuildLabel, dependent *core.BuildTarget, seen map[core.BuildLabel]struct{}) error { - for t, err := range r.resolveTarget(ctx, dep, dependent) { - if err != nil { - return err - } else if _, present := seen[t.Label]; present { - continue - } - seen[t.Label] = struct{}{} - if !t.IsFilegroup { - continue - } - for _, src := range t.AllSources() { - if l, ok := src.Label(); ok { - if err := r.resolveForOutputs(ctx, l, t, seen); err != nil { - return err - } - } - } - } - return nil -} - // runBuildAction calls the build for a single target. func (r *runner) runBuildAction(ctx context.Context, target *core.BuildTarget) error { remote := r.anyRemote && !target.Local diff --git a/test/data_output_dirs/BUILD b/test/data_output_dirs/BUILD new file mode 100644 index 000000000..c37091953 --- /dev/null +++ b/test/data_output_dirs/BUILD @@ -0,0 +1,15 @@ +subinclude("//test/build_defs") + +# A target's build command refers to the outputs of a target in its data that has output directories. +# Those outputs can't be known until it's built, so it has to be built before the target. +please_repo_e2e_test( + name = "data_output_dirs_test", + timeout = 60, + expected_output = { + "plz-out/gen/a/gen.txt": "a/x.txt", + }, + plz_command = "plz build //a:gen", + repo = "test_repo", + # Otherwise a bad result could be retrieved from the cache instead of testing the ordering. + shared_cache = False, +) diff --git a/test/data_output_dirs/test_repo/.plzconfig b/test/data_output_dirs/test_repo/.plzconfig new file mode 100644 index 000000000..e69de29bb diff --git a/test/data_output_dirs/test_repo/a/BUILD_FILE b/test/data_output_dirs/test_repo/a/BUILD_FILE new file mode 100644 index 000000000..e7de7a26f --- /dev/null +++ b/test/data_output_dirs/test_repo/a/BUILD_FILE @@ -0,0 +1,12 @@ +genrule( + name = "dir", + cmd = "sleep 3 && mkdir _out && echo x > _out/x.txt", + output_dirs = ["_out"], +) + +genrule( + name = "gen", + outs = ["gen.txt"], + cmd = "echo $(location :dir) > $OUT", + data = [":dir"], +)