Build data deps before targets

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
 	}

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)

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

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,
+)

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"],
+)