Fix a couple of undocumented beta features

VERSION

@@ -1 +1 @@
-17.34.0-beta.1
+17.34.0-beta.2

src/build/build_step.go

@@ -363,17 +363,21 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 	if runRemotely {
 		if metadata.Cached {
 			target.SetState(core.ReusedRemotely)
-			state.LogBuildResult(target, core.TargetBuilt, "Reused existing action")
 		} else {
 			target.SetState(core.BuiltRemotely)
-			state.LogBuildResult(target, core.TargetBuilt, "Built remotely")
 		}
+		// Download before reporting the target as built, so it's still shown as active while that happens.
 		if state.ShouldDownload(target) {
 			if err := state.EnsureDownloaded(target); err != nil {
 				return err
 			}
 			buildLinks(state, target)
 		}
+		if metadata.Cached {
+			state.LogBuildResult(target, core.TargetBuilt, "Reused existing action")
+		} else {
+			state.LogBuildResult(target, core.TargetBuilt, "Built remotely")
+		}
 		return nil
 	} else if err := StoreTargetMetadata(target, metadata); err != nil {
 		return fmt.Errorf("failed to store target build metadata for %s: %w", target.Label, err)

src/plz/cycle_detector.go

@@ -103,8 +103,12 @@ func (c *cycleDetector) deps(label core.BuildLabel) iter.Seq[core.BuildLabel] {
 			Name:        "all",
 		}})
 	}
-	// Snapshot the dependencies rather than holding the target's lock for the whole recursive walk.
-	deps := slices.Collect(target.DeclaredDependencies())
+	resolved, unresolved := target.Dependencies(c.graph)
+	deps := make([]core.BuildLabel, 0, len(resolved)+len(unresolved))
+	for _, dep := range resolved {
+		deps = append(deps, dep.Label)
+	}
+	deps = append(deps, unresolved...)
 	return slices.Values(append(deps, slices.Collect(c.graph.Subincludes(label))...))
 }
 

src/plz/cycle_detector_test.go

@@ -55,6 +55,20 @@ func TestCycleDetector(t *testing.T) {
 			core.ParseBuildLabel("//src:f", ""),
 		}, cerr.Cycle)
 	})
+
+	t.Run("ProvidesIsNotACycle", func(t *testing.T) {
+		// :sidecar declares a dependency on :k8s, which in turn depends on :sidecar. But :k8s
+		// provides :svc to :sidecar, so the real dependency is only on :svc and there is no cycle.
+		state := core.NewDefaultBuildState()
+		k8s := newTarget(state, "//src:k8s", "//src:svc", "//src:sidecar")
+		k8s.AddProvide("k8s_svc", []core.BuildLabel{core.ParseBuildLabel("//src:svc", "")})
+		sidecar := newTarget(state, "//src:sidecar", "//src:k8s")
+		sidecar.AddRequire("k8s_svc")
+		newTarget(state, "//src:svc")
+
+		detector := cycleDetector{graph: state.Graph}
+		assert.Nil(t, detector.Check())
+	})
 }
 
 func TestCycleDetectorSubincludes(t *testing.T) {

src/plz/plz.go

@@ -558,11 +558,10 @@ 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 {
-		for _, err := range r.resolveTarget(ctx, dep, target) {
-			if err != nil {
-				return err
-			}
+		if err := r.resolveForOutputs(ctx, dep, target, seen); err != nil {
+			return err
 		}
 	}
 
@@ -617,6 +616,31 @@ 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

src/remote/remote.go

@@ -439,11 +439,19 @@ func (c *Client) Download(target *core.BuildTarget) error {
 			log.Debug("Not downloading outputs for %s, they're already up-to-date", target)
 			return nil
 		}
+		// Show the target as active while it downloads; otherwise nothing appears to be happening during a long
+		// download (which also looks like a stall to cycle detection). Failures aren't logged here since all
+		// callers report them already.
+		c.state.LogBuildResult(target, core.TargetBuilding, "Downloading...")
 		_, ar := c.retrieveResults(target, nil, buildAction, false, false, 0)
 		if ar == nil {
 			return fmt.Errorf("Failed to retrieve action result for %s", target)
 		}
-		return c.reallyDownload(target, buildAction, ar)
+		if err := c.reallyDownload(target, buildAction, ar); err != nil {
+			return err
+		}
+		c.state.LogBuildResult(target, core.TargetBuilt, "Downloaded")
+		return nil
 	})
 }
 

src/remote/remote_test.go

@@ -60,6 +60,46 @@ func TestExecuteBuild(t *testing.T) {
 	assert.Equal(t, []byte("hello\n"), metadata.Stdout)
 }
 
+func TestDownloadLogsProgress(t *testing.T) {
+	c := newClientInstance("mock")
+	defer server.Reset()
+
+	content := []byte("this is the content of the output")
+	contentDigest := digest.NewFromBlob(content)
+	server.blobs[contentDigest.Hash] = content
+	server.mockActionResult = &pb.ActionResult{
+		OutputFiles: []*pb.OutputFile{{Path: "out.txt", Digest: contentDigest.ToProto()}},
+		ExitCode:    0,
+		ExecutionMetadata: &pb.ExecutedActionMetadata{
+			Worker:                      "kev",
+			QueuedTimestamp:             timestamppb.Now(),
+			ExecutionStartTimestamp:     timestamppb.Now(),
+			ExecutionCompletedTimestamp: timestamppb.Now(),
+		},
+	}
+
+	target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "download_progress"})
+	target.AddOutput("out.txt")
+	target.Command = "echo hello > $OUT"
+	c.state.Graph.AddTarget(target)
+	_, err := c.Build(target)
+	require.NoError(t, err)
+
+	results := c.state.Results()
+	require.NoError(t, c.Download(target))
+	// A second download is a no-op so shouldn't report anything else.
+	require.NoError(t, c.Download(target))
+	c.state.CloseResults()
+
+	var statuses []core.BuildResultStatus
+	for result := range results {
+		assert.Equal(t, target.Label, result.Label)
+		statuses = append(statuses, result.Status)
+	}
+	assert.Equal(t, []core.BuildResultStatus{core.TargetBuilding, core.TargetBuilt}, statuses)
+	assert.True(t, fs.FileExists(filepath.Join(target.OutDir(), "out.txt")))
+}
+
 type postBuildFunction func(*core.BuildTarget, string) error //nolint:unused
 
 //nolint:unused

test/data_filegroup_outputs/BUILD

@@ -0,0 +1,16 @@
+subinclude("//test/build_defs")
+
+# A target's build command refers to the outputs of a filegroup in its data, whose source is in a
+# package that's slow to parse. The filegroup's outputs can't be calculated until that source has
+# been parsed, even though it doesn't need to be built before the target.
+please_repo_e2e_test(
+    name = "data_filegroup_outputs_test",
+    timeout = 60,
+    expected_output = {
+        "plz-out/gen/a/gen.txt": "a/x.txt",
+    },
+    plz_command = "plz build //a:gen",
+    repo = "test_repo",
+    # The slow parse is the point of the test, so it mustn't be retrieved from the cache.
+    shared_cache = False,
+)

test/data_filegroup_outputs/test_repo/a/BUILD_FILE

@@ -0,0 +1,11 @@
+filegroup(
+    name = "fg",
+    srcs = ["//b:x"],
+)
+
+genrule(
+    name = "gen",
+    outs = ["gen.txt"],
+    cmd = "echo $(location :fg) > $OUT",
+    data = [":fg"],
+)

test/data_filegroup_outputs/test_repo/b/BUILD_FILE

@@ -0,0 +1,14 @@
+genrule(
+    name = "slow_defs",
+    outs = ["slow.build_defs"],
+    cmd = "sleep 3; echo 'X = 1' > $OUT",
+)
+
+subinclude(":slow_defs")
+
+genrule(
+    name = "x",
+    outs = ["x.txt"],
+    cmd = "echo x > $OUT",
+    visibility = ["PUBLIC"],
+)