From acf01c81763717a3370ada9a39fca0a0d6a4f310 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 12 Oct 2026 17:57:10 +0100 Subject: [PATCH 1/3] Fix data dep issue --- src/core/build_target.go | 17 +++++++++++------ src/core/build_target_test.go | 16 ++++++++++++++++ src/plz/plz.go | 10 ++++++++-- 3 files changed, 35 insertions(+), 8 deletions(-) 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..21a3d6454 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -573,7 +573,7 @@ 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) + // least have them resolved here (they don't generally have to be built yet though) deps := slices.Collect(target.RuntimeAndDataDependencies()) seen := map[core.BuildLabel]struct{}{} for _, dep := range deps { @@ -635,7 +635,8 @@ func (r *runner) buildTargetAndDeps(ctx context.Context, target *core.BuildTarge // 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). +// since they can be filegroups themselves). Targets whose outputs aren't known until they're built (e.g. ones with +// output directories) have to be built. 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 { @@ -644,6 +645,11 @@ func (r *runner) resolveForOutputs(ctx context.Context, dep core.BuildLabel, dep continue } seen[t.Label] = struct{}{} + if t.BuildCouldModifyTarget() { + if _, err := r.Build(ctx, t.Label, dependent.Label); err != nil { + return err + } + } if !t.IsFilegroup { continue } From 3f15a34bb0f8f37aca309e01c50dd08f216cc88d Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 12 Oct 2026 18:04:42 +0100 Subject: [PATCH 2/3] Build data deps first --- src/plz/plz.go | 88 ++++++++------------------------------------------ 1 file changed, 13 insertions(+), 75 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 21a3d6454..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 generally 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,37 +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). Targets whose outputs aren't known until they're built (e.g. ones with -// output directories) have to be built. -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.BuildCouldModifyTarget() { - if _, err := r.Build(ctx, t.Label, dependent.Label); err != nil { - return err - } - } - 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 From 612945edc0fff4abe05b79c9f8c389f85ce5ca15 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 12 Oct 2026 18:04:52 +0100 Subject: [PATCH 3/3] Add test --- test/data_output_dirs/BUILD | 15 +++++++++++++++ test/data_output_dirs/test_repo/.plzconfig | 0 test/data_output_dirs/test_repo/a/BUILD_FILE | 12 ++++++++++++ 3 files changed, 27 insertions(+) create mode 100644 test/data_output_dirs/BUILD create mode 100644 test/data_output_dirs/test_repo/.plzconfig create mode 100644 test/data_output_dirs/test_repo/a/BUILD_FILE 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"], +)