diff --git a/VERSION b/VERSION index 5b5f5612b..665666191 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -17.34.0-beta.1 +17.34.0-beta.2 diff --git a/src/build/build_step.go b/src/build/build_step.go index 14d15975d..bfb43778e 100644 --- a/src/build/build_step.go +++ b/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) diff --git a/src/plz/cycle_detector.go b/src/plz/cycle_detector.go index 71edfbab2..9ef0c97ee 100644 --- a/src/plz/cycle_detector.go +++ b/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))...)) } diff --git a/src/plz/cycle_detector_test.go b/src/plz/cycle_detector_test.go index 696015b81..38b64fe5a 100644 --- a/src/plz/cycle_detector_test.go +++ b/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) { diff --git a/src/plz/plz.go b/src/plz/plz.go index f013fb186..22cc3d72e 100644 --- a/src/plz/plz.go +++ b/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 diff --git a/src/remote/remote.go b/src/remote/remote.go index c6e9407f4..b2ef4fad0 100644 --- a/src/remote/remote.go +++ b/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 }) } diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index ffe2bc127..3680332d6 100644 --- a/src/remote/remote_test.go +++ b/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 diff --git a/test/data_filegroup_outputs/BUILD b/test/data_filegroup_outputs/BUILD new file mode 100644 index 000000000..b8e3f68c1 --- /dev/null +++ b/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, +) diff --git a/test/data_filegroup_outputs/test_repo/.plzconfig b/test/data_filegroup_outputs/test_repo/.plzconfig new file mode 100644 index 000000000..e69de29bb diff --git a/test/data_filegroup_outputs/test_repo/a/BUILD_FILE b/test/data_filegroup_outputs/test_repo/a/BUILD_FILE new file mode 100644 index 000000000..2cb10e7ea --- /dev/null +++ b/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"], +) diff --git a/test/data_filegroup_outputs/test_repo/b/BUILD_FILE b/test/data_filegroup_outputs/test_repo/b/BUILD_FILE new file mode 100644 index 000000000..9641ee3c9 --- /dev/null +++ b/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"], +)