From 72811635b397bcafeba1f523681cdd33fcf6df5e Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 10:05:02 +0100 Subject: [PATCH 01/44] Sort out original targets and fix up all the tests in src/... --- src/build/build_step_stress_test.go | 13 +++----- src/build/build_step_test.go | 4 +-- src/core/command_replacements_test.go | 2 ++ src/core/cycle_detector_test.go | 1 - src/core/state.go | 5 +++ src/core/state_test.go | 48 ++++++--------------------- src/core/utils_test.go | 2 +- src/plz/plz.go | 1 + src/remote/remote_test.go | 2 +- 9 files changed, 27 insertions(+), 51 deletions(-) diff --git a/src/build/build_step_stress_test.go b/src/build/build_step_stress_test.go index 6a4830e802..58fc489905 100644 --- a/src/build/build_step_stress_test.go +++ b/src/build/build_step_stress_test.go @@ -33,10 +33,10 @@ func TestBuildLotsOfTargets(t *testing.T) { pkg := core.NewPackage("pkg") state.Graph.AddPackage(pkg) + targets := []core.BuildLabel{} for i := 1; i <= size; i++ { - addTarget(state, i) + targets = append(targets, addTarget(state, i).Label) } - state.TaskDone() // Initial target adding counts as one. results := state.Results() // Consume and discard any results @@ -47,7 +47,7 @@ func TestBuildLotsOfTargets(t *testing.T) { } }() - plz.RunHost(nil, state) + plz.RunHost(targets, state) } func addTarget(state *core.BuildState, i int) *core.BuildTarget { @@ -68,9 +68,6 @@ func addTarget(state *core.BuildState, i int) *core.BuildTarget { log.Info("Adding dependency %s -> %s", target.Label, dep) target.AddDependency(dep) } - } else { - // These are buildable now - state.QueueTarget(target.Label, core.OriginalTarget, false, core.ParseModeNormal) } } return target @@ -106,7 +103,7 @@ func (fake *fakeParser) RegisterPreload(core.BuildLabel) error { } // ParseFile stub -func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error { +func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error { return nil } @@ -125,7 +122,7 @@ func (fake *fakeParser) Init(state *core.BuildState) { } // ParseReader stub -func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel, mode core.ParseMode) error { +func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel) error { return nil } diff --git a/src/build/build_step_test.go b/src/build/build_step_test.go index abee373e3a..01c1834ca8 100644 --- a/src/build/build_step_test.go +++ b/src/build/build_step_test.go @@ -614,7 +614,7 @@ func (fake *fakeParser) RegisterPreload(core.BuildLabel) error { } // ParseFile stub -func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error { +func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error { return nil } @@ -632,7 +632,7 @@ func (fake *fakeParser) NewParser(state *core.BuildState) { } // ParseReader stub -func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel, mode core.ParseMode) error { +func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel) error { return nil } diff --git a/src/core/command_replacements_test.go b/src/core/command_replacements_test.go index 576c80d75f..3111934f55 100644 --- a/src/core/command_replacements_test.go +++ b/src/core/command_replacements_test.go @@ -356,8 +356,10 @@ func TestTestCommand(t *testing.T) { }) t.Run("Combined sequence and placeholder replacement", func(t *testing.T) { + state := NewDefaultBuildState() target2 := makeTarget2("//path/to:target2", "", nil) target1 := makeTarget2("//path/to:target1", "$(location //path/to:target2) __TEST_ARGS__", target2) + state.Graph.AddTarget(target2) target1.Test = &TestFields{ Command: "$(location //path/to:target2) __TEST_ARGS__", ArgsPlaceholder: "__TEST_ARGS__", diff --git a/src/core/cycle_detector_test.go b/src/core/cycle_detector_test.go index 0990cd7e7b..58a9ca8cf8 100644 --- a/src/core/cycle_detector_test.go +++ b/src/core/cycle_detector_test.go @@ -14,7 +14,6 @@ func TestCycleDetector(t *testing.T) { target.AddDependency(ParseBuildLabel(dep, "")) } state.Graph.AddTarget(target) - state.QueueTarget(target.Label, OriginalTarget, true, ParseModeForSubinclude) return target } diff --git a/src/core/state.go b/src/core/state.go index 122fe3bade..526c1844b5 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -323,6 +323,11 @@ func (state *BuildState) CloseResults() { } } +// AddOriginalTarget adds an original target to this state +func (state *BuildState) AddOriginalTarget(label BuildLabel) { + state.progress.originalTargets.Add(label) +} + // IsOriginalTarget returns true if a target is an original target, ie. one specified on the command line. func (state *BuildState) IsOriginalTarget(target *BuildTarget) bool { return state.isOriginalTarget(target, false) diff --git a/src/core/state_test.go b/src/core/state_test.go index 010988a24e..581cf09834 100644 --- a/src/core/state_test.go +++ b/src/core/state_test.go @@ -9,8 +9,8 @@ import ( func TestExpandOriginalLabels(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/parse", Name: "parse"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}) + state.AddOriginalTarget(BuildLabel{PackageName: "src/parse", Name: "parse"}) state.Include = []string{"go"} state.Exclude = []string{"py"} @@ -34,7 +34,7 @@ func TestExpandOriginalLabels(t *testing.T) { func TestExpandOriginalTestLabels(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}) state.NeedTests = true state.Include = []string{"go"} state.Exclude = []string{"py"} @@ -50,7 +50,7 @@ func TestExpandOriginalTestLabels(t *testing.T) { func TestExpandVisibleOriginalTargets(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}) addTarget(state, "//src/core:target1", "py") addTarget(state, "//src/core:_target1#zip", "py") @@ -59,8 +59,8 @@ func TestExpandVisibleOriginalTargets(t *testing.T) { func TestExpandOriginalSubLabels(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}) state.Include = []string{"go"} state.Exclude = []string{"py"} addTarget(state, "//src/core:target1", "go") @@ -76,10 +76,10 @@ func TestExpandOriginalSubLabels(t *testing.T) { func TestExpandOriginalLabelsOrdering(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/parse", Name: "parse"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/build", Name: "build"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/parse", Name: "parse"}) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}) + state.AddOriginalTarget(BuildLabel{PackageName: "src/build", Name: "build"}) addTarget(state, "//src/core:target1", "go") addTarget(state, "//src/core:target2", "py") addTarget(state, "//src/core/tests:target3", "go") @@ -109,24 +109,6 @@ func TestAddTargetFilegroupPackageOutputs(t *testing.T) { assert.True(t, exists) } -func TestAddDepsToTarget(t *testing.T) { - state := NewDefaultBuildState() - _, builds := state.TaskQueues() - pkg := NewPackage("src/core") - target1 := addTargetDeps(state, pkg, "//src/core:target1", "//src/core:target2") - target2 := addTargetDeps(state, pkg, "//src/core:target2") - state.Graph.AddPackage(pkg) - state.QueueTarget(target1.Label, OriginalTarget, false, ParseModeNormal) - task := <-builds - assert.Equal(t, Task{Target: target2}, task) - // Now simulate target2 being built and adding a new dep to target1 in its post-build function. - target3 := addTargetDeps(state, pkg, "//src/core:target3") - target1.AddDependency(target3.Label) - target2.FinishBuild() - task = <-builds - assert.Equal(t, Task{Target: target3}, task) -} - func addTarget(state *BuildState, name string, labels ...string) { target := NewBuildTarget(ParseBuildLabel(name, "")) target.Labels = labels @@ -142,16 +124,6 @@ func addTarget(state *BuildState, name string, labels ...string) { state.Graph.AddTarget(target) } -func addTargetDeps(state *BuildState, pkg *Package, name string, deps ...string) *BuildTarget { - target := NewBuildTarget(ParseBuildLabel(name, "")) - for _, d := range deps { - target.AddDependency(ParseBuildLabel(d, "")) - } - pkg.AddTarget(target) - state.Graph.AddTarget(target) - return target -} - func TestCopyPlugin(t *testing.T) { plugin := &Plugin{ ExtraValues: map[string][]string{ diff --git a/src/core/utils_test.go b/src/core/utils_test.go index 79b61493b6..831c12fb9a 100644 --- a/src/core/utils_test.go +++ b/src/core/utils_test.go @@ -72,8 +72,8 @@ func TestIterSources(t *testing.T) { assert.Equal(t, []SourcePair{ {"src/output/output2.go", "plz-out/tmp/src/output/output2._build/src/output/output2.go"}, - {"plz-out/gen/src/core/target2.a", "plz-out/tmp/src/output/output2._build/src/core/target2.a"}, {"plz-out/gen/src/output/output1.a", "plz-out/tmp/src/output/output2._build/src/output/output1.a"}, + {"plz-out/gen/src/core/target2.a", "plz-out/tmp/src/output/output2._build/src/core/target2.a"}, }, iterSources("//src/output:output2")) assert.Equal(t, []SourcePair{ diff --git a/src/plz/plz.go b/src/plz/plz.go index 796d3bcee8..991144607b 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -400,6 +400,7 @@ func (tf *taskFinder) findOriginalTask(target core.BuildLabel, needTest, needBui func (tf *taskFinder) queueTask(target core.BuildLabel, needTest, needBuild bool) { tf.tasks.Go(func() error { + tf.state.AddOriginalTarget(target) if needTest { return tf.state.Test(target) } else if needBuild { diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index f5a326f36f..07a35f90be 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -255,7 +255,7 @@ func TestOutDirsSetOutsOnTarget(t *testing.T) { Name: "out_dir_target", }) - c.state.AddOriginalTarget(outDirTarget.Label, true) + c.state.AddOriginalTarget(outDirTarget.Label) c.state.OutputDownload = core.OriginalOutputDownload require.True(t, c.state.ShouldDownload(outDirTarget)) From 271a5ec8cc1b39df53367b8f7ce5c9dfffc6bdba Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 15:28:02 +0100 Subject: [PATCH 02/44] Refactor all this to a separate type with member functions. Bit simpler and we pass the context around better --- src/core/state.go | 10 +- src/plz/plz.go | 468 ++++++++++++++++++++++++---------------------- 2 files changed, 246 insertions(+), 232 deletions(-) diff --git a/src/core/state.go b/src/core/state.go index 526c1844b5..e926496de4 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -217,13 +217,11 @@ type BuildState struct { // NeedDebugDeps is true if we're doing a `plz debug` and we need to build the debug tools and data NeedDebugDeps bool - // Various callbacks which are set from outside. - // Parse parses a package synchronously - Parse func(BuildLabel) (*Package, error) - // Build builds a single target + // Build is a callback to build a single target. It's set from outside here. + // TODO(peter): can we find a way of moving these off this struct? it feels weird here Build func(BuildLabel) (*BuildTarget, error) - // Test runs tests on a single target - Test func(BuildLabel) error + // Parse is a callback to parse a single package. It's also set from outside. + Parse func(BuildLabel) (*Package, error) // initOnce is used to control loading the subrepo .plzconfig initOnce *sync.Once diff --git a/src/plz/plz.go b/src/plz/plz.go index 991144607b..8d98b34a38 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -42,300 +42,316 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress parse.InitParser(state) - localLimiter := make(limiter, state.Config.Please.NumThreads) - remoteLimiter := make(limiter, state.Config.NumRemoteExecutors()) - anyRemote := state.Config.NumRemoteExecutors() > 0 + g, ctx := errgroup.WithContext(context.Background()) - limiter := func(remote bool) limiter { - if remote { - return remoteLimiter - } - return localLimiter + r := runner{ + state: state, + arch: arch, + tasks: g, + progress: progress, + buildOnce: cmap.NewErrMap[core.BuildLabel, *core.BuildTarget](cmap.DefaultShardCount, func(l core.BuildLabel) uint64 { + return cmap.XXHashes(l.Subrepo, l.PackageName, l.Name) + }, nil), + localLimiter: make(limiter, state.Config.Please.NumThreads), + remoteLimiter: make(limiter, state.Config.NumRemoteExecutors()), + anyRemote: state.Config.NumRemoteExecutors() > 0, } - // TODO(peter): we should probably stitch these contexts around more - g, ctx := errgroup.WithContext(context.Background()) - + // We don't have context as an argument to this, because they're not fully plumbed through (but probably should be) + state.Build = func(label core.BuildLabel) (*core.BuildTarget, error) { + return r.Build(ctx, label) + } state.Parse = func(label core.BuildLabel) (*core.Package, error) { - if pkg, err := state.Graph.PackageOrWait(ctx, label); err != nil || pkg != nil { - return pkg, err - } - // If we get here then we have to parse it (we only get here if we are the first one) - progress.numParsing.Add(1) - defer progress.numParsing.Add(-1) - // If the target defines a subrepo, we must make sure that is built first. - if label.Subrepo != "" { - sl := label.SubrepoLabel(state) - if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { - // TODO(peter): Unsure if this is a legit case or not. - return nil, fmt.Errorf("subinclude from within same package of a subrepo") - } - if _, err := state.Parse(sl); err != nil { - return nil, err - } - } - // TODO(peter): can we drop the dependent here? - return parse.Parse(state, label, label) + return r.Parse(ctx, label) } - // parseTarget returns a particular build target, parsing the build file along the way if necessary. - parseTarget := func(label core.BuildLabel) (*core.BuildTarget, error) { - if target := state.Graph.Target(label); target != nil { - return target, nil - } - pkg, err := state.Parse(label) - if err != nil { - return nil, err - } - if target := pkg.Target(label.Name); target != nil { - return target, nil - } - return nil, fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) + // Register the preloaded targets with the parser + if err := registerPreloads(state); err != nil { + return err } - // resolveTarget resolves a target, dealing with require/provide as needed. - resolveTarget := func(label core.BuildLabel, dependent *core.BuildTarget) iter.Seq2[*core.BuildTarget, error] { - return func(yield func(*core.BuildTarget, error) bool) { - target, err := parseTarget(label) - if err != nil { - yield(nil, err) - return - } - // TODO(peter): We might want the minor optimisation here to avoid creating a slice in the common case - provided := target.ProvideFor(dependent) - if len(provided) == 1 && provided[0] == target.Label { - yield(target, nil) - return - } - // TODO(peter): Would parallelism here be useful? - for _, p := range provided { - if !yield(parseTarget(p)) { - break - } - } - } + // Start looking for the initial targets to kick the build off + if err := r.FindOriginalTasks(ctx, preTargets, targets); err != nil { + return err } - - buildDep := func(dep core.BuildLabel, target *core.BuildTarget) error { - for t, err := range resolveTarget(dep, target) { - if err != nil { - return err - } - if _, err := state.Build(t.Label); err != nil { - return err - } - } - return nil + if err := g.Wait(); err != nil { + return err + } + if state.Cache != nil { + state.Cache.Shutdown() } + if state.RemoteClient != nil { + _, _, in, out := state.RemoteClient.DataRate() + log.Info("Total remote RPC data in: %d out: %d", in, out) + } + state.CloseResults() + metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) + return nil +} - reallyBuild := func(target *core.BuildTarget) error { - // TODO(peterebden): want to restructure how all this stuff sits on build targets as well - g, _ := errgroup.WithContext(ctx) - for dep := range target.BuildDependencyLabels() { - g.Go(func() error { - return buildDep(dep, target) - }) - } - for _, src := range target.AllSources() { - if l, ok := src.Label(); ok { - g.Go(func() error { - return buildDep(l, target) - }) - } +// RunHost is a convenience function that uses the host architecture, the given state's +// configuration and no pre targets. It is otherwise identical to Run. +func RunHost(targets []core.BuildLabel, state *core.BuildState) { + Run(targets, nil, state, &Progress{}, cli.HostArch()) +} + +type runner struct { + tasks *errgroup.Group + state *core.BuildState + arch cli.Arch + progress *Progress + buildOnce *cmap.ErrMap[core.BuildLabel, *core.BuildTarget] + localLimiter limiter + remoteLimiter limiter + anyRemote bool +} + +// Parse parses for a target. It can be called more than once for the same build label. +func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Package, error) { + if pkg, err := r.state.Graph.PackageOrWait(ctx, label); err != nil || pkg != nil { + return pkg, err + } + // If we get here then we have to parse it (we only get here if we are the first one) + r.progress.numParsing.Add(1) + defer r.progress.numParsing.Add(-1) + // If the target defines a subrepo, we must make sure that is built first. + if label.Subrepo != "" { + sl := label.SubrepoLabel(r.state) + if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { + // TODO(peter): Unsure if this is a legit case or not. + return nil, fmt.Errorf("subinclude from within same package of a subrepo") } - if err := g.Wait(); err != nil { - return err + if _, err := r.state.Parse(sl); err != nil { + return nil, err } + } + // TODO(peter): can we drop the dependent here? + return parse.Parse(r.state, label, label) +} - // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. - - // Now we are ready to build this target. Grab a thread and get started. - remote := anyRemote && !target.Local - limiter := limiter(remote) - limiter.Acquire() - defer limiter.Release() - return build.Build(state, target, remote) +func (r *runner) parseTarget(ctx context.Context, label core.BuildLabel) (*core.BuildTarget, error) { + if target := r.state.Graph.Target(label); target != nil { + return target, nil } + pkg, err := r.Parse(ctx, label) + if err != nil { + return nil, err + } + if target := pkg.Target(label.Name); target != nil { + return target, nil + } + return nil, fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) +} - buildAll := func(label core.BuildLabel) error { - pkg, err := state.Parse(label) +// resolveTarget resolves a target, dealing with require/provide as needed. +func (r *runner) resolveTarget(ctx context.Context, label core.BuildLabel, dependent *core.BuildTarget) iter.Seq2[*core.BuildTarget, error] { + return func(yield func(*core.BuildTarget, error) bool) { + target, err := r.parseTarget(ctx, label) if err != nil { - return err + yield(nil, err) + return } - g, _ := errgroup.WithContext(ctx) - for _, target := range pkg.AllTargets() { - g.Go(func() error { - return reallyBuild(target) - }) + // TODO(peter): We might want the minor optimisation here to avoid creating a slice in the common case + provided := target.ProvideFor(dependent) + if len(provided) == 1 && provided[0] == target.Label { + yield(target, nil) + return } - return g.Wait() - } - - // We use this to ensure that we only build each target exactly once. - buildOnce := cmap.NewErrMap[core.BuildLabel, *core.BuildTarget](cmap.DefaultShardCount, func(l core.BuildLabel) uint64 { - return cmap.XXHashes(l.Subrepo, l.PackageName, l.Name) - }, nil) - - state.Build = func(label core.BuildLabel) (_ *core.BuildTarget, err error) { - return buildOnce.GetOrSet(label, func() (*core.BuildTarget, error) { - if label.IsAllTargets() { - return nil, buildAll(label) - } - progress.numTotal.Add(1) - defer progress.numDone.Add(1) - target, err := parseTarget(label) - if err != nil { - return nil, err + // TODO(peter): Would parallelism here be useful? + for _, p := range provided { + if !yield(r.parseTarget(ctx, p)) { + break } - return target, reallyBuild(target) - }) + } } +} - state.Test = func(label core.BuildLabel) error { - progress.numTotal.Add(int64(state.NumTestRuns)) - target, err := parseTarget(label) +// buildDep builds a single dependency of a target (which might of course turn into multiple when resolved) +func (r *runner) buildDep(ctx context.Context, dep core.BuildLabel, target *core.BuildTarget) error { + for t, err := range r.resolveTarget(ctx, dep, target) { if err != nil { return err } - g, _ := errgroup.WithContext(ctx) - g.Go(func() error { - _, err := state.Build(label) + if _, err := r.Build(ctx, t.Label); err != nil { return err + } + } + return nil +} + +// buildOne builds a single target (which cannot be a pseudo-label like :all) +func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { + g, ctx := errgroup.WithContext(ctx) + for dep := range target.BuildDependencyLabels() { + g.Go(func() error { + return r.buildDep(ctx, dep, target) }) - for dep := range target.RuntimeDependencies() { + } + for _, src := range target.AllSources() { + if l, ok := src.Label(); ok { g.Go(func() error { - _, err := state.Build(dep) - return err + return r.buildDep(ctx, l, target) }) } - if err := g.Wait(); err != nil { - return err - } - // Now we're ready to test this target. - // TODO(peter): Is it okay for none of these to return errors? I _think_ so and we will capture it later? - remote := anyRemote && !target.Local - limiter := limiter(remote) - if state.TestSequentially || state.NumTestRuns == 1 { // minor optimisation to avoid creating unnecessary goroutines - limiter.Acquire() - defer limiter.Release() - for run := range int(state.NumTestRuns) { - test.Test(state, target, remote, run+1) - progress.numDone.Add(1) - } - return nil - } - var wg sync.WaitGroup - for run := range int(state.NumTestRuns) { - wg.Go(func() { - limiter.Acquire() - defer limiter.Release() - test.Test(state, target, remote, run+1) - progress.numDone.Add(1) - }) - } - wg.Wait() - return nil + } + if err := g.Wait(); err != nil { + return err } - // Register the preloaded targets with the parser - if err := registerPreloads(state); err != nil { + // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. + + // Now we are ready to build this target. Grab a thread and get started. + remote := r.anyRemote && !target.Local + limiter := r.limiter(remote) + limiter.Acquire() + defer limiter.Release() + return build.Build(r.state, target, remote) +} + +// buildAll builds all the targets specified by the given label (which can be :all, but can't be ...). +func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { + pkg, err := r.state.Parse(label) + if err != nil { return err } + g, ctx := errgroup.WithContext(ctx) + for _, target := range pkg.AllTargets() { + g.Go(func() error { + return r.buildOne(ctx, target) + }) + } + return g.Wait() +} - // Start looking for the initial targets to kick the build off - tf := taskFinder{ - state: state, - arch: arch, - tasks: g, +// Build is the main entrypoint to build a label +func (r *runner) Build(ctx context.Context, label core.BuildLabel) (_ *core.BuildTarget, err error) { + return r.buildOnce.GetOrSet(label, func() (*core.BuildTarget, error) { + if label.IsAllTargets() { + return nil, r.buildAll(ctx, label) + } + r.progress.numTotal.Add(1) + defer r.progress.numDone.Add(1) + target, err := r.parseTarget(ctx, label) + if err != nil { + return nil, err + } + return target, r.buildOne(ctx, target) + }) +} + +// Test is the main entrypoint to run tests for a label +func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { + r.progress.numTotal.Add(int64(r.state.NumTestRuns)) + target, err := r.parseTarget(ctx, label) + if err != nil { + return err } - if err := tf.FindOriginalTasks(preTargets, targets); err != nil { + g, ctx := errgroup.WithContext(ctx) + g.Go(func() error { + _, err := r.Build(ctx, label) return err + }) + for dep := range target.RuntimeDependencies() { + g.Go(func() error { + _, err := r.Build(ctx, dep) + return err + }) } if err := g.Wait(); err != nil { return err } - if state.Cache != nil { - state.Cache.Shutdown() + // Now we're ready to test this target. + // TODO(peter): Is it okay for none of these to return errors? I _think_ so and we will capture it later? + remote := r.anyRemote && !target.Local + limiter := r.limiter(remote) + if r.state.TestSequentially || r.state.NumTestRuns == 1 { // minor optimisation to avoid creating unnecessary goroutines + limiter.Acquire() + defer limiter.Release() + for run := range int(r.state.NumTestRuns) { + test.Test(r.state, target, remote, run+1) + r.progress.numDone.Add(1) + } + return nil } - if state.RemoteClient != nil { - _, _, in, out := state.RemoteClient.DataRate() - log.Info("Total remote RPC data in: %d out: %d", in, out) + var wg sync.WaitGroup + for run := range int(r.state.NumTestRuns) { + wg.Go(func() { + limiter.Acquire() + defer limiter.Release() + test.Test(r.state, target, remote, run+1) + r.progress.numDone.Add(1) + }) } - state.CloseResults() - metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) + wg.Wait() return nil } -// RunHost is a convenience function that uses the host architecture, the given state's -// configuration and no pre targets. It is otherwise identical to Run. -func RunHost(targets []core.BuildLabel, state *core.BuildState) { - Run(targets, nil, state, &Progress{}, cli.HostArch()) -} - -type taskFinder struct { - tasks *errgroup.Group - state *core.BuildState - arch cli.Arch +// limiter returns either a local or remote limiter that ensures we don't build too many things at once. +func (r *runner) limiter(remote bool) limiter { + if remote { + return r.remoteLimiter + } + return r.localLimiter } // findOriginalTasks finds the original parse tasks for the original set of targets. -func (tf *taskFinder) FindOriginalTasks(preTargets, targets []core.BuildLabel) error { +func (r *runner) FindOriginalTasks(ctx context.Context, preTargets, targets []core.BuildLabel) error { log.Debug("Original target scan beginning...") - if tf.state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { + if r.state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { // We have to parse the WORKSPACE file before anything else to understand subrepos. // This is a bit crap really since it inhibits parallelism for the first step. - if _, err := tf.state.Parse(core.NewBuildLabel("workspace", "all")); err != nil { + if _, err := r.state.Parse(core.NewBuildLabel("workspace", "all")); err != nil { return err } } - if tf.arch.Arch != "" && tf.arch != cli.HostArch() { + if r.arch.Arch != "" && r.arch != cli.HostArch() { // Set up a new subrepo for this architecture. - tf.state.Graph.AddSubrepo(core.SubrepoForArch(tf.state, tf.arch)) + r.state.Graph.AddSubrepo(core.SubrepoForArch(r.state, r.arch)) } if len(preTargets) > 0 { - tf.findOriginalTaskSet(preTargets, false, true) - if err := tf.tasks.Wait(); err != nil { + r.findOriginalTaskSet(ctx, preTargets, false, true) + if err := r.tasks.Wait(); err != nil { return err } - tf.tasks = &errgroup.Group{} + r.tasks = &errgroup.Group{} } - tf.findOriginalTaskSet(targets, tf.state.NeedTests, tf.state.NeedBuild) - if tf.state.NeedDebugDeps { + r.findOriginalTaskSet(ctx, targets, r.state.NeedTests, r.state.NeedBuild) + if r.state.NeedDebugDeps { if len(targets) != 1 { return fmt.Errorf("expected exactly 1 target in debug mode; got %d", len(targets)) } - tf.tasks.Go(func() error { - return tf.queueTargetsForDebug(targets[0]) + r.tasks.Go(func() error { + return r.queueTargetsForDebug(ctx, targets[0]) }) } - if err := tf.tasks.Wait(); err != nil { + if err := r.tasks.Wait(); err != nil { return err } log.Debug("Original target scan complete") return nil } -func (tf *taskFinder) findOriginalTaskSet(targets []core.BuildLabel, needTest, needBuild bool) { +func (r *runner) findOriginalTaskSet(ctx context.Context, targets []core.BuildLabel, needTest, needBuild bool) { for _, target := range ReadStdinLabels(targets) { - tf.tasks.Go(func() error { - return tf.findOriginalTask(target, needTest, needBuild) + r.tasks.Go(func() error { + return r.findOriginalTask(ctx, target, needTest, needBuild) }) } } -func (tf *taskFinder) queueTargetsForDebug(target core.BuildLabel) error { - if _, err := tf.state.Parse(target); err != nil { +func (r *runner) queueTargetsForDebug(ctx context.Context, target core.BuildLabel) error { + if _, err := r.Parse(ctx, target); err != nil { return err } - t := tf.state.Graph.TargetOrDie(target) + t := r.state.Graph.TargetOrDie(target) for _, tool := range t.AllDebugTools() { if l, ok := tool.Label(); ok { - tf.findOriginalTask(l, false, true) + r.findOriginalTask(ctx, l, false, true) } } for _, data := range t.AllDebugData() { if l, ok := data.Label(); ok { - tf.findOriginalTask(l, false, true) + r.findOriginalTask(ctx, l, false, true) } } return nil @@ -361,13 +377,13 @@ func stripHostRepoName(config *core.Configuration, label core.BuildLabel) core.B return label } -func (tf *taskFinder) findOriginalTask(target core.BuildLabel, needTest, needBuild bool) error { - if tf.arch != cli.HostArch() { - target = core.LabelToArch(target, tf.arch) +func (r *runner) findOriginalTask(ctx context.Context, target core.BuildLabel, needTest, needBuild bool) error { + if r.arch != cli.HostArch() { + target = core.LabelToArch(target, r.arch) } - target = stripHostRepoName(tf.state.Config, target) + target = stripHostRepoName(r.state.Config, target) if !target.IsAllSubpackages() { - tf.queueTask(target, needTest, needBuild) + r.queueTask(ctx, target, needTest, needBuild) return nil } // Any command-line labels with subrepos and ... require us to know where they are in order to @@ -375,40 +391,40 @@ func (tf *taskFinder) findOriginalTask(target core.BuildLabel, needTest, needBui dir := target.PackageName prefix := "" if target.Subrepo != "" { - subrepoLabel := target.SubrepoLabel(tf.state) - if target, err := tf.state.Build(subrepoLabel); err != nil { + subrepoLabel := target.SubrepoLabel(r.state) + if target, err := r.Build(ctx, subrepoLabel); err != nil { return err - } else if err := tf.state.EnsureDownloaded(target); err != nil { + } else if err := r.state.EnsureDownloaded(target); err != nil { return err } // Targets now get activated during parsing, so can be built before we finish parsing their package. - pkg, err := tf.state.Parse(subrepoLabel) + pkg, err := r.Parse(ctx, subrepoLabel) if err != nil { return err } dir = pkg.Subrepo.Dir(dir) prefix = pkg.Subrepo.Dir(prefix) } - for filename := range FindAllBuildFiles(tf.state.Config, dir, "") { + for filename := range FindAllBuildFiles(r.state.Config, dir, "") { dirname, _ := filepath.Split(filename) l := core.NewBuildLabel(strings.TrimLeft(strings.TrimPrefix(strings.TrimRight(dirname, "/"), prefix), "/"), "all") l.Subrepo = target.Subrepo - tf.queueTask(l, needTest, needBuild) + r.queueTask(ctx, l, needTest, needBuild) } return nil } -func (tf *taskFinder) queueTask(target core.BuildLabel, needTest, needBuild bool) { - tf.tasks.Go(func() error { - tf.state.AddOriginalTarget(target) +func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest, needBuild bool) { + r.tasks.Go(func() error { + r.state.AddOriginalTarget(target) if needTest { - return tf.state.Test(target) + return r.Test(ctx, target) } else if needBuild { - _, err := tf.state.Build(target) + _, err := r.Build(ctx, target) // TODO(peter): Ensure this gets downloaded if needed return err } - _, err := tf.state.Parse(target) + _, err := r.Parse(ctx, target) return err }) } From dbdda4ce6afe272243f908c3a1a88bb08aa5bd28 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 17:13:46 +0100 Subject: [PATCH 03/44] Sort out some error stuff to filter spurious errors --- src/core/state.go | 5 +++++ src/parse/asp/builtins.go | 4 ++-- src/parse/asp/errors.go | 5 +++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/core/state.go b/src/core/state.go index e926496de4..fcfdc84780 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -2,8 +2,10 @@ package core import ( "bytes" + "context" "crypto/sha1" "crypto/sha256" + "errors" "fmt" "hash" "hash/crc32" @@ -463,6 +465,9 @@ func (state *BuildState) LogBuildError(label BuildLabel, status BuildResultStatu // logResult logs a build result directly to the state's queue. func (state *BuildState) logResult(result *BuildResult) { + if result.Err != nil && errors.Is(result.Err, context.Canceled) { + return + } result.Time = time.Now() state.progress.internalResults <- result if result.Status.IsFailure() { diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 589bbf89c6..8a5944e774 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -382,7 +382,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget { Name: "all", } if _, err := s.state.Parse(subrepoPackageLabel); err != nil { - s.Error("Failed to parse subrepo target: %v", err) + s.Error("Failed to parse subrepo target: %w", err) } } @@ -398,7 +398,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget { } t, err := s.WaitForSubincludedTarget(l, pkgLabel) if err != nil { - s.Error("Failed to build subincluded target: %v", err) + s.Error("Failed to build subincluded target: %w", err) } else if s.pkg != nil { s.pkg.RegisterSubinclude(l) } else if s.subincludeLabel != nil { // If this is nil, that indicates a preloadedSubinclude diff --git a/src/parse/asp/errors.go b/src/parse/asp/errors.go index 1d458f0d40..148988d458 100644 --- a/src/parse/asp/errors.go +++ b/src/parse/asp/errors.go @@ -94,6 +94,11 @@ func (stack *errorStack) ShortError() string { return stack.err.Error() } +// Unwrap implements the errors interface so this can be unwrapped to get the contained error +func (stack *errorStack) Unwrap() error { + return stack.err +} + // stackTrace returns the lines of stacktrace from the error. func (stack *errorStack) stackTrace() string { ret := make([]string, len(stack.Stack)) From 2617f457a0384e773aa982aa1f2c98f271c05ee3 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 17:31:14 +0100 Subject: [PATCH 04/44] Support :all targets for test --- src/plz/plz.go | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 8d98b34a38..7a7320d7b1 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -121,7 +121,7 @@ func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Packag // TODO(peter): Unsure if this is a legit case or not. return nil, fmt.Errorf("subinclude from within same package of a subrepo") } - if _, err := r.state.Parse(sl); err != nil { + if _, err := r.Parse(ctx, sl); err != nil { return nil, err } } @@ -210,7 +210,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { // buildAll builds all the targets specified by the given label (which can be :all, but can't be ...). func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { - pkg, err := r.state.Parse(label) + pkg, err := r.Parse(ctx, label) if err != nil { return err } @@ -239,16 +239,12 @@ func (r *runner) Build(ctx context.Context, label core.BuildLabel) (_ *core.Buil }) } -// Test is the main entrypoint to run tests for a label -func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { +// testOne tests one single target +func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { r.progress.numTotal.Add(int64(r.state.NumTestRuns)) - target, err := r.parseTarget(ctx, label) - if err != nil { - return err - } g, ctx := errgroup.WithContext(ctx) g.Go(func() error { - _, err := r.Build(ctx, label) + _, err := r.Build(ctx, target.Label) return err }) for dep := range target.RuntimeDependencies() { @@ -286,6 +282,31 @@ func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { return nil } +// Test is the main entrypoint to run tests for a label +func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { + if !label.IsAllTargets() { + target, err := r.parseTarget(ctx, label) + if err != nil || !target.IsTest() { + return err + } + return r.testOne(ctx, target) + } + pkg, err := r.Parse(ctx, label) + if err != nil { + return err + } + g, ctx := errgroup.WithContext(ctx) + for _, target := range pkg.AllTargets() { + if target.IsTest() { + g.Go(func() error { + return r.testOne(ctx, target) + }) + } + } + return g.Wait() + +} + // limiter returns either a local or remote limiter that ensures we don't build too many things at once. func (r *runner) limiter(remote bool) limiter { if remote { @@ -300,7 +321,7 @@ func (r *runner) FindOriginalTasks(ctx context.Context, preTargets, targets []co if r.state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { // We have to parse the WORKSPACE file before anything else to understand subrepos. // This is a bit crap really since it inhibits parallelism for the first step. - if _, err := r.state.Parse(core.NewBuildLabel("workspace", "all")); err != nil { + if _, err := r.Parse(ctx, core.NewBuildLabel("workspace", "all")); err != nil { return err } } @@ -324,9 +345,6 @@ func (r *runner) FindOriginalTasks(ctx context.Context, preTargets, targets []co return r.queueTargetsForDebug(ctx, targets[0]) }) } - if err := r.tasks.Wait(); err != nil { - return err - } log.Debug("Original target scan complete") return nil } From 71ffa4c5321d90ada05410380e8f8814aed7d84e Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 17:44:29 +0100 Subject: [PATCH 05/44] Clean up errgroup / context around pre-targets --- src/plz/plz.go | 68 ++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 7a7320d7b1..c2f1b59304 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -70,10 +70,36 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress return err } - // Start looking for the initial targets to kick the build off - if err := r.FindOriginalTasks(ctx, preTargets, targets); err != nil { - return err + if state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { + // We have to parse the WORKSPACE file before anything else to understand subrepos. + // This is a bit crap really since it inhibits parallelism for the first step. + if _, err := r.Parse(ctx, core.NewBuildLabel("workspace", "all")); err != nil { + return err + } } + if arch.Arch != "" && arch != cli.HostArch() { + // Set up a new subrepo for this architecture. + state.Graph.AddSubrepo(core.SubrepoForArch(state, arch)) + } + if len(preTargets) > 0 { + r.FindOriginalTaskSet(ctx, preTargets, false, true) + if err := g.Wait(); err != nil { + return err + } + // Reset the group & context for next time + g, ctx = errgroup.WithContext(context.Background()) + r.tasks = g + } + r.FindOriginalTaskSet(ctx, targets, r.state.NeedTests, r.state.NeedBuild) + if state.NeedDebugDeps { + if len(targets) != 1 { + return fmt.Errorf("expected exactly 1 target in debug mode; got %d", len(targets)) + } + g.Go(func() error { + return r.queueTargetsForDebug(ctx, targets[0]) + }) + } + if err := g.Wait(); err != nil { return err } @@ -315,41 +341,7 @@ func (r *runner) limiter(remote bool) limiter { return r.localLimiter } -// findOriginalTasks finds the original parse tasks for the original set of targets. -func (r *runner) FindOriginalTasks(ctx context.Context, preTargets, targets []core.BuildLabel) error { - log.Debug("Original target scan beginning...") - if r.state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { - // We have to parse the WORKSPACE file before anything else to understand subrepos. - // This is a bit crap really since it inhibits parallelism for the first step. - if _, err := r.Parse(ctx, core.NewBuildLabel("workspace", "all")); err != nil { - return err - } - } - if r.arch.Arch != "" && r.arch != cli.HostArch() { - // Set up a new subrepo for this architecture. - r.state.Graph.AddSubrepo(core.SubrepoForArch(r.state, r.arch)) - } - if len(preTargets) > 0 { - r.findOriginalTaskSet(ctx, preTargets, false, true) - if err := r.tasks.Wait(); err != nil { - return err - } - r.tasks = &errgroup.Group{} - } - r.findOriginalTaskSet(ctx, targets, r.state.NeedTests, r.state.NeedBuild) - if r.state.NeedDebugDeps { - if len(targets) != 1 { - return fmt.Errorf("expected exactly 1 target in debug mode; got %d", len(targets)) - } - r.tasks.Go(func() error { - return r.queueTargetsForDebug(ctx, targets[0]) - }) - } - log.Debug("Original target scan complete") - return nil -} - -func (r *runner) findOriginalTaskSet(ctx context.Context, targets []core.BuildLabel, needTest, needBuild bool) { +func (r *runner) FindOriginalTaskSet(ctx context.Context, targets []core.BuildLabel, needTest, needBuild bool) { for _, target := range ReadStdinLabels(targets) { r.tasks.Go(func() error { return r.findOriginalTask(ctx, target, needTest, needBuild) From 195d21c32a6c3ea41d9a26a90879342d897c61c9 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 18:49:33 +0100 Subject: [PATCH 06/44] Defer shutting down the state. I swear I had this on a branch somewhere... --- src/plz/plz.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index c2f1b59304..68afa2b6f2 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -42,6 +42,20 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress parse.InitParser(state) + // This must happen however we exit; anything reading state.Results() (e.g. the display) + // waits for that channel to be closed, so it would hang forever if we returned an error first. + defer func() { + if state.Cache != nil { + state.Cache.Shutdown() + } + if state.RemoteClient != nil { + _, _, in, out := state.RemoteClient.DataRate() + log.Info("Total remote RPC data in: %d out: %d", in, out) + } + state.CloseResults() + metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) + }() + g, ctx := errgroup.WithContext(context.Background()) r := runner{ @@ -100,19 +114,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress }) } - if err := g.Wait(); err != nil { - return err - } - if state.Cache != nil { - state.Cache.Shutdown() - } - if state.RemoteClient != nil { - _, _, in, out := state.RemoteClient.DataRate() - log.Info("Total remote RPC data in: %d out: %d", in, out) - } - state.CloseResults() - metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) - return nil + return g.Wait() } // RunHost is a convenience function that uses the host architecture, the given state's From 7892d39538e4cd99670cac749ae7573ba96fad97 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 18:55:16 +0100 Subject: [PATCH 07/44] Log a build error at this point --- src/plz/plz.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 68afa2b6f2..9636ee492d 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -168,7 +168,9 @@ func (r *runner) parseTarget(ctx context.Context, label core.BuildLabel) (*core. if target := pkg.Target(label.Name); target != nil { return target, nil } - return nil, fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) + err = fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) + r.state.LogBuildError(label, core.ParseFailed, err, "%s", err) + return nil, err } // resolveTarget resolves a target, dealing with require/provide as needed. From 49f0a66dbb2d1e5dc25177718fcc36256880cf2a Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Mon, 3 Aug 2026 19:05:21 +0100 Subject: [PATCH 08/44] Be a bit more resilient in these places --- src/test/coverage.go | 4 +++- src/test/surefire.go | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/test/coverage.go b/src/test/coverage.go index b52870f0d9..5bd8ef1012 100644 --- a/src/test/coverage.go +++ b/src/test/coverage.go @@ -59,7 +59,9 @@ func collectCoverageFiles(state *core.BuildState, includeAllFiles bool) map[stri doneTargets := map[*core.BuildTarget]bool{} coverageFiles := map[string]bool{} for _, label := range state.ExpandAllOriginalLabels() { - collectAllFiles(state, state.Graph.TargetOrDie(label), coverageFiles, includeAllFiles, true, doneTargets) + if target := state.Graph.Target(label); target != nil { // It won't be if it failed to parse + collectAllFiles(state, target, coverageFiles, includeAllFiles, true, doneTargets) + } } return coverageFiles } diff --git a/src/test/surefire.go b/src/test/surefire.go index dac7275552..5c574f613e 100644 --- a/src/test/surefire.go +++ b/src/test/surefire.go @@ -11,7 +11,10 @@ import ( // CopySurefireXMLFilesToDir copies all the XML test results files into the given directory. func CopySurefireXMLFilesToDir(state *core.BuildState, surefireDir string) { for _, label := range state.ExpandOriginalLabels() { - target := state.Graph.TargetOrDie(label) + target := state.Graph.Target(label) + if target == nil { + continue // The target failed to parse, so there's nothing to copy for it. + } if state.ShouldInclude(target) && target.IsTest() && !target.Test.NoOutput { copySurefireXMLtoDir(target.TestResultsFile(), surefireDir) } From e0135f3923a1d0574ebbfa7ccdb3b61ff0de3de9 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 4 Aug 2026 09:49:36 +0100 Subject: [PATCH 09/44] not all deps might be ready here --- src/core/build_target.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index 675c98b6ee..938f772b23 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -1003,14 +1003,15 @@ func (target *BuildTarget) CanSee(state *BuildState, dep *BuildTarget) bool { // Returns an error if not, or nil if all's well. func (target *BuildTarget) CheckDependencyVisibility(state *BuildState) error { for _, d := range target.dependencies { - dep := state.Graph.TargetOrDie(d.Label) - if !target.CanSee(state, dep) { - return fmt.Errorf("Target %s isn't visible to %s", dep.Label, target.Label) - } else if dep.TestOnly && !target.IsTest() && !target.TestOnly { - if target.Label.isExperimental(state) { - log.Info("Test-only restrictions suppressed for %s since %s is in the experimental tree", dep.Label, target.Label) - } else { - return fmt.Errorf("Target %s can't depend on %s, it's marked test_only", target.Label, dep.Label) + if dep := state.Graph.Target(d.Label); dep != nil { + if !target.CanSee(state, dep) { + return fmt.Errorf("Target %s isn't visible to %s", dep.Label, target.Label) + } else if dep.TestOnly && !target.IsTest() && !target.TestOnly { + if target.Label.isExperimental(state) { + log.Info("Test-only restrictions suppressed for %s since %s is in the experimental tree", dep.Label, target.Label) + } else { + return fmt.Errorf("Target %s can't depend on %s, it's marked test_only", target.Label, dep.Label) + } } } } From 87377be7cb95d77868d808e5f06af01936a0b8fd Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 4 Aug 2026 09:55:15 +0100 Subject: [PATCH 10/44] um I think this is right? --- src/core/build_target.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index 938f772b23..bc0f24ce74 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -684,7 +684,7 @@ func (target *BuildTarget) RuntimeDependencies() iter.Seq[BuildLabel] { target.mutex.RLock() defer target.mutex.RUnlock() for _, deps := range target.dependencies { - if deps.Runtime { + if deps.Runtime || deps.Data { if !yield(deps.Label) { break } From 521362c6aa153eae36d6849027f2856a8ccd5b6e Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 4 Aug 2026 12:47:56 +0100 Subject: [PATCH 11/44] Need to check for arch subrepos here now --- src/plz/plz.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 9636ee492d..00ab0c39b9 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -144,13 +144,15 @@ func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Packag defer r.progress.numParsing.Add(-1) // If the target defines a subrepo, we must make sure that is built first. if label.Subrepo != "" { - sl := label.SubrepoLabel(r.state) - if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { - // TODO(peter): Unsure if this is a legit case or not. - return nil, fmt.Errorf("subinclude from within same package of a subrepo") - } - if _, err := r.Parse(ctx, sl); err != nil { - return nil, err + if r.state.CheckArchSubrepo(label.Subrepo) == nil { + sl := label.SubrepoLabel(r.state) + if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { + // TODO(peter): Unsure if this is a legit case or not. + return nil, fmt.Errorf("subinclude from within same package of a subrepo") + } + if _, err := r.Parse(ctx, sl); err != nil { + return nil, err + } } } // TODO(peter): can we drop the dependent here? From 3141b87c0b914e930c868404ac09af79b973c38a Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 09:49:48 +0100 Subject: [PATCH 12/44] Path seems to be simpler --- test/query_somepath_test.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/test/query_somepath_test.txt b/test/query_somepath_test.txt index 5b22f3f28c..6fbdfad7ff 100644 --- a/test/query_somepath_test.txt +++ b/test/query_somepath_test.txt @@ -1,4 +1,3 @@ Found path: //src:please - //src/audit:audit //src/fs:fs From 308fb1e968ce4ec09c51f26c9ab7f4732685183d Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 09:55:08 +0100 Subject: [PATCH 13/44] Still need to build targets even if they are not tests --- src/plz/plz.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 00ab0c39b9..08a8367402 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -288,6 +288,9 @@ func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { if err := g.Wait(); err != nil { return err } + if !target.IsTest() { + return nil + } // Now we're ready to test this target. // TODO(peter): Is it okay for none of these to return errors? I _think_ so and we will capture it later? remote := r.anyRemote && !target.Local @@ -318,7 +321,7 @@ func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { if !label.IsAllTargets() { target, err := r.parseTarget(ctx, label) - if err != nil || !target.IsTest() { + if err != nil { return err } return r.testOne(ctx, target) @@ -329,11 +332,9 @@ func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { } g, ctx := errgroup.WithContext(ctx) for _, target := range pkg.AllTargets() { - if target.IsTest() { - g.Go(func() error { - return r.testOne(ctx, target) - }) - } + g.Go(func() error { + return r.testOne(ctx, target) + }) } return g.Wait() From d78ebcdf7a31a8b335621b32e40170af2118a35d Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 10:00:01 +0100 Subject: [PATCH 14/44] Honour labels correctly --- src/plz/plz.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 08a8367402..2d4c5bcd15 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -248,9 +248,11 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { } g, ctx := errgroup.WithContext(ctx) for _, target := range pkg.AllTargets() { - g.Go(func() error { - return r.buildOne(ctx, target) - }) + if r.state.ShouldInclude(target) { + g.Go(func() error { + return r.buildOne(ctx, target) + }) + } } return g.Wait() } @@ -332,9 +334,11 @@ func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { } g, ctx := errgroup.WithContext(ctx) for _, target := range pkg.AllTargets() { - g.Go(func() error { - return r.testOne(ctx, target) - }) + if r.state.ShouldInclude(target) { + g.Go(func() error { + return r.testOne(ctx, target) + }) + } } return g.Wait() From c4b3420cdf5ea3c676217f72b45963983cc618fc Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 10:10:18 +0100 Subject: [PATCH 15/44] Need to parse recursively for queries etc --- src/plz/plz.go | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 2d4c5bcd15..ccac39ca5b 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -159,6 +159,46 @@ func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Packag return parse.Parse(r.state, label, label) } +// RecursiveParse is like Parse but recurses down into all dependencies of the target as well. +func (r *runner) RecursiveParse(ctx context.Context, label core.BuildLabel) error { + if !label.IsAllTargets() { + return r.recursiveParse(ctx, label) + } + pkg, err := r.Parse(ctx, label) + if err != nil { + return err + } + // Deduplicate dependencies as early as we can here. + seen := map[core.BuildLabel]struct{}{} + g, ctx := errgroup.WithContext(ctx) + for _, target := range pkg.AllTargets() { + for dep := range target.DeclaredDependencies() { + if _, present := seen[dep]; !present { + seen[dep] = struct{}{} + g.Go(func() error { + return r.recursiveParse(ctx, dep) + }) + } + } + } + return g.Wait() +} + +func (r *runner) recursiveParse(ctx context.Context, label core.BuildLabel) error { + target, err := r.parseTarget(ctx, label) + if err != nil { + return err + } + g, ctx := errgroup.WithContext(ctx) + for dep := range target.DeclaredDependencies() { + g.Go(func() error { + _, err := r.Parse(ctx, dep) + return err + }) + } + return g.Wait() +} + func (r *runner) parseTarget(ctx context.Context, label core.BuildLabel) (*core.BuildTarget, error) { if target := r.state.Graph.Target(label); target != nil { return target, nil @@ -445,8 +485,7 @@ func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest // TODO(peter): Ensure this gets downloaded if needed return err } - _, err := r.Parse(ctx, target) - return err + return r.RecursiveParse(ctx, target) }) } From c962065ab3c142a40b7a10bf44f3571cf51f8da7 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 10:17:16 +0100 Subject: [PATCH 16/44] Hmmmm this might be unreliable --- test/query_somepath_test.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/query_somepath_test.txt b/test/query_somepath_test.txt index 6fbdfad7ff..5b22f3f28c 100644 --- a/test/query_somepath_test.txt +++ b/test/query_somepath_test.txt @@ -1,3 +1,4 @@ Found path: //src:please + //src/audit:audit //src/fs:fs From aa5e78017ad228fb714a8dbcf61d779af1656cea Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 10:42:47 +0100 Subject: [PATCH 17/44] Make cyclic dependencies cancel properly --- src/cmap/cerrmap.go | 30 ++++++++++++++++++++++++++++++ src/core/state.go | 4 +++- src/plz/plz.go | 6 ++++-- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/cmap/cerrmap.go b/src/cmap/cerrmap.go index 78c7dc8ed6..8ccf070ef4 100644 --- a/src/cmap/cerrmap.go +++ b/src/cmap/cerrmap.go @@ -1,5 +1,9 @@ package cmap +import ( + "context" +) + // A Limiter is the interface that we use to release/acquire workers while waiting. type Limiter interface { Acquire() @@ -79,6 +83,32 @@ func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) { return v.Val, v.Err } +// GetOrSetCtx is like GetOrSet but accepts a context that can be cancelled. +func (m *ErrMap[K, V]) GetOrSetCtx(ctx context.Context, key K, f func() (V, error)) (V, error) { + v, wait, first := m.m.GetOrWait(key) + if v.Err != nil { + return v.Val, v.Err + } else if first { + val, err := f() + m.m.Set(key, errV[V]{Val: val, Err: err}) + return val, err + } else if wait != nil { + if m.l != nil { + // Release the limiter for the duration we're waiting + m.l.Release() + defer m.l.Acquire() + } + select { + case <-wait: + return m.Get(key) + case <-ctx.Done(): + var v V + return v, ctx.Err() + } + } + return v.Val, v.Err +} + // Range calls f for each key-value pair in the map. // No particular consistency guarantees are made during iteration. func (m *ErrMap[K, V]) Range(f func(key K, val V)) { diff --git a/src/core/state.go b/src/core/state.go index fcfdc84780..60b6e97bc5 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -224,6 +224,8 @@ type BuildState struct { Build func(BuildLabel) (*BuildTarget, error) // Parse is a callback to parse a single package. It's also set from outside. Parse func(BuildLabel) (*Package, error) + // Cancel is a cancel function called when the state detects a cycle. + Cancel func() // initOnce is used to control loading the subrepo .plzconfig initOnce *sync.Once @@ -534,7 +536,7 @@ func (state *BuildState) forwardResults() { func (state *BuildState) checkForCycles() { if err := state.progress.cycleDetector.Check(); err != nil { state.LogBuildError(err.Cycle[0].Label, TargetBuildFailed, err, "") - // state.Stop() + state.Cancel() } } diff --git a/src/plz/plz.go b/src/plz/plz.go index ccac39ca5b..f69ca1d3b1 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -56,7 +56,9 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) }() - g, ctx := errgroup.WithContext(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) + g, ctx := errgroup.WithContext(ctx) + state.Cancel = cancel r := runner{ state: state, @@ -299,7 +301,7 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { // Build is the main entrypoint to build a label func (r *runner) Build(ctx context.Context, label core.BuildLabel) (_ *core.BuildTarget, err error) { - return r.buildOnce.GetOrSet(label, func() (*core.BuildTarget, error) { + return r.buildOnce.GetOrSetCtx(ctx, label, func() (*core.BuildTarget, error) { if label.IsAllTargets() { return nil, r.buildAll(ctx, label) } From a297ac1a603bcc7754a4bf746f49ee9a953e39b0 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Wed, 5 Aug 2026 11:27:34 +0100 Subject: [PATCH 18/44] We need to build runtime dependencies alongside the target itself --- src/core/graph.go | 3 ++- src/plz/plz.go | 35 ++++++++++++++++++++++------------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/core/graph.go b/src/core/graph.go index 9aa32abdbb..f22a9265e1 100644 --- a/src/core/graph.go +++ b/src/core/graph.go @@ -7,6 +7,7 @@ package core import ( "context" "maps" + "fmt" "slices" "sort" "sync" @@ -65,7 +66,7 @@ func (graph *BuildGraph) Target(label BuildLabel) *BuildTarget { func (graph *BuildGraph) TargetOrDie(label BuildLabel) *BuildTarget { target := graph.Target(label) if target == nil { - log.Fatalf("Target %s not found in build graph\n", label) + panic(fmt.Sprintf("Target %s not found in build graph\n", label)) } return target } diff --git a/src/plz/plz.go b/src/plz/plz.go index f69ca1d3b1..f0bd699a30 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "sync" + "slices" "sync/atomic" "github.com/peterebden/go-cli-init/v5/flags" @@ -274,7 +275,26 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. - // Now we are ready to build this target. Grab a thread and get started. + // Okay, now the runtime dependencies can happen in parallel with the target itself. + deps := slices.Collect(target.RuntimeDependencies()) + if len(deps) == 0 { + return r.buildJustOne(target) + } + g, ctx = errgroup.WithContext(ctx) + g.Go(func() error { + return r.buildJustOne(target) + }) + for _, dep := range deps { + g.Go(func() error { + _, err := r.Build(ctx, dep) + return err + }) + } + return g.Wait() +} + +// buildJustOne calls the build for a single target. +func (r *runner) buildJustOne(target *core.BuildTarget) error { remote := r.anyRemote && !target.Local limiter := r.limiter(remote) limiter.Acquire() @@ -318,18 +338,7 @@ func (r *runner) Build(ctx context.Context, label core.BuildLabel) (_ *core.Buil // testOne tests one single target func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { r.progress.numTotal.Add(int64(r.state.NumTestRuns)) - g, ctx := errgroup.WithContext(ctx) - g.Go(func() error { - _, err := r.Build(ctx, target.Label) - return err - }) - for dep := range target.RuntimeDependencies() { - g.Go(func() error { - _, err := r.Build(ctx, dep) - return err - }) - } - if err := g.Wait(); err != nil { + if _, err := r.Build(ctx, target.Label); err != nil { return err } if !target.IsTest() { From 09d63b2d4b45e78000f474086ba73cf37a47df6f Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Thu, 6 Aug 2026 13:34:15 +0100 Subject: [PATCH 19/44] unused --- src/parse/parse_step.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/parse/parse_step.go b/src/parse/parse_step.go index dd5749b2b5..d0d93a7938 100644 --- a/src/parse/parse_step.go +++ b/src/parse/parse_step.go @@ -80,10 +80,6 @@ func parse(state *core.BuildState, label, dependent core.BuildLabel) (*core.Pack return pkg, nil } -func inSamePackage(label, dependent core.BuildLabel) bool { - return !dependent.IsOriginalTarget() && label.Subrepo == dependent.Subrepo && label.PackageName == dependent.PackageName -} - // checkSubrepo checks if the label we're parsing is within a subrepo, returning that subrepo, if present in the label. // // The subrepo target can be inferred from the subrepo name using convention i.e. ///foo/bar//:baz has a subrepo label From 274d7b0bfdccca5a8e116e3797c2ce4af7d446f2 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Thu, 6 Aug 2026 18:05:44 +0100 Subject: [PATCH 20/44] fmt --- src/plz/plz.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index f0bd699a30..41b59c2d47 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -5,9 +5,9 @@ import ( "fmt" "iter" "path/filepath" + "slices" "strings" "sync" - "slices" "sync/atomic" "github.com/peterebden/go-cli-init/v5/flags" @@ -180,11 +180,11 @@ func (r *runner) RecursiveParse(ctx context.Context, label core.BuildLabel) erro seen[dep] = struct{}{} g.Go(func() error { return r.recursiveParse(ctx, dep) - }) + }) } } } - return g.Wait() + return g.Wait() } func (r *runner) recursiveParse(ctx context.Context, label core.BuildLabel) error { From 262b5d17b5bbd3fa54b5ae1d5666290d01ef39e1 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 10:14:27 +0100 Subject: [PATCH 21/44] Don't shadow ctx --- src/parse/asp/main/main.go | 2 +- src/plz/plz.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/parse/asp/main/main.go b/src/parse/asp/main/main.go index 03ff09ba6a..336dc6faab 100644 --- a/src/parse/asp/main/main.go +++ b/src/parse/asp/main/main.go @@ -63,7 +63,7 @@ func parseFile(pkg *core.Package, p *asp.Parser, filename string) error { } return err } - return p.ParseFile(pkg, nil, nil, 0, nil, filename) + return p.ParseFile(pkg, nil, nil, nil, filename) } type assignment struct { diff --git a/src/plz/plz.go b/src/plz/plz.go index 41b59c2d47..13dd42fc5e 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -256,16 +256,16 @@ func (r *runner) buildDep(ctx context.Context, dep core.BuildLabel, target *core // buildOne builds a single target (which cannot be a pseudo-label like :all) func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { - g, ctx := errgroup.WithContext(ctx) + g, gctx := errgroup.WithContext(ctx) for dep := range target.BuildDependencyLabels() { g.Go(func() error { - return r.buildDep(ctx, dep, target) + return r.buildDep(gctx, dep, target) }) } for _, src := range target.AllSources() { if l, ok := src.Label(); ok { g.Go(func() error { - return r.buildDep(ctx, l, target) + return r.buildDep(gctx, l, target) }) } } @@ -280,13 +280,13 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { if len(deps) == 0 { return r.buildJustOne(target) } - g, ctx = errgroup.WithContext(ctx) + g, gctx = errgroup.WithContext(ctx) g.Go(func() error { return r.buildJustOne(target) }) for _, dep := range deps { g.Go(func() error { - _, err := r.Build(ctx, dep) + _, err := r.Build(gctx, dep) return err }) } From b49d06eda42a071cc24421d170a32c99d1508500 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 10:20:07 +0100 Subject: [PATCH 22/44] Rebuild cancel ctx --- src/plz/plz.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 13dd42fc5e..342360d8f6 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -104,7 +104,9 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress return err } // Reset the group & context for next time - g, ctx = errgroup.WithContext(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) + g, ctx = errgroup.WithContext(ctx) + state.Cancel = cancel r.tasks = g } r.FindOriginalTaskSet(ctx, targets, r.state.NeedTests, r.state.NeedBuild) From 913b09022b882f8c8cb6a298a7359c9ea891ae42 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 11:22:33 +0100 Subject: [PATCH 23/44] Fix a few things --- src/core/build_target.go | 17 +++++++++++++++++ src/plz/plz.go | 32 ++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index bc0f24ce74..b0c8728a66 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -680,6 +680,23 @@ func (target *BuildTarget) BuildDependencies() iter.Seq[BuildLabel] { // Although run-time dependencies are transitive, RuntimeDependencies only returns this target's direct run-time // dependencies. Use IterAllRuntimeDependencies to iterate over the target's run-time dependencies transitively. func (target *BuildTarget) RuntimeDependencies() iter.Seq[BuildLabel] { + return func(yield func(BuildLabel) bool) { + target.mutex.RLock() + defer target.mutex.RUnlock() + for _, deps := range target.dependencies { + if deps.Runtime { + if !yield(deps.Label) { + break + } + } + } + } +} + +// RuntimeAndDataDependencies returns the direct run-time and data dependencies of this target, i.e. everything +// that has to be available when it's run or tested but not when it's built. +// N.B. This is not the same as RuntimeDependencies, which is what the target declared as runtime_deps. +func (target *BuildTarget) RuntimeAndDataDependencies() iter.Seq[BuildLabel] { return func(yield func(BuildLabel) bool) { target.mutex.RLock() defer target.mutex.RUnlock() diff --git a/src/plz/plz.go b/src/plz/plz.go index 342360d8f6..4df92a97ef 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -278,7 +278,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. // Okay, now the runtime dependencies can happen in parallel with the target itself. - deps := slices.Collect(target.RuntimeDependencies()) + deps := slices.Collect(target.RuntimeAndDataDependencies()) if len(deps) == 0 { return r.buildJustOne(target) } @@ -310,11 +310,14 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { if err != nil { return err } - g, ctx := errgroup.WithContext(ctx) + g, gctx := errgroup.WithContext(ctx) for _, target := range pkg.AllTargets() { if r.state.ShouldInclude(target) { g.Go(func() error { - return r.buildOne(ctx, target) + // N.B. This must go through Build, not buildOne, so we don't build a target twice + // if it's reached both via :all and as a dependency of something else. + _, err := r.Build(gctx, target.Label) + return err }) } } @@ -322,17 +325,22 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { } // Build is the main entrypoint to build a label -func (r *runner) Build(ctx context.Context, label core.BuildLabel) (_ *core.BuildTarget, err error) { - return r.buildOnce.GetOrSetCtx(ctx, label, func() (*core.BuildTarget, error) { - if label.IsAllTargets() { +func (r *runner) Build(ctx context.Context, label core.BuildLabel) (*core.BuildTarget, error) { + if label.IsAllTargets() { + return r.buildOnce.GetOrSetCtx(ctx, label, func() (*core.BuildTarget, error) { return nil, r.buildAll(ctx, label) - } + }) + } + // N.B. We must parse the target _before_ claiming its entry in buildOnce; parsing its package can + // re-enter here for the same label (e.g. a BUILD file that subincludes a target it defines + // earlier in the same file) and we'd then deadlock waiting on ourselves. + target, err := r.parseTarget(ctx, label) + if err != nil { + return nil, err + } + return r.buildOnce.GetOrSetCtx(ctx, label, func() (*core.BuildTarget, error) { r.progress.numTotal.Add(1) defer r.progress.numDone.Add(1) - target, err := r.parseTarget(ctx, label) - if err != nil { - return nil, err - } return target, r.buildOne(ctx, target) }) } @@ -489,8 +497,8 @@ func (r *runner) findOriginalTask(ctx context.Context, target core.BuildLabel, n } func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest, needBuild bool) { + r.state.AddOriginalTarget(target) r.tasks.Go(func() error { - r.state.AddOriginalTarget(target) if needTest { return r.Test(ctx, target) } else if needBuild { From a75c38af90f6929b1984e99744b1ad43f353acca Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 11:25:04 +0100 Subject: [PATCH 24/44] Pass in context --- src/plz/plz.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 4df92a97ef..7e961dbaea 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -83,7 +83,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress } // Register the preloaded targets with the parser - if err := registerPreloads(state); err != nil { + if err := r.RegisterPreloads(ctx, state); err != nil { return err } @@ -510,19 +510,19 @@ func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest }) } -// registerPreloads waits for all preloaded subinclude targets to be built, downloads them, and then registers them with +// RegisterPreloads waits for all preloaded subinclude targets to be built, downloads them, and then registers them with // the interpreter. We have to actually register them otherwise this will return before we build any // transitive subincludes. -func registerPreloads(state *core.BuildState) error { - var eg errgroup.Group +func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState) error { + g, ctx := errgroup.WithContext(ctx) for _, inc := range state.GetPreloadedSubincludes() { if inc.IsPseudoTarget() { return fmt.Errorf("Can't preload pseudotarget %v", inc) } // Queue them up asynchronously to feed the queues as quickly as possible - eg.Go(func() error { - if _, err := state.Build(inc); err != nil { + g.Go(func() error { + if _, err := r.Build(ctx, inc); err != nil { return err } return state.Parser.RegisterPreload(inc) @@ -530,7 +530,7 @@ func registerPreloads(state *core.BuildState) error { } // We must wait for all the subinclude targets to be built otherwise updating the locals might race with parsing // a package - return eg.Wait() + return g.Wait() } // FindAllBuildFiles finds all BUILD files under a particular path. From 32660f11407b4cca304e6f4ca4642f770c10b925 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 11:27:19 +0100 Subject: [PATCH 25/44] Notes of things that remain to be done --- REFACTOR_FAILURES.md | 184 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 REFACTOR_FAILURES.md diff --git a/REFACTOR_FAILURES.md b/REFACTOR_FAILURES.md new file mode 100644 index 0000000000..02cf96cb98 --- /dev/null +++ b/REFACTOR_FAILURES.md @@ -0,0 +1,184 @@ +# Outstanding `//test/...` failures from the `src/plz/plz.go` refactor + +Status as of the run below. 12 failing test targets, grouped by root cause. + +``` +plz plz test //test/... //src/... -o test.timeout:60s --exclude benchmark +261 test targets and 1041 tests run; 1027 passed, 12 errored, 2 skipped. +``` + +All of `//src/...` passes. The failures are all in `//test/...` (e2e). + +## Already fixed + +Recorded for context; these are done and are not in the 12 below. + +| Problem | Symptom | Fix | +| --- | --- | --- | +| `buildOne` re-derived its second errgroup from a context that `Wait()` had already cancelled | Large builds stopped early with `context canceled` and no error | Don't shadow `ctx`; use a separate `gctx` per group | +| `buildAll` called `buildOne` directly, bypassing the `buildOnce` memo | `panic: close of closed channel` in `FinishBuild` when a target was reached both via `:all` and as a dependency | Route through `Build` | +| `Build` claimed the `buildOnce` entry around `parseTarget` | Deadlock when parsing a package re-entered `Build` for a label defined in that same file (`subinclude(":defs_2")`) | Claim the entry after parsing, so only the build phase is memoised | +| `RuntimeDependencies()` widened to include data deps | `query print -f runtime_deps` printed data deps too | Split into `RuntimeDependencies()` and `RuntimeAndDataDependencies()` | +| `AddOriginalTarget` called inside the task goroutine | Nondeterministic original-target order, visible as `.gitignore` write order | Call it synchronously in `queueTask`, in walk order | + +## A. A failed package parse strands every other waiter + +**Failing:** `//test/preloaded_subinc:preload_subinc_test` (hangs until the test timeout) + +`BuildGraph.PackageOrWait` goes through `cmap.Map.GetOrWait`: the first caller claims the +slot and everyone else gets a wait channel. If that first caller's parse *fails*, the package +is never `Set`, so the channel is never closed and the other waiters block forever. + +In this test, preload `//plugins:shell` fails with `build file not found: ... there's no BUILD +file in plugins/`, and `///pleasings//k8s:k8s` then waits on package `plugins` indefinitely: + +``` +core.(*BuildGraph).PackageOrWait src/core/graph.go:117 +plz.(*runner).Parse src/plz/plz.go:144 +plz.(*runner).Parse src/plz/plz.go:158 +plz.(*runner).parseTarget src/plz/plz.go:211 +plz.(*runner).Build src/plz/plz.go:337 +plz.(*runner).RegisterPreloads src/plz/plz.go:525 +``` + +The packages map has no error path, unlike `cmap.ErrMap` which `buildOnce` uses — a failing +parse has no way to publish its failure to waiters. Giving `graph.packages` the same +error-carrying treatment looks like the fix. + +`RegisterPreloads` now takes a context (it previously used a bare `errgroup.Group`), so a +failing preload can at least cancel its siblings. That turns the hang into an error but +doesn't fix the underlying stranding — any parse failure anywhere still relies entirely on +context cancellation to release waiters. + +## B. `runner.Parse` lost the `dependent` label + +**Failing:** `//test/subrepo/same_package_error:same_package_error` (hangs until the test timeout) + +The test asserts the error `subrepo X is not defined in this package yet` for a BUILD file +that subincludes from a subrepo it declares further down: + +```python +subinclude("///subrepo//:foo") + +local_repository(name = "subrepo", path = "subrepo") +``` + +The old `parse.checkSubrepo` guarded this with `inSamePackage(sl, dependent)`. The current +check at `src/plz/plz.go:154` compares `sl` against `label` instead: + +```go +if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { + // TODO(peter): Unsure if this is a legit case or not. + return nil, fmt.Errorf("subinclude from within same package of a subrepo") +} +``` + +For `///subrepo//:foo`, `sl` is `//:subrepo`, so `sl.Subrepo` (`""`) never equals +`label.Subrepo` (`"subrepo"`) and the guard can't fire. We then recurse into `r.Parse(sl)` +and land in `PackageOrWait` for the package this goroutine is *already* parsing. + +The comparison needs the dependent, not the label — which means reversing the +`TODO(peter): can we drop the dependent here?` at `src/plz/plz.go:163` +(`parse.Parse(r.state, label, label)`). The answer appears to be no. + +## C. `CheckArchSubrepo` is consulted first, and it accepts anything + +**Failing:** `//test/proto_plugin:proto_rules_test` + +``` +CRITICAL: Found multiple definitions for subrepo 'go_proto' + (... Target://plugins:go-proto ... Arch:{OS:linux Arch:amd64} IsCrossCompile:false) + (... Target: ... Arch:{OS:go Arch:proto} IsCrossCompile:true) +``` + +`cli.Arch.UnmarshalFlag` accepts any `x_y` string with no `/` or `@`, so `go_proto` parses as +OS=`go`, Arch=`proto`. `src/plz/plz.go:152` checks that *before* trying to resolve the real +subrepo definition, registering a phantom arch subrepo that later collides with the genuine +one from `//plugins:go-proto`. + +Previously `CheckArchSubrepo` was only a fallback, reached from `maybeParseSubrepoPackage` +after the normal subrepo lookup had already failed. Restoring that ordering is the fix, but +it needs care: the failed normal lookup goes through `parse.Parse`, which calls +`state.LogBuildError`, so a naive "try normal, fall back to arch" would log a spurious parse +error for every legitimate arch subrepo. + +## D. Cross-compiling doesn't take effect + +**Failing:** `//test/cross_compile:location_cross_compile_test`, `//test/cross_compile:x86_test` + +``` +Unexpected contents of file: test/cross_compile/arch.txt +unexpected architecture of binary +``` + +Targets under `///linux_x86//` are being built for the host architecture. Plausibly downstream +of C, since the arch subrepo is now registered from a different place, but that is unverified — +this one needs its own investigation. + +## E. `BuildTarget.Dependencies` panics on lazily-resolved deps + +**Failing:** `//test/export:export_src_please_test`, `//test/export/test_go_bin:export_go_target`, +`//test/export/test_native_target_with_go_dep:export_native_target_with_go_dep`, +`//test/local_subinclude_test:test_subrepo` + +`src/core/build_target.go:598` resolves each declared dep with `graph.TargetOrDie`. Since +dependency resolution moved out of `build_target`, the graph legitimately may not contain a +declared dep, and the lookup panics. Two callers hit it: + +``` +panic: Target ///third_party/go/github.com_dustin_go-humanize//:go-humanize not found in build graph +core.(*BuildTarget).Dependencies src/core/build_target.go:598 +export.(*export).export src/export/export.go:189 +export.ToDir src/export/export.go:51 +``` + +``` +panic: Target //_please:arcat not found in build graph +core.(*BuildTarget).Dependencies src/core/build_target.go:598 +core.(*cycleDetector).Check.func1 src/core/cycle_detector.go:38 +core.(*BuildState).checkForCycles src/core/state.go:537 +``` + +Note the second one fires from a background goroutine, so it takes the process down +regardless of what the build is doing. + +## F. Post-build-added runtime deps are never built + +**Failing:** `//test/runtime_deps:runtime_deps_test`, `//test/plz_run:_add_data_target_test` + +``` +ERROR: //test:runtime_deps_test_case failed: + cannot calculate hash for plz-out/gen/test/post_build_runtime_dep: file does not exist +ERROR: //src:_add_data_file_and_target failed: + cannot calculate hash for plz-out/gen/src/foo.txt: file does not exist +``` + +`buildOne` snapshots the deps *before* the target itself is built: + +```go +deps := slices.Collect(target.RuntimeAndDataDependencies()) +``` + +so anything a post-build function adds via `add_runtime_dep` / `add_data` is missed. This is +the existing `TODO(peter): Need to handle targets getting extra deps added by post-build +functions here.` at `src/plz/plz.go:277` — the collect needs redoing once the target has +finished building. + +## G. `--keep_going` is not implemented + +**Failing:** `//test/keep_going:plz_build_all` + +``` +Build stopped after 40ms. 2 targets failed: //package:fail, //package:fail2 +plz-out/gen/package/dep_pass doesnt exist +``` + +`state.KeepGoing` is set in `src/please.go:1149` and read nowhere in `src/plz/plz.go`. The +errgroups short-circuit on the first error, so unrelated targets like `//package:dep_pass` +never get built. An `errgroup` can't express this on its own — the group needs to run +everything and collect errors when `KeepGoing` is set. + +## Suggested order + +A and B first: both are hangs, and both come down to the parse path waiting on a channel that +nothing guarantees will be closed. C then D, since D may fall out of C. From 56eed83a5b0ba072ca77e47d436377c9b6aecd05 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 8 Aug 2026 11:33:11 +0100 Subject: [PATCH 26/44] Update absl --- third_party/python/BUILD | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/third_party/python/BUILD b/third_party/python/BUILD index 2e85c368d6..ed0b0c2fe4 100644 --- a/third_party/python/BUILD +++ b/third_party/python/BUILD @@ -65,8 +65,8 @@ python_wheel( python_wheel( name = "absl", package_name = "absl_py", - hashes = ["c106f6ef0ae86c1273b0858b40ee15b99fad1c223838387b9d11446a033bbcb1"], - version = "0.9.0", + hashes = ["0f17b89f2a4eaaedc4f28c622998aa690564b3012a396a4ffad0821007fe03ba"], + version = "2.5.0", deps = [":six"], ) From 89ff16978efcc727a8d1272eedecd00c2c8288d6 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 10:08:36 +0100 Subject: [PATCH 27/44] Deal better with hanging parses --- src/cmap/cerrmap.go | 6 ++++++ src/core/graph.go | 38 ++++++++++++++------------------------ src/plz/plz.go | 36 +++++++++++++++++------------------- 3 files changed, 37 insertions(+), 43 deletions(-) diff --git a/src/cmap/cerrmap.go b/src/cmap/cerrmap.go index 8ccf070ef4..c2cd946c1a 100644 --- a/src/cmap/cerrmap.go +++ b/src/cmap/cerrmap.go @@ -61,6 +61,12 @@ func (m *ErrMap[K, V]) Get(key K) (V, error) { return v.Val, v.Err } +// GetOrWait returns the value corresponding to the given key, or a channel to wait on if it doesn't exist in the map. +func (m *ErrMap[K, V]) GetOrWait(key K) (V, <-chan struct{}, error) { + v, ch, _ := m.m.GetOrWait(key) + return v.Val, ch, v.Err +} + // GetOrSet returns the value if set, or an error if one has been set. // If nothing has been set for the key, it runs the given function to generate the value and then sets it. func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) { diff --git a/src/core/graph.go b/src/core/graph.go index f22a9265e1..8a21eeaf75 100644 --- a/src/core/graph.go +++ b/src/core/graph.go @@ -6,8 +6,8 @@ package core import ( "context" - "maps" "fmt" + "maps" "slices" "sort" "sync" @@ -32,7 +32,7 @@ type BuildGraph struct { // Map of all currently known targets by their label. targets *cmap.Map[BuildLabel, *BuildTarget] // Map of all currently known packages. - packages *cmap.Map[packageKey, *Package] + packages *cmap.ErrMap[packageKey, *Package] // Registered subrepos, as a map of their name to their root. subrepos *cmap.Map[string, *Subrepo] // Subincludes that are subincluded by other subincludes @@ -101,25 +101,14 @@ func (graph *BuildGraph) PackageByLabel(label BuildLabel) *Package { // Package retrieves a package from the graph by name & subrepo, or nil if it can't be found. func (graph *BuildGraph) Package(name, subrepo string) *Package { - return graph.packages.Get(packageKey{Name: name, Subrepo: subrepo}) + pkg, _ := graph.packages.Get(packageKey{Name: name, Subrepo: subrepo}) + return pkg } -// PackageOrWait retrieves a package from the graph. -// If the package doesn't exist and nobody has asked for it before, it returns nil. -// If the package doesn't exist and somebody has asked for it before, it waits until it is added with AddPackage, then returns it. -func (graph *BuildGraph) PackageOrWait(ctx context.Context, label BuildLabel) (*Package, error) { - key := packageKey{Name: label.PackageName, Subrepo: label.Subrepo} - pkg, wait, first := graph.packages.GetOrWait(key) - if pkg != nil || first { - return pkg, nil - } - done := ctx.Done() - select { - case <-wait: - return graph.packages.Get(key), nil - case <-done: - return nil, ctx.Err() - } +// GetOrSetPackage retrieves a package from the graph. +// If it doesn't exist, it calls the supplied function to create it. +func (graph *BuildGraph) GetOrSetPackage(ctx context.Context, label BuildLabel, f func() (*Package, error)) (*Package, error) { + return graph.packages.GetOrSetCtx(ctx, packageKey{Name: label.PackageName, Subrepo: label.Subrepo}, f) } // PackageOrDie retrieves a package by label, and dies if it can't be found. @@ -173,12 +162,13 @@ func (graph *BuildGraph) AllTargets() BuildTargets { return targets } -// PackageMap returns a copy of the graph's internal map of name to package. +// PackageMap returns a map of name to package. +// TODO(peterebden): Change this to an iterator. func (graph *BuildGraph) PackageMap() map[string]*Package { packages := map[string]*Package{} - for _, pkg := range graph.packages.Values() { - packages[packageKey{Subrepo: pkg.SubrepoName, Name: pkg.Name}.String()] = pkg - } + graph.packages.Range(func(k packageKey, v *Package) { + packages[k.String()] = v + }) return packages } @@ -186,7 +176,7 @@ func (graph *BuildGraph) PackageMap() map[string]*Package { func NewGraph() *BuildGraph { g := &BuildGraph{ targets: cmap.New[BuildLabel, *BuildTarget](cmap.DefaultShardCount, hashBuildLabel), - packages: cmap.New[packageKey, *Package](cmap.DefaultShardCount, hashPackageKey), + packages: cmap.NewErrMap[packageKey, *Package](cmap.DefaultShardCount, hashPackageKey, nil), subrepos: cmap.New[string, *Subrepo](cmap.SmallShardCount, cmap.XXHash), subincludeSubincludes: map[BuildLabel]labelSet{}, } diff --git a/src/plz/plz.go b/src/plz/plz.go index 7e961dbaea..3908160e4a 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -141,27 +141,25 @@ type runner struct { // Parse parses for a target. It can be called more than once for the same build label. func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Package, error) { - if pkg, err := r.state.Graph.PackageOrWait(ctx, label); err != nil || pkg != nil { - return pkg, err - } - // If we get here then we have to parse it (we only get here if we are the first one) - r.progress.numParsing.Add(1) - defer r.progress.numParsing.Add(-1) - // If the target defines a subrepo, we must make sure that is built first. - if label.Subrepo != "" { - if r.state.CheckArchSubrepo(label.Subrepo) == nil { - sl := label.SubrepoLabel(r.state) - if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { - // TODO(peter): Unsure if this is a legit case or not. - return nil, fmt.Errorf("subinclude from within same package of a subrepo") - } - if _, err := r.Parse(ctx, sl); err != nil { - return nil, err + return r.state.Graph.GetOrSetPackage(ctx, label, func() (*core.Package, error) { + r.progress.numParsing.Add(1) + defer r.progress.numParsing.Add(-1) + // If the target defines a subrepo, we must make sure that is built first. + if label.Subrepo != "" { + if r.state.CheckArchSubrepo(label.Subrepo) == nil { + sl := label.SubrepoLabel(r.state) + if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { + // TODO(peter): Unsure if this is a legit case or not. + return nil, fmt.Errorf("subinclude from within same package of a subrepo") + } + if _, err := r.Parse(ctx, sl); err != nil { + return nil, err + } } } - } - // TODO(peter): can we drop the dependent here? - return parse.Parse(r.state, label, label) + // TODO(peter): can we drop the dependent here? + return parse.Parse(r.state, label, label) + }) } // RecursiveParse is like Parse but recurses down into all dependencies of the target as well. From 61167247ccb63d7856ad7176b3111c3e237826fd Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 10:13:54 +0100 Subject: [PATCH 28/44] Remove finishBuilding chan, nothing needs it any more --- src/build/build_step.go | 3 --- src/core/build_target.go | 13 ------------- src/core/state.go | 13 ------------- 3 files changed, 29 deletions(-) diff --git a/src/build/build_step.go b/src/build/build_step.go index 25e2c1f872..f77405950b 100644 --- a/src/build/build_step.go +++ b/src/build/build_step.go @@ -74,7 +74,6 @@ func Build(state *core.BuildState, target *core.BuildTarget, remote bool) error log.Errorf("Failed to remove outputs for %s: %s", target.Label, err) } target.SetState(core.Failed) - target.FinishBuild() return err } if remote { @@ -82,8 +81,6 @@ func Build(state *core.BuildState, target *core.BuildTarget, remote bool) error } else { successfulLocalTargetBuildDuration.WithLabelValues(metrics.CILabel).Observe(float64(time.Since(start).Milliseconds())) } - // Mark the target as having finished building. - target.FinishBuild() return nil } diff --git a/src/core/build_target.go b/src/core/build_target.go index b0c8728a66..cef43d87d0 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -205,8 +205,6 @@ type BuildTarget struct { EntryPoints map[string]string `name:"entry_points"` // Used to arbitrate concurrent access to dependencies, and to the test results. mutex sync.RWMutex `print:"false"` - // Used to notify once this target has built successfully. - finishedBuilding chan struct{} `print:"false"` // Env are any custom environment variables to set for this build target Env map[string]string `name:"env"` // The content of text_file() rules @@ -405,7 +403,6 @@ func NewBuildTarget(label BuildLabel) *BuildTarget { Label: label, state: int32(Inactive), BuildingDescription: DefaultBuildingDescription, - finishedBuilding: make(chan struct{}), } } @@ -794,16 +791,6 @@ func (target *BuildTarget) IterAllRuntimeDependencies(graph *BuildGraph) iter.Se } } -// FinishBuild marks this target as having built. -func (target *BuildTarget) FinishBuild() { - close(target.finishedBuilding) -} - -// WaitForBuild blocks until this target has finished building. -func (target *BuildTarget) WaitForBuild(dependant BuildLabel) { - waitOnChan(target.finishedBuilding, "Still waiting on (target %v).WaitForBuild(dependant %v)", target.Label, dependant) -} - // DeclaredOutputs returns the outputs from this target's original declaration. // Hence it's similar to Outputs() but without the resolving of other rule names. func (target *BuildTarget) DeclaredOutputs() []string { diff --git a/src/core/state.go b/src/core/state.go index 60b6e97bc5..785bd8d4ad 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -663,19 +663,6 @@ func (state *BuildState) ExpandVisibleOriginalTargets() BuildLabels { return ret } -func waitOnChan[T any](ch chan T, message string, args ...any) { - start := time.Now() - t := time.NewTimer(10 * time.Second) - defer t.Stop() - select { - case <-ch: - return - case <-t.C: - log.Debugf("%v (after %v)", fmt.Sprintf(message, args...), time.Since(start)) - } - <-ch -} - // AddTarget adds a new target to the build graph. func (state *BuildState) AddTarget(pkg *Package, target *BuildTarget) { pkg.AddTarget(target) From 13321ecab302d07ed871e9f79ee00e8098c9efb6 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 10:32:59 +0100 Subject: [PATCH 29/44] Clean up preloading subincludes --- REFACTOR_FAILURES.md | 1 + src/build/build_step_stress_test.go | 4 ---- src/build/build_step_test.go | 4 ---- src/core/state.go | 1 - src/parse/asp/parser.go | 21 ++++++++------------- src/parse/init.go | 17 +++++------------ src/plz/BUILD | 1 + src/plz/plz.go | 18 ++++++++++++------ 8 files changed, 27 insertions(+), 40 deletions(-) diff --git a/REFACTOR_FAILURES.md b/REFACTOR_FAILURES.md index 02cf96cb98..5b8beb2c72 100644 --- a/REFACTOR_FAILURES.md +++ b/REFACTOR_FAILURES.md @@ -22,6 +22,7 @@ Recorded for context; these are done and are not in the 12 below. | `AddOriginalTarget` called inside the task goroutine | Nondeterministic original-target order, visible as `.gitignore` write order | Call it synchronously in `queueTask`, in walk order | ## A. A failed package parse strands every other waiter +# DONE in 89ff16978efcc727a8d1272eedecd00c2c8288d6 and d2a0d2dd773a8d651d9fac991a04a2a7e67a292b **Failing:** `//test/preloaded_subinc:preload_subinc_test` (hangs until the test timeout) diff --git a/src/build/build_step_stress_test.go b/src/build/build_step_stress_test.go index 58fc489905..02d9930909 100644 --- a/src/build/build_step_stress_test.go +++ b/src/build/build_step_stress_test.go @@ -98,10 +98,6 @@ type fakeParser struct { PostBuildFunctions buildFunctionMap } -func (fake *fakeParser) RegisterPreload(core.BuildLabel) error { - return nil -} - // ParseFile stub func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error { return nil diff --git a/src/build/build_step_test.go b/src/build/build_step_test.go index 01c1834ca8..ba0a4a36c4 100644 --- a/src/build/build_step_test.go +++ b/src/build/build_step_test.go @@ -609,10 +609,6 @@ func (*mockCache) Shutdown() {} type fakeParser struct { } -func (fake *fakeParser) RegisterPreload(core.BuildLabel) error { - return nil -} - // ParseFile stub func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error { return nil diff --git a/src/core/state.go b/src/core/state.go index 785bd8d4ad..e9b4f4f6f2 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -77,7 +77,6 @@ type Parser interface { RunPreBuildFunction(state *BuildState, target *BuildTarget) error // RunPostBuildFunction runs a post-build function for a target. RunPostBuildFunction(state *BuildState, target *BuildTarget, output string) error - RegisterPreload(label BuildLabel) error } // A RemoteClient is the interface to a remote execution service. diff --git a/src/parse/asp/parser.go b/src/parse/asp/parser.go index 95a45770d7..4d52615ad8 100644 --- a/src/parse/asp/parser.go +++ b/src/parse/asp/parser.go @@ -10,7 +10,6 @@ import ( iofs "io/fs" "os" "strings" - "sync" "github.com/thought-machine/please/src/cli/logging" "github.com/thought-machine/please/src/core" @@ -32,9 +31,6 @@ type Parser struct { // Parallelism limiter to ensure we don't try to run too many parses simultaneously limiter semaphore - - // Used during subinclude preloads - preloadMutex sync.Mutex } // NewParser creates a new parser instance. One is normally sufficient for a process lifetime. @@ -93,8 +89,8 @@ func (p *Parser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, return err } -// RegisterPreload pre-registers a preload, forcing us to build any transitive preloads before we move on -func (p *Parser) RegisterPreload(label core.BuildLabel) error { +// PreloadSubinclude pre-registers a preload, forcing us to build any transitive preloads before we move on +func (p *Parser) PreloadSubinclude(label core.BuildLabel) error { p.limiter.Acquire() defer p.limiter.Release() @@ -102,13 +98,12 @@ func (p *Parser) RegisterPreload(label core.BuildLabel) error { s := p.interpreter.scope.newScope(nil, "", 0) s.config = p.interpreter.scope.config.Copy() s.Set("CONFIG", s.config) - if err := p.interpreter.preloadSubinclude(s, label); err != nil { - return err - } - p.preloadMutex.Lock() - defer p.preloadMutex.Unlock() - p.interpreter.preloads = append(p.interpreter.preloads, label) - return nil + return p.interpreter.preloadSubinclude(s, label) +} + +// RegisterPreloads registers the set of preloaded subincludes. +func (p *Parser) RegisterPreloads(labels []core.BuildLabel) { + p.interpreter.preloads = labels } // ParseReader parses the contents of the given ReadSeeker as a BUILD file. diff --git a/src/parse/init.go b/src/parse/init.go index 8b44d60778..285ef945bb 100644 --- a/src/parse/init.go +++ b/src/parse/init.go @@ -16,13 +16,11 @@ import ( "github.com/thought-machine/please/src/parse/asp" ) -// InitParser initialises the parser engine. This is guaranteed to be called exactly once before any calls to Parse(). -func InitParser(state *core.BuildState) *core.BuildState { - if state.Parser == nil { - p := &aspParser{parser: newAspParser(state)} - state.Parser = p - } - return state +// InitParser initialises the parser engine. +func InitParser(state *core.BuildState) *asp.Parser { + p := newAspParser(state) + state.Parser = &aspParser{parser: p} + return p } // GetAspParser returns the underlying asp.Parser from the state's parser. @@ -91,11 +89,6 @@ func (p *aspParser) RunPostBuildFunction(state *core.BuildState, target *core.Bu }) } -// RegisterPreload pre-registers a preload, forcing us to build any transitive preloads before we move on -func (p *aspParser) RegisterPreload(label core.BuildLabel) error { - return p.parser.RegisterPreload(label) -} - // runBuildFunction runs either the pre- or post-build function. func (p *aspParser) runBuildFunction(state *core.BuildState, target *core.BuildTarget, callbackType string, f func() error) error { state.LogBuildResult(target, core.PackageParsing, fmt.Sprintf("Running %s-build function for %s", callbackType, target.Label)) diff --git a/src/plz/BUILD b/src/plz/BUILD index a04583b435..c70633f2f8 100644 --- a/src/plz/BUILD +++ b/src/plz/BUILD @@ -13,6 +13,7 @@ go_library( "//src/core", "//src/fs", "//src/metrics", + "//src/parse/asp", "//src/parse", "//src/remote", "//src/test", diff --git a/src/plz/plz.go b/src/plz/plz.go index 3908160e4a..8da8cc4001 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -21,6 +21,7 @@ import ( "github.com/thought-machine/please/src/fs" "github.com/thought-machine/please/src/metrics" "github.com/thought-machine/please/src/parse" + "github.com/thought-machine/please/src/parse/asp" "github.com/thought-machine/please/src/remote" "github.com/thought-machine/please/src/test" ) @@ -41,7 +42,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress go state.UpdateResources() } - parse.InitParser(state) + parser := parse.InitParser(state) // This must happen however we exit; anything reading state.Results() (e.g. the display) // waits for that channel to be closed, so it would hang forever if we returned an error first. @@ -83,7 +84,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress } // Register the preloaded targets with the parser - if err := r.RegisterPreloads(ctx, state); err != nil { + if err := r.RegisterPreloads(ctx, state, parser); err != nil { return err } @@ -511,9 +512,10 @@ func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest // RegisterPreloads waits for all preloaded subinclude targets to be built, downloads them, and then registers them with // the interpreter. We have to actually register them otherwise this will return before we build any // transitive subincludes. -func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState) error { +func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState, parser *asp.Parser) error { g, ctx := errgroup.WithContext(ctx) - for _, inc := range state.GetPreloadedSubincludes() { + preloads := state.GetPreloadedSubincludes() + for _, inc := range preloads { if inc.IsPseudoTarget() { return fmt.Errorf("Can't preload pseudotarget %v", inc) } @@ -523,12 +525,16 @@ func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState) e if _, err := r.Build(ctx, inc); err != nil { return err } - return state.Parser.RegisterPreload(inc) + return parser.PreloadSubinclude(inc) }) } // We must wait for all the subinclude targets to be built otherwise updating the locals might race with parsing // a package - return g.Wait() + if err := g.Wait(); err != nil { + return err + } + parser.RegisterPreloads(preloads) + return nil } // FindAllBuildFiles finds all BUILD files under a particular path. From 0ba3093a522688f55a0025c02ff43a0f622b3809 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 10:51:41 +0100 Subject: [PATCH 30/44] Plumb dependent label around again --- src/core/state.go | 6 ++- src/parse/asp/builtins.go | 4 +- src/parse/init.go | 2 +- src/parse/parse_step.go | 2 +- src/plz/plz.go | 103 ++++++++++++++++++++------------------ 5 files changed, 63 insertions(+), 54 deletions(-) diff --git a/src/core/state.go b/src/core/state.go index e9b4f4f6f2..7f1dffe115 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -220,9 +220,11 @@ type BuildState struct { // Build is a callback to build a single target. It's set from outside here. // TODO(peter): can we find a way of moving these off this struct? it feels weird here - Build func(BuildLabel) (*BuildTarget, error) + // The second label is the dependent, i.e. whatever is asking for this to be built. + Build func(label, dependent BuildLabel) (*BuildTarget, error) // Parse is a callback to parse a single package. It's also set from outside. - Parse func(BuildLabel) (*Package, error) + // The second label is the dependent, i.e. whatever is asking for this to be parsed. + Parse func(label, dependent BuildLabel) (*Package, error) // Cancel is a cancel function called when the state detects a cycle. Cancel func() diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 8a5944e774..e2a82137bf 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -305,7 +305,7 @@ func bazelLoad(s *scope, args []pyObject) pyObject { func (s *scope) WaitForSubincludedTarget(l, dependent core.BuildLabel) (*core.BuildTarget, error) { s.interpreter.limiter.Release() defer s.interpreter.limiter.Acquire() - return s.state.Build(l) + return s.state.Build(l, dependent) } // builtinFail raises an immediate error that can't be intercepted. @@ -381,7 +381,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget { Subrepo: subrepoLabel.Subrepo, Name: "all", } - if _, err := s.state.Parse(subrepoPackageLabel); err != nil { + if _, err := s.state.Parse(subrepoPackageLabel, pkgLabel); err != nil { s.Error("Failed to parse subrepo target: %w", err) } } diff --git a/src/parse/init.go b/src/parse/init.go index 285ef945bb..8bef84d787 100644 --- a/src/parse/init.go +++ b/src/parse/init.go @@ -92,7 +92,7 @@ func (p *aspParser) RunPostBuildFunction(state *core.BuildState, target *core.Bu // runBuildFunction runs either the pre- or post-build function. func (p *aspParser) runBuildFunction(state *core.BuildState, target *core.BuildTarget, callbackType string, f func() error) error { state.LogBuildResult(target, core.PackageParsing, fmt.Sprintf("Running %s-build function for %s", callbackType, target.Label)) - if _, err := state.Parse(target.Label); err != nil { + if _, err := state.Parse(target.Label, target.Label); err != nil { return err } if err := f(); err != nil { diff --git a/src/parse/parse_step.go b/src/parse/parse_step.go index d0d93a7938..ec2ac452df 100644 --- a/src/parse/parse_step.go +++ b/src/parse/parse_step.go @@ -59,7 +59,7 @@ func parse(state *core.BuildState, label, dependent core.BuildLabel) (*core.Pack if subrepo != nil && subrepo.Target != nil { // We have got the definition of the subrepo, but it depends on something, make sure that has been built. - if _, err := state.Build(subrepo.Target.Label); err != nil { + if _, err := state.Build(subrepo.Target.Label, dependent); err != nil { return nil, err } if err := subrepo.State.Initialise(subrepo); err != nil { diff --git a/src/plz/plz.go b/src/plz/plz.go index 8da8cc4001..4f01200801 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -76,11 +76,11 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress } // We don't have context as an argument to this, because they're not fully plumbed through (but probably should be) - state.Build = func(label core.BuildLabel) (*core.BuildTarget, error) { - return r.Build(ctx, label) + state.Build = func(label, dependent core.BuildLabel) (*core.BuildTarget, error) { + return r.Build(ctx, label, dependent) } - state.Parse = func(label core.BuildLabel) (*core.Package, error) { - return r.Parse(ctx, label) + state.Parse = func(label, dependent core.BuildLabel) (*core.Package, error) { + return r.Parse(ctx, label, dependent) } // Register the preloaded targets with the parser @@ -91,7 +91,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress if state.Config.Bazel.Compatibility && fs.FileExists("WORKSPACE") { // We have to parse the WORKSPACE file before anything else to understand subrepos. // This is a bit crap really since it inhibits parallelism for the first step. - if _, err := r.Parse(ctx, core.NewBuildLabel("workspace", "all")); err != nil { + if _, err := r.Parse(ctx, core.NewBuildLabel("workspace", "all"), core.OriginalTarget); err != nil { return err } } @@ -141,34 +141,41 @@ type runner struct { } // Parse parses for a target. It can be called more than once for the same build label. -func (r *runner) Parse(ctx context.Context, label core.BuildLabel) (*core.Package, error) { +// The dependent is whatever is asking for this to be parsed; it's used to produce better error +// messages, and to detect a package that is asking to parse itself. +func (r *runner) Parse(ctx context.Context, label, dependent core.BuildLabel) (*core.Package, error) { return r.state.Graph.GetOrSetPackage(ctx, label, func() (*core.Package, error) { r.progress.numParsing.Add(1) defer r.progress.numParsing.Add(-1) - // If the target defines a subrepo, we must make sure that is built first. - if label.Subrepo != "" { + // If the target is in a subrepo that we don't know about yet, we must make sure that is built first. + // If we already have it there's nothing to do here; it's been registered by whatever parse defined it. + if label.Subrepo != "" && r.state.Graph.Subrepo(label.Subrepo) == nil { if r.state.CheckArchSubrepo(label.Subrepo) == nil { sl := label.SubrepoLabel(r.state) - if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { - // TODO(peter): Unsure if this is a legit case or not. - return nil, fmt.Errorf("subinclude from within same package of a subrepo") + if inSamePackage(sl, dependent) { + return nil, fmt.Errorf("subrepo %v is not defined in this package yet. It must appear before it is used by %v", label.Subrepo, dependent) } - if _, err := r.Parse(ctx, sl); err != nil { + if _, err := r.Parse(ctx, sl, label); err != nil { return nil, err } } } - // TODO(peter): can we drop the dependent here? - return parse.Parse(r.state, label, label) + return parse.Parse(r.state, label, dependent) }) } +// inSamePackage returns true if the two labels are in the same package (and hence, if one of them is +// currently being parsed, both are). +func inSamePackage(label, dependent core.BuildLabel) bool { + return !dependent.IsOriginalTarget() && label.Subrepo == dependent.Subrepo && label.PackageName == dependent.PackageName +} + // RecursiveParse is like Parse but recurses down into all dependencies of the target as well. -func (r *runner) RecursiveParse(ctx context.Context, label core.BuildLabel) error { +func (r *runner) RecursiveParse(ctx context.Context, label, dependent core.BuildLabel) error { if !label.IsAllTargets() { - return r.recursiveParse(ctx, label) + return r.recursiveParse(ctx, label, dependent) } - pkg, err := r.Parse(ctx, label) + pkg, err := r.Parse(ctx, label, dependent) if err != nil { return err } @@ -180,7 +187,7 @@ func (r *runner) RecursiveParse(ctx context.Context, label core.BuildLabel) erro if _, present := seen[dep]; !present { seen[dep] = struct{}{} g.Go(func() error { - return r.recursiveParse(ctx, dep) + return r.recursiveParse(ctx, dep, target.Label) }) } } @@ -188,33 +195,33 @@ func (r *runner) RecursiveParse(ctx context.Context, label core.BuildLabel) erro return g.Wait() } -func (r *runner) recursiveParse(ctx context.Context, label core.BuildLabel) error { - target, err := r.parseTarget(ctx, label) +func (r *runner) recursiveParse(ctx context.Context, label, dependent core.BuildLabel) error { + target, err := r.parseTarget(ctx, label, dependent) if err != nil { return err } g, ctx := errgroup.WithContext(ctx) for dep := range target.DeclaredDependencies() { g.Go(func() error { - _, err := r.Parse(ctx, dep) + _, err := r.Parse(ctx, dep, target.Label) return err }) } return g.Wait() } -func (r *runner) parseTarget(ctx context.Context, label core.BuildLabel) (*core.BuildTarget, error) { +func (r *runner) parseTarget(ctx context.Context, label, dependent core.BuildLabel) (*core.BuildTarget, error) { if target := r.state.Graph.Target(label); target != nil { return target, nil } - pkg, err := r.Parse(ctx, label) + pkg, err := r.Parse(ctx, label, dependent) if err != nil { return nil, err } if target := pkg.Target(label.Name); target != nil { return target, nil } - err = fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) + err = fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, dependent)) r.state.LogBuildError(label, core.ParseFailed, err, "%s", err) return nil, err } @@ -222,7 +229,7 @@ func (r *runner) parseTarget(ctx context.Context, label core.BuildLabel) (*core. // resolveTarget resolves a target, dealing with require/provide as needed. func (r *runner) resolveTarget(ctx context.Context, label core.BuildLabel, dependent *core.BuildTarget) iter.Seq2[*core.BuildTarget, error] { return func(yield func(*core.BuildTarget, error) bool) { - target, err := r.parseTarget(ctx, label) + target, err := r.parseTarget(ctx, label, dependent.Label) if err != nil { yield(nil, err) return @@ -235,7 +242,7 @@ func (r *runner) resolveTarget(ctx context.Context, label core.BuildLabel, depen } // TODO(peter): Would parallelism here be useful? for _, p := range provided { - if !yield(r.parseTarget(ctx, p)) { + if !yield(r.parseTarget(ctx, p, dependent.Label)) { break } } @@ -248,7 +255,7 @@ func (r *runner) buildDep(ctx context.Context, dep core.BuildLabel, target *core if err != nil { return err } - if _, err := r.Build(ctx, t.Label); err != nil { + if _, err := r.Build(ctx, t.Label, target.Label); err != nil { return err } } @@ -287,7 +294,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { }) for _, dep := range deps { g.Go(func() error { - _, err := r.Build(gctx, dep) + _, err := r.Build(gctx, dep, target.Label) return err }) } @@ -304,8 +311,8 @@ func (r *runner) buildJustOne(target *core.BuildTarget) error { } // buildAll builds all the targets specified by the given label (which can be :all, but can't be ...). -func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { - pkg, err := r.Parse(ctx, label) +func (r *runner) buildAll(ctx context.Context, label, dependent core.BuildLabel) error { + pkg, err := r.Parse(ctx, label, dependent) if err != nil { return err } @@ -315,7 +322,7 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { g.Go(func() error { // N.B. This must go through Build, not buildOne, so we don't build a target twice // if it's reached both via :all and as a dependency of something else. - _, err := r.Build(gctx, target.Label) + _, err := r.Build(gctx, target.Label, dependent) return err }) } @@ -324,16 +331,16 @@ func (r *runner) buildAll(ctx context.Context, label core.BuildLabel) error { } // Build is the main entrypoint to build a label -func (r *runner) Build(ctx context.Context, label core.BuildLabel) (*core.BuildTarget, error) { +func (r *runner) Build(ctx context.Context, label, dependent core.BuildLabel) (*core.BuildTarget, error) { if label.IsAllTargets() { return r.buildOnce.GetOrSetCtx(ctx, label, func() (*core.BuildTarget, error) { - return nil, r.buildAll(ctx, label) + return nil, r.buildAll(ctx, label, dependent) }) } // N.B. We must parse the target _before_ claiming its entry in buildOnce; parsing its package can // re-enter here for the same label (e.g. a BUILD file that subincludes a target it defines // earlier in the same file) and we'd then deadlock waiting on ourselves. - target, err := r.parseTarget(ctx, label) + target, err := r.parseTarget(ctx, label, dependent) if err != nil { return nil, err } @@ -345,9 +352,9 @@ func (r *runner) Build(ctx context.Context, label core.BuildLabel) (*core.BuildT } // testOne tests one single target -func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { +func (r *runner) testOne(ctx context.Context, target *core.BuildTarget, dependent core.BuildLabel) error { r.progress.numTotal.Add(int64(r.state.NumTestRuns)) - if _, err := r.Build(ctx, target.Label); err != nil { + if _, err := r.Build(ctx, target.Label, dependent); err != nil { return err } if !target.IsTest() { @@ -380,15 +387,15 @@ func (r *runner) testOne(ctx context.Context, target *core.BuildTarget) error { } // Test is the main entrypoint to run tests for a label -func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { +func (r *runner) Test(ctx context.Context, label, dependent core.BuildLabel) error { if !label.IsAllTargets() { - target, err := r.parseTarget(ctx, label) + target, err := r.parseTarget(ctx, label, dependent) if err != nil { return err } - return r.testOne(ctx, target) + return r.testOne(ctx, target, dependent) } - pkg, err := r.Parse(ctx, label) + pkg, err := r.Parse(ctx, label, dependent) if err != nil { return err } @@ -396,7 +403,7 @@ func (r *runner) Test(ctx context.Context, label core.BuildLabel) error { for _, target := range pkg.AllTargets() { if r.state.ShouldInclude(target) { g.Go(func() error { - return r.testOne(ctx, target) + return r.testOne(ctx, target, dependent) }) } } @@ -421,7 +428,7 @@ func (r *runner) FindOriginalTaskSet(ctx context.Context, targets []core.BuildLa } func (r *runner) queueTargetsForDebug(ctx context.Context, target core.BuildLabel) error { - if _, err := r.Parse(ctx, target); err != nil { + if _, err := r.Parse(ctx, target, core.OriginalTarget); err != nil { return err } t := r.state.Graph.TargetOrDie(target) @@ -473,13 +480,13 @@ func (r *runner) findOriginalTask(ctx context.Context, target core.BuildLabel, n prefix := "" if target.Subrepo != "" { subrepoLabel := target.SubrepoLabel(r.state) - if target, err := r.Build(ctx, subrepoLabel); err != nil { + if target, err := r.Build(ctx, subrepoLabel, core.OriginalTarget); err != nil { return err } else if err := r.state.EnsureDownloaded(target); err != nil { return err } // Targets now get activated during parsing, so can be built before we finish parsing their package. - pkg, err := r.Parse(ctx, subrepoLabel) + pkg, err := r.Parse(ctx, subrepoLabel, core.OriginalTarget) if err != nil { return err } @@ -499,13 +506,13 @@ func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest r.state.AddOriginalTarget(target) r.tasks.Go(func() error { if needTest { - return r.Test(ctx, target) + return r.Test(ctx, target, core.OriginalTarget) } else if needBuild { - _, err := r.Build(ctx, target) + _, err := r.Build(ctx, target, core.OriginalTarget) // TODO(peter): Ensure this gets downloaded if needed return err } - return r.RecursiveParse(ctx, target) + return r.RecursiveParse(ctx, target, core.OriginalTarget) }) } @@ -522,7 +529,7 @@ func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState, p // Queue them up asynchronously to feed the queues as quickly as possible g.Go(func() error { - if _, err := r.Build(ctx, inc); err != nil { + if _, err := r.Build(ctx, inc, core.OriginalTarget); err != nil { return err } return parser.PreloadSubinclude(inc) From d39467816df75ecbb3678b31a68fadb604f08148 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 10:52:16 +0100 Subject: [PATCH 31/44] update doc --- REFACTOR_FAILURES.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/REFACTOR_FAILURES.md b/REFACTOR_FAILURES.md index 5b8beb2c72..8defe7fbed 100644 --- a/REFACTOR_FAILURES.md +++ b/REFACTOR_FAILURES.md @@ -52,7 +52,7 @@ doesn't fix the underlying stranding — any parse failure anywhere still relies context cancellation to release waiters. ## B. `runner.Parse` lost the `dependent` label - +DONE in 0ba3093a522688f55a0025c02ff43a0f622b3809 **Failing:** `//test/subrepo/same_package_error:same_package_error` (hangs until the test timeout) The test asserts the error `subrepo X is not defined in this package yet` for a BUILD file @@ -83,7 +83,7 @@ The comparison needs the dependent, not the label — which means reversing the (`parse.Parse(r.state, label, label)`). The answer appears to be no. ## C. `CheckArchSubrepo` is consulted first, and it accepts anything - +SORT OF DONE in 0ba3093a522688f55a0025c02ff43a0f622b3809 **Failing:** `//test/proto_plugin:proto_rules_test` ``` @@ -119,8 +119,7 @@ this one needs its own investigation. ## E. `BuildTarget.Dependencies` panics on lazily-resolved deps **Failing:** `//test/export:export_src_please_test`, `//test/export/test_go_bin:export_go_target`, -`//test/export/test_native_target_with_go_dep:export_native_target_with_go_dep`, -`//test/local_subinclude_test:test_subrepo` +`//test/export/test_native_target_with_go_dep:export_native_target_with_go_dep` `src/core/build_target.go:598` resolves each declared dep with `graph.TargetOrDie`. Since dependency resolution moved out of `build_target`, the graph legitimately may not contain a From ba3fef2921b215d29ced22bb5336e65fd70fbbbc Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 11:28:01 +0100 Subject: [PATCH 32/44] Subrepo fixes for C --- src/core/state.go | 10 ------ src/parse/parse_step.go | 23 ------------- src/plz/plz.go | 76 +++++++++++++++++++++++++++++++++++------ 3 files changed, 66 insertions(+), 43 deletions(-) diff --git a/src/core/state.go b/src/core/state.go index 7f1dffe115..700f07ccc1 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -730,16 +730,6 @@ func exportFile(state *BuildState, pkg *Package, label BuildLabel) { state.AddTarget(pkg, t) } -// CheckArchSubrepo checks if a target refers to a cross-compiling subrepo. -// Those don't have to be explicitly defined - maybe we should insist on that, but it's nicer not to have to. -func (state *BuildState) CheckArchSubrepo(name string) *Subrepo { - var arch cli.Arch - if err := arch.UnmarshalFlag(name); err == nil { - return state.Graph.MaybeAddSubrepo(SubrepoForArch(state, arch)) - } - return nil -} - // ForTarget returns the state associated with a given target. // This differs if the target is in a subrepo for a different architecture. func (state *BuildState) ForTarget(target *BuildTarget) *BuildState { diff --git a/src/parse/parse_step.go b/src/parse/parse_step.go index ec2ac452df..3f1082c94f 100644 --- a/src/parse/parse_step.go +++ b/src/parse/parse_step.go @@ -22,30 +22,7 @@ var log = logging.Log var ErrMissingBuildFile = errors.New("build file not found") // Parse parses the package corresponding to a single build label. The label can be :all to add all targets in a package. -// It is not an error if the package has already been parsed. -// -// By default, after the package is parsed, any targets that are now needed for the build and ready -// to be built are queued, and any new packages are queued for parsing. When a specific label is requested -// this is straightforward, but when parsing for pseudo-targets like :all and ..., various flags affect it: -// 'include' and 'exclude' refer to the labels of targets to be added. If 'include' is non-empty then only -// targets with at least one matching label are added. Any targets with a label in 'exclude' are not added. -// 'forSubinclude' is set when the parse is required for a subinclude target so should proceed -// even when we're not otherwise building targets. func Parse(state *core.BuildState, label, dependent core.BuildLabel) (*core.Package, error) { - pkg, err := parse(state, label, dependent) - if err != nil { - state.LogBuildError(label, core.ParseFailed, err, "Failed to parse package") - return nil, err - } - return pkg, nil -} - -func parse(state *core.BuildState, label, dependent core.BuildLabel) (*core.Package, error) { - // TODO(peter): I don't think we need this any more - // if t := state.Graph.Target(label); t != nil && t.State() < core.Active { - // return state.ActivateTarget(nil, label, dependent, mode) - // } - subrepo, err := checkSubrepo(state, label) if err != nil { return nil, err diff --git a/src/plz/plz.go b/src/plz/plz.go index 4f01200801..2784b6f374 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -2,6 +2,7 @@ package plz import ( "context" + "errors" "fmt" "iter" "path/filepath" @@ -144,26 +145,70 @@ type runner struct { // The dependent is whatever is asking for this to be parsed; it's used to produce better error // messages, and to detect a package that is asking to parse itself. func (r *runner) Parse(ctx context.Context, label, dependent core.BuildLabel) (*core.Package, error) { + return r.parse(ctx, label, dependent, false) +} + +// tryParse is like Parse but doesn't report failures. It's used where a failure isn't necessarily an +// error, i.e. when we're speculatively looking for the package that might define a subrepo; the caller +// is responsible for reporting anything it can't handle itself. +func (r *runner) tryParse(ctx context.Context, label, dependent core.BuildLabel) (*core.Package, error) { + return r.parse(ctx, label, dependent, true) +} + +func (r *runner) parse(ctx context.Context, label, dependent core.BuildLabel, quiet bool) (*core.Package, error) { return r.state.Graph.GetOrSetPackage(ctx, label, func() (*core.Package, error) { r.progress.numParsing.Add(1) defer r.progress.numParsing.Add(-1) - // If the target is in a subrepo that we don't know about yet, we must make sure that is built first. - // If we already have it there's nothing to do here; it's been registered by whatever parse defined it. - if label.Subrepo != "" && r.state.Graph.Subrepo(label.Subrepo) == nil { - if r.state.CheckArchSubrepo(label.Subrepo) == nil { - sl := label.SubrepoLabel(r.state) - if inSamePackage(sl, dependent) { - return nil, fmt.Errorf("subrepo %v is not defined in this package yet. It must appear before it is used by %v", label.Subrepo, dependent) - } - if _, err := r.Parse(ctx, sl, label); err != nil { + pkg, err := func() (*core.Package, error) { + // If the target is in a subrepo that we don't know about yet, we must make sure that is defined first. + // If we already have it there's nothing to do here; it's been registered by whatever parse defined it. + if label.Subrepo != "" && r.state.Graph.Subrepo(label.Subrepo) == nil { + if err := r.ensureSubrepo(ctx, label, dependent); err != nil { return nil, err } } + return parse.Parse(r.state, label, dependent) + }() + if err != nil && !quiet { + r.state.LogBuildError(label, core.ParseFailed, err, "Failed to parse package") } - return parse.Parse(r.state, label, dependent) + return pkg, err }) } +// ensureSubrepo makes sure that the subrepo the given label is in has been defined. +// +// A name like `linux_amd64` is ambiguous: it could be a subrepo defined by a target somewhere, or one +// of the architecture subrepos, which are implicitly defined and so have no defining target anywhere. +// We resolve that by always preferring a real definition, and only falling back to the architecture +// interpretation once we know there isn't one. +func (r *runner) ensureSubrepo(ctx context.Context, label, dependent core.BuildLabel) error { + sl := label.SubrepoLabel(r.state) + // The subrepo would be defined by a target in the dependent's package, which means that package is + // the one currently being parsed - and since we didn't find the subrepo, the call that defines it + // hasn't been reached yet. We can't wait for that parse because we are that parse. + if inSamePackage(sl, dependent) { + return fmt.Errorf("subrepo %v is not defined in this package yet. It must appear before it is used by %v", label.Subrepo, dependent) + } + // Parsing the package that should define it registers the subrepo as a side effect. A missing BUILD + // file isn't fatal yet; that's exactly what we'd expect for an architecture subrepo. + _, err := r.tryParse(ctx, sl, label) + if err != nil && !errors.Is(err, parse.ErrMissingBuildFile) { + return err + } + if r.state.Graph.Subrepo(label.Subrepo) != nil { + return nil // The parse above defined it, we're done. + } + // Nothing defines it, so the only remaining possibility is an architecture subrepo. + if arch, ok := couldBeArch(label.Subrepo); ok { + r.state.Graph.MaybeAddSubrepo(core.SubrepoForArch(r.state, arch)) + return nil + } else if err != nil { + return err + } + return fmt.Errorf("Subrepo %s is not defined (referenced by %s)", label.Subrepo, dependent) +} + // inSamePackage returns true if the two labels are in the same package (and hence, if one of them is // currently being parsed, both are). func inSamePackage(label, dependent core.BuildLabel) bool { @@ -647,3 +692,14 @@ func (p *Progress) NumDone() int { func (p *Progress) NumParsing() int { return int(p.numParsing.Load()) } + +// couldBeArch returns the architecture for a potential subrepo name, if it could be one for +// cross-compiling. Note that this is only a syntactic check; a real subrepo can be named this way too, +// so a caller must satisfy itself that nothing else defines it before treating it as an architecture. +func couldBeArch(name string) (cli.Arch, bool) { + var arch cli.Arch + if err := arch.UnmarshalFlag(name); err != nil { + return arch, false + } + return arch, true +} From 0cfea81ef699975da72e432a295d8f32f1e3c5cd Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 11:28:38 +0100 Subject: [PATCH 33/44] update file --- REFACTOR_FAILURES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/REFACTOR_FAILURES.md b/REFACTOR_FAILURES.md index 8defe7fbed..467c71e0dc 100644 --- a/REFACTOR_FAILURES.md +++ b/REFACTOR_FAILURES.md @@ -84,6 +84,7 @@ The comparison needs the dependent, not the label — which means reversing the ## C. `CheckArchSubrepo` is consulted first, and it accepts anything SORT OF DONE in 0ba3093a522688f55a0025c02ff43a0f622b3809 +DONE in ba3fef2921b215d29ced22bb5336e65fd70fbbbc **Failing:** `//test/proto_plugin:proto_rules_test` ``` From 0c77751376d59cd4ca6eb911077123c5a12df377 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 11:47:57 +0100 Subject: [PATCH 34/44] Handle targets that are modified by post-build functions --- src/build/incrementality_test.go | 1 + src/core/build_target.go | 2 ++ src/parse/asp/builtins.go | 4 ++- src/plz/plz.go | 49 +++++++++++++++++++++++++------- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/build/incrementality_test.go b/src/build/incrementality_test.go index e74450413f..d1f222569f 100644 --- a/src/build/incrementality_test.go +++ b/src/build/incrementality_test.go @@ -111,6 +111,7 @@ var KnownFields = map[string]bool{ "mutex": true, "dependenciesRegistered": true, "finishedBuilding": true, + "ModifiedByCallback": true, // Used to save the rule hash rather than actually being hashed itself. "RuleHash": true, diff --git a/src/core/build_target.go b/src/core/build_target.go index cef43d87d0..78dd737e25 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -250,6 +250,8 @@ type BuildTarget struct { IsTextFile bool `print:"false"` // Marks that the target was added in a post-build function. AddedPostBuild bool `print:"false"` + // Marks that this target was modified by a pre or post build function + ModifiedByCallback bool `print:"false"` // If true, skips generating environment variables for sources; instead files will be generated in // the build environment containing the lists of sources as follows: // - _plz/srcs (equivalent to $SRCS) always diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index e2a82137bf..fbff731a96 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -1219,6 +1219,7 @@ func addDep(s *scope, args []pyObject) pyObject { exported := args[2].IsTruthy() runtime := args[3].IsTruthy() target.AddMaybeExportedDependency(dep, exported, false, false, runtime) + target.ModifiedByCallback = true return None } @@ -1253,8 +1254,9 @@ func addData(s *scope, args []pyObject) pyObject { } } } else { - log.Fatal("Unrecognised data type passed to add_data") + s.Error("Unrecognised data type passed to add_data") } + target.ModifiedByCallback = true return None } diff --git a/src/plz/plz.go b/src/plz/plz.go index 2784b6f374..c97ac7bd0f 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -326,21 +326,50 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { return err } - // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. + if target.ModifiedByCallback { + // A pre- or post-build function modified this target post parse, so we need to check its dependencies again. + g, gctx := errgroup.WithContext(ctx) + for dep := range target.BuildDependencyLabels() { + g.Go(func() error { + return r.buildDep(gctx, dep, target) + }) + } + if err := g.Wait(); err != nil { + return err + } + } // Okay, now the runtime dependencies can happen in parallel with the target itself. - deps := slices.Collect(target.RuntimeAndDataDependencies()) - if len(deps) == 0 { - return r.buildJustOne(target) + // 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 deps := slices.Collect(target.RuntimeAndDataDependencies()); len(deps) == 0 { + if err := r.buildJustOne(target); err != nil { + return err + } + } else { + g, gctx = errgroup.WithContext(ctx) + g.Go(func() error { + return r.buildJustOne(target) + }) + for _, dep := range deps { + g.Go(func() error { + return r.buildDep(gctx, dep, target) + }) + } + if err := g.Wait(); err != nil { + return err + } } + + if !target.ModifiedByCallback { + return nil + } + // It could have modified itself with its own post-build function, so we have to check runtime dpendencies again. + // This is a little unfortunate that we can't immediately distinguish from the case we checked above. g, gctx = errgroup.WithContext(ctx) - g.Go(func() error { - return r.buildJustOne(target) - }) - for _, dep := range deps { + for dep := range target.RuntimeAndDataDependencies() { g.Go(func() error { - _, err := r.Build(gctx, dep, target.Label) - return err + return r.buildDep(gctx, dep, target) }) } return g.Wait() From c7c3f38926cfbdae4a5ba175eefbb94e06951dc7 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 12:09:32 +0100 Subject: [PATCH 35/44] Recurse better --- src/plz/plz.go | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index c97ac7bd0f..d5b5814c80 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -71,6 +71,9 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress buildOnce: cmap.NewErrMap[core.BuildLabel, *core.BuildTarget](cmap.DefaultShardCount, func(l core.BuildLabel) uint64 { return cmap.XXHashes(l.Subrepo, l.PackageName, l.Name) }, nil), + parseOnce: cmap.New[core.BuildLabel, struct{}](cmap.DefaultShardCount, func(l core.BuildLabel) uint64 { + return cmap.XXHashes(l.Subrepo, l.PackageName, l.Name) + }), localLimiter: make(limiter, state.Config.Please.NumThreads), remoteLimiter: make(limiter, state.Config.NumRemoteExecutors()), anyRemote: state.Config.NumRemoteExecutors() > 0, @@ -136,6 +139,7 @@ type runner struct { arch cli.Arch progress *Progress buildOnce *cmap.ErrMap[core.BuildLabel, *core.BuildTarget] + parseOnce *cmap.Map[core.BuildLabel, struct{}] localLimiter limiter remoteLimiter limiter anyRemote bool @@ -224,32 +228,31 @@ func (r *runner) RecursiveParse(ctx context.Context, label, dependent core.Build if err != nil { return err } - // Deduplicate dependencies as early as we can here. - seen := map[core.BuildLabel]struct{}{} - g, ctx := errgroup.WithContext(ctx) + g, gctx := errgroup.WithContext(ctx) for _, target := range pkg.AllTargets() { for dep := range target.DeclaredDependencies() { - if _, present := seen[dep]; !present { - seen[dep] = struct{}{} - g.Go(func() error { - return r.recursiveParse(ctx, dep, target.Label) - }) - } + g.Go(func() error { + // N.B. No need to deduplicate these; recursiveParse does that for the whole walk. + return r.recursiveParse(gctx, dep, target.Label) + }) } } return g.Wait() } +// recursiveParse parses a target and, transitively, everything it depends on. func (r *runner) recursiveParse(ctx context.Context, label, dependent core.BuildLabel) error { + if !r.parseOnce.Add(label, struct{}{}) { + return nil // Someone else has this one; they're in the same errgroup so we needn't wait for them. + } target, err := r.parseTarget(ctx, label, dependent) if err != nil { return err } - g, ctx := errgroup.WithContext(ctx) + g, gctx := errgroup.WithContext(ctx) for dep := range target.DeclaredDependencies() { g.Go(func() error { - _, err := r.Parse(ctx, dep, target.Label) - return err + return r.recursiveParse(gctx, dep, target.Label) }) } return g.Wait() From c10c94ebec6fba7da56596f1e881357a2194479d Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 12:20:35 +0100 Subject: [PATCH 36/44] Change Dependencies not to panic --- src/core/build_target.go | 45 ++++++++++++++++++++++++++--------- src/core/build_target_test.go | 18 ++++++++++---- src/core/cycle_detector.go | 5 +++- src/core/stamp.go | 6 ++++- src/export/export.go | 7 +++++- src/gc/gc.go | 4 +++- src/parse/asp/builtins.go | 7 +++++- src/query/graph.go | 12 ++++++++-- src/test/coverage.go | 6 ++++- src/watch/watch.go | 4 +++- 10 files changed, 89 insertions(+), 25 deletions(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index 78dd737e25..88e8224e4f 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -585,7 +585,11 @@ func (target *BuildTarget) DeclaredDependenciesStrict() iter.Seq[BuildLabel] { // Dependencies returns the resolved dependencies of this target, applying any require/provide // relationships to map each declared dependency to the target(s) that actually satisfy it. // It requires the graph to look targets up, since a BuildTarget no longer caches these itself. -func (target *BuildTarget) Dependencies(graph *BuildGraph) []*BuildTarget { +// +// The second return is the labels of any dependencies that aren't in the graph. For most callers +// that indicates something has gone wrong and should be reported, but it's a legitimate state for +// anything that runs while the graph is still being built up (e.g. the cycle detector). +func (target *BuildTarget) Dependencies(graph *BuildGraph) ([]*BuildTarget, []BuildLabel) { target.mutex.RLock() labels := make([]BuildLabel, len(target.dependencies)) for i, dep := range target.dependencies { @@ -593,20 +597,30 @@ func (target *BuildTarget) Dependencies(graph *BuildGraph) []*BuildTarget { } target.mutex.RUnlock() ret := make(BuildTargets, 0, len(labels)) + var unresolved []BuildLabel for _, l := range labels { - depTarget := graph.TargetOrDie(l) + depTarget := graph.Target(l) + if depTarget == nil { + unresolved = append(unresolved, l) + continue + } for _, provided := range depTarget.ProvideFor(target) { - ret = append(ret, graph.TargetOrDie(provided)) + if t := graph.Target(provided); t != nil { + ret = append(ret, t) + } else { + unresolved = append(unresolved, provided) + } } } sort.Sort(ret) - return ret + return ret, unresolved } // ExternalDependencies returns the resolved dependencies of this target, with any internal // dependencies (i.e. "_target#tag" ones sharing this target's parent) flattened out to the -// external targets they in turn depend on. Require/provide relationships are applied as in Dependencies. -func (target *BuildTarget) ExternalDependencies(graph *BuildGraph) []*BuildTarget { +// external targets they in turn depend on. Require/provide relationships are applied as in Dependencies, +// as is the second return of any dependencies that aren't in the graph. +func (target *BuildTarget) ExternalDependencies(graph *BuildGraph) ([]*BuildTarget, []BuildLabel) { target.mutex.RLock() labels := make([]BuildLabel, len(target.dependencies)) for i, dep := range target.dependencies { @@ -614,19 +628,28 @@ func (target *BuildTarget) ExternalDependencies(graph *BuildGraph) []*BuildTarge } target.mutex.RUnlock() ret := make(BuildTargets, 0, len(labels)) + var unresolved []BuildLabel for _, l := range labels { - depTarget := graph.TargetOrDie(l) + depTarget := graph.Target(l) + if depTarget == nil { + unresolved = append(unresolved, l) + continue + } for _, provided := range depTarget.ProvideFor(target) { - dep := graph.TargetOrDie(provided) - if dep.Label.Parent() != target.Label { + dep := graph.Target(provided) + if dep == nil { + unresolved = append(unresolved, provided) + } else if dep.Label.Parent() != target.Label { ret = append(ret, dep) } else { - ret = append(ret, dep.ExternalDependencies(graph)...) + deps, u := dep.ExternalDependencies(graph) + ret = append(ret, deps...) + unresolved = append(unresolved, u...) } } } sort.Sort(ret) - return ret + return ret, unresolved } // BuildDependencies returns the build-time dependency labels of this target (i.e. not run-time dependencies, data, internal nor source). diff --git a/src/core/build_target_test.go b/src/core/build_target_test.go index 470a563d08..94a65eb874 100644 --- a/src/core/build_target_test.go +++ b/src/core/build_target_test.go @@ -420,11 +420,11 @@ func TestDependencies(t *testing.T) { target3 := makeTarget1("//src/core:target3", "", target1, target2) graph := graphWith(target1, target2, target3) assert.Empty(t, slices.Collect(target1.DeclaredDependencies())) - assert.Empty(t, target1.Dependencies(graph)) + assert.Empty(t, resolved(target1.Dependencies(graph))) assert.Equal(t, []BuildLabel{target1.Label}, slices.Collect(target2.DeclaredDependencies())) - assert.Equal(t, []*BuildTarget{target1}, target2.Dependencies(graph)) + assert.Equal(t, []*BuildTarget{target1}, resolved(target2.Dependencies(graph))) assert.Equal(t, []BuildLabel{target1.Label, target2.Label}, slices.Collect(target3.DeclaredDependencies())) - assert.Equal(t, []*BuildTarget{target1, target2}, target3.Dependencies(graph)) + assert.Equal(t, []*BuildTarget{target1, target2}, resolved(target3.Dependencies(graph))) } func TestBuildDependencies(t *testing.T) { @@ -502,7 +502,7 @@ func TestAddDependency(t *testing.T) { target2.AddMaybeExportedDependency(target1.Label, true, false, false, false) assert.Equal(t, []BuildLabel{target1.Label}, slices.Collect(target2.DeclaredDependencies())) assert.Equal(t, []BuildLabel{target1.Label}, slices.Collect(target2.ExportedDependencies())) - assert.Equal(t, []*BuildTarget{target1}, target2.Dependencies(graphWith(target1, target2))) + assert.Equal(t, []*BuildTarget{target1}, resolved(target2.Dependencies(graphWith(target1, target2)))) } func TestAddRuntimeDependency(t *testing.T) { @@ -748,7 +748,7 @@ func TestExternalDependencies(t *testing.T) { t2a := makeTarget1("//src/core:_target2#a", "PUBLIC", t1) t2 := makeTarget1("//src/core:target2", "PUBLIC", t2a) graph := graphWith(t1a, t1, t2a, t2) - assert.Equal(t, []*BuildTarget{t1}, t2.ExternalDependencies(graph)) + assert.Equal(t, []*BuildTarget{t1}, resolved(t2.ExternalDependencies(graph))) } func TestBuildTargetOwnBuildInputs(t *testing.T) { @@ -1044,6 +1044,14 @@ func makeTarget1(label, visibility string, deps ...*BuildTarget) *BuildTarget { return target } +// resolved unwraps a call to Dependencies / ExternalDependencies, requiring that everything resolved. +func resolved(deps []*BuildTarget, unresolved []BuildLabel) []*BuildTarget { + if len(unresolved) > 0 { + panic(fmt.Sprintf("dependencies not in graph: %s", unresolved)) + } + return deps +} + // graphWith returns a graph populated with the given targets, for tests that need dependency // resolution (which now happens against the graph rather than being cached on the target). func graphWith(targets ...*BuildTarget) *BuildGraph { diff --git a/src/core/cycle_detector.go b/src/core/cycle_detector.go index c25bdbe928..12d76aff79 100644 --- a/src/core/cycle_detector.go +++ b/src/core/cycle_detector.go @@ -35,7 +35,10 @@ func (c *cycleDetector) Check() *errCycle { return []*BuildTarget{target}, false } partial[target] = struct{}{} - for _, dep := range target.Dependencies(c.graph) { + // Ignore anything we can't resolve; we run while the build is still going on so it's + // entirely normal for parts of the graph not to exist yet. + deps, _ := target.Dependencies(c.graph) + for _, dep := range deps { if cycle, done := visit(dep); cycle != nil { if done || target == cycle[len(cycle)-1] { return cycle, true // This target is already in the cycle diff --git a/src/core/stamp.go b/src/core/stamp.go index 8262838491..44a8e28c7a 100644 --- a/src/core/stamp.go +++ b/src/core/stamp.go @@ -27,7 +27,11 @@ func populateStampInfo(state *BuildState, target *BuildTarget, info *stampInfo) AcceptedLicence: accepted, Labels: target.Labels, } - for _, dep := range target.Dependencies(state.Graph) { + deps, unresolved := target.Dependencies(state.Graph) + if len(unresolved) > 0 { + log.Fatalf("Can't stamp %s; dependencies not in build graph: %s", target.Label, unresolved) + } + for _, dep := range deps { if _, present := info.Targets[dep.Label]; !present { populateStampInfo(state, dep, info) } diff --git a/src/export/export.go b/src/export/export.go index e0fc87d81a..46bad491e8 100644 --- a/src/export/export.go +++ b/src/export/export.go @@ -186,7 +186,12 @@ func (e *export) export(target *core.BuildTarget) { } e.exportedTargets[target.Label] = true - for _, dep := range target.Dependencies(e.state.Graph) { + deps, unresolved := target.Dependencies(e.state.Graph) + if len(unresolved) > 0 { + // Carrying on would silently produce an exported repo that doesn't build. + log.Fatalf("Can't export %s; dependencies not in build graph: %s", target.Label, unresolved) + } + for _, dep := range deps { e.export(dep) } for _, subinclude := range e.state.Graph.PackageOrDie(target.Label).AllSubincludes(e.state.Graph) { diff --git a/src/gc/gc.go b/src/gc/gc.go index 622aec80d0..66e7859a6c 100644 --- a/src/gc/gc.go +++ b/src/gc/gc.go @@ -170,7 +170,9 @@ func addTarget(graph *core.BuildGraph, m targetMap, target *core.BuildTarget) { for dep := range target.DeclaredDependencies() { addTarget(graph, m, graph.Target(dep)) } - for _, dep := range target.Dependencies(graph) { + // As above, anything we can't resolve is simply skipped. + deps, _ := target.Dependencies(graph) + for _, dep := range deps { addTarget(graph, m, dep) } if target.Subrepo != nil && target.Subrepo.Target != nil { diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index fbff731a96..fb7359b38b 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -1181,7 +1181,12 @@ func getLabelsInternal(graph *core.BuildGraph, target *core.BuildTarget, prefix return } if !t.OutputIsComplete || t == target || all { - for _, dep := range t.Dependencies(graph) { + deps, unresolved := t.Dependencies(graph) + if len(unresolved) > 0 { + // Shouldn't happen; by the time this is callable the target's dependencies are built. + log.Fatalf("get_labels called on %s, but its dependencies aren't in the build graph: %s", t.Label, unresolved) + } + for _, dep := range deps { if !done[dep] { getLabels(dep, max(depth-1, -1)) } diff --git a/src/query/graph.go b/src/query/graph.go index 06a88d2cc5..0e32d07b38 100644 --- a/src/query/graph.go +++ b/src/query/graph.go @@ -133,7 +133,11 @@ func addJSONTarget(state *core.BuildState, graph *JSONGraph, label core.BuildLab }, } } - for _, dep := range target.Dependencies(state.Graph) { + deps, unresolved := target.Dependencies(state.Graph) + if len(unresolved) > 0 { + log.Fatalf("Can't generate graph for %s; dependencies not in build graph: %s", target.Label, unresolved) + } + for _, dep := range deps { addJSONTarget(state, graph, dep.Label, done) } } @@ -157,7 +161,11 @@ func makeJSONTarget(state *core.BuildState, target *core.BuildTarget) JSONTarget for _, out := range target.Outputs(state.Graph) { t.Outputs = append(t.Outputs, filepath.Join(target.Label.PackageName, out)) } - for _, dep := range target.Dependencies(state.Graph) { + deps, unresolved := target.Dependencies(state.Graph) + if len(unresolved) > 0 { + log.Fatalf("Can't generate graph for %s; dependencies not in build graph: %s", target.Label, unresolved) + } + for _, dep := range deps { t.Deps = append(t.Deps, dep.Label.String()) } // just use run 1 as this is only used to print the test dir diff --git a/src/test/coverage.go b/src/test/coverage.go index 5bd8ef1012..1386b094fc 100644 --- a/src/test/coverage.go +++ b/src/test/coverage.go @@ -76,7 +76,11 @@ func collectAllFiles(state *core.BuildState, target *core.BuildTarget, coverageF } } if deps { - for _, dep := range target.ExternalDependencies(state.Graph) { + extDeps, unresolved := target.ExternalDependencies(state.Graph) + if len(unresolved) > 0 { + log.Warning("Can't collect coverage for dependencies of %s; not in build graph: %s", target.Label, unresolved) + } + for _, dep := range extDeps { collectAllFiles(state, dep, coverageFiles, includeAllFiles, deps, doneTargets) } } diff --git a/src/watch/watch.go b/src/watch/watch.go index 7fc3f2ee6f..01ac403475 100644 --- a/src/watch/watch.go +++ b/src/watch/watch.go @@ -103,7 +103,9 @@ func startWatching(watcher *fsnotify.Watcher, state *core.BuildState, labels []c addSource(watcher, state, datum, dirs, files) } } - for _, dep := range target.Dependencies(state.Graph) { + // Anything unresolved just doesn't get watched; it's not worth failing the whole watch for. + deps, _ := target.Dependencies(state.Graph) + for _, dep := range deps { startWatch(dep) } pkg := state.Graph.PackageOrDie(target.Label) From eca7652c281a9558415acec06222b73a7cecfc37 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 12:30:21 +0100 Subject: [PATCH 37/44] Implement --keep_going semantics --- src/plz/plz.go | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index d5b5814c80..44b1d9f148 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -60,7 +60,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress }() ctx, cancel := context.WithCancel(context.Background()) - g, ctx := errgroup.WithContext(ctx) + g, ctx := group(state, ctx) state.Cancel = cancel r := runner{ @@ -110,7 +110,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress } // Reset the group & context for next time ctx, cancel := context.WithCancel(context.Background()) - g, ctx = errgroup.WithContext(ctx) + g, ctx = group(state, ctx) state.Cancel = cancel r.tasks = g } @@ -213,6 +213,18 @@ func (r *runner) ensureSubrepo(ctx context.Context, label, dependent core.BuildL return fmt.Errorf("Subrepo %s is not defined (referenced by %s)", label.Subrepo, dependent) } +// group returns an errgroup to run a set of tasks in, and the context to run them with. +// +// Normally that context is cancelled as soon as any of them fails, which stops us starting more work. +// With --keep_going we don't cancel anything, so everything that can still be built gets built; the +// group waits for all of it either way and Wait still returns the first error. +func group(state *core.BuildState, ctx context.Context) (*errgroup.Group, context.Context) { + if state.KeepGoing { + return &errgroup.Group{}, ctx + } + return errgroup.WithContext(ctx) +} + // inSamePackage returns true if the two labels are in the same package (and hence, if one of them is // currently being parsed, both are). func inSamePackage(label, dependent core.BuildLabel) bool { @@ -228,7 +240,7 @@ func (r *runner) RecursiveParse(ctx context.Context, label, dependent core.Build if err != nil { return err } - g, gctx := errgroup.WithContext(ctx) + g, gctx := group(r.state, ctx) for _, target := range pkg.AllTargets() { for dep := range target.DeclaredDependencies() { g.Go(func() error { @@ -249,7 +261,7 @@ func (r *runner) recursiveParse(ctx context.Context, label, dependent core.Build if err != nil { return err } - g, gctx := errgroup.WithContext(ctx) + g, gctx := group(r.state, ctx) for dep := range target.DeclaredDependencies() { g.Go(func() error { return r.recursiveParse(gctx, dep, target.Label) @@ -312,7 +324,7 @@ func (r *runner) buildDep(ctx context.Context, dep core.BuildLabel, target *core // buildOne builds a single target (which cannot be a pseudo-label like :all) func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { - g, gctx := errgroup.WithContext(ctx) + g, gctx := group(r.state, ctx) for dep := range target.BuildDependencyLabels() { g.Go(func() error { return r.buildDep(gctx, dep, target) @@ -331,7 +343,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { if target.ModifiedByCallback { // A pre- or post-build function modified this target post parse, so we need to check its dependencies again. - g, gctx := errgroup.WithContext(ctx) + g, gctx := group(r.state, ctx) for dep := range target.BuildDependencyLabels() { g.Go(func() error { return r.buildDep(gctx, dep, target) @@ -350,7 +362,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { return err } } else { - g, gctx = errgroup.WithContext(ctx) + g, gctx = group(r.state, ctx) g.Go(func() error { return r.buildJustOne(target) }) @@ -369,7 +381,7 @@ func (r *runner) buildOne(ctx context.Context, target *core.BuildTarget) error { } // It could have modified itself with its own post-build function, so we have to check runtime dpendencies again. // This is a little unfortunate that we can't immediately distinguish from the case we checked above. - g, gctx = errgroup.WithContext(ctx) + g, gctx = group(r.state, ctx) for dep := range target.RuntimeAndDataDependencies() { g.Go(func() error { return r.buildDep(gctx, dep, target) @@ -393,7 +405,7 @@ func (r *runner) buildAll(ctx context.Context, label, dependent core.BuildLabel) if err != nil { return err } - g, gctx := errgroup.WithContext(ctx) + g, gctx := group(r.state, ctx) for _, target := range pkg.AllTargets() { if r.state.ShouldInclude(target) { g.Go(func() error { @@ -476,7 +488,7 @@ func (r *runner) Test(ctx context.Context, label, dependent core.BuildLabel) err if err != nil { return err } - g, ctx := errgroup.WithContext(ctx) + g, ctx := group(r.state, ctx) for _, target := range pkg.AllTargets() { if r.state.ShouldInclude(target) { g.Go(func() error { @@ -597,7 +609,7 @@ func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest // the interpreter. We have to actually register them otherwise this will return before we build any // transitive subincludes. func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState, parser *asp.Parser) error { - g, ctx := errgroup.WithContext(ctx) + g, ctx := group(r.state, ctx) preloads := state.GetPreloadedSubincludes() for _, inc := range preloads { if inc.IsPseudoTarget() { From 851e20d1ffbd109b81f8826c8edba38b86046f0a Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 12:32:00 +0100 Subject: [PATCH 38/44] Don't need this any more --- REFACTOR_FAILURES.md | 185 ------------------------------------------- 1 file changed, 185 deletions(-) delete mode 100644 REFACTOR_FAILURES.md diff --git a/REFACTOR_FAILURES.md b/REFACTOR_FAILURES.md deleted file mode 100644 index 467c71e0dc..0000000000 --- a/REFACTOR_FAILURES.md +++ /dev/null @@ -1,185 +0,0 @@ -# Outstanding `//test/...` failures from the `src/plz/plz.go` refactor - -Status as of the run below. 12 failing test targets, grouped by root cause. - -``` -plz plz test //test/... //src/... -o test.timeout:60s --exclude benchmark -261 test targets and 1041 tests run; 1027 passed, 12 errored, 2 skipped. -``` - -All of `//src/...` passes. The failures are all in `//test/...` (e2e). - -## Already fixed - -Recorded for context; these are done and are not in the 12 below. - -| Problem | Symptom | Fix | -| --- | --- | --- | -| `buildOne` re-derived its second errgroup from a context that `Wait()` had already cancelled | Large builds stopped early with `context canceled` and no error | Don't shadow `ctx`; use a separate `gctx` per group | -| `buildAll` called `buildOne` directly, bypassing the `buildOnce` memo | `panic: close of closed channel` in `FinishBuild` when a target was reached both via `:all` and as a dependency | Route through `Build` | -| `Build` claimed the `buildOnce` entry around `parseTarget` | Deadlock when parsing a package re-entered `Build` for a label defined in that same file (`subinclude(":defs_2")`) | Claim the entry after parsing, so only the build phase is memoised | -| `RuntimeDependencies()` widened to include data deps | `query print -f runtime_deps` printed data deps too | Split into `RuntimeDependencies()` and `RuntimeAndDataDependencies()` | -| `AddOriginalTarget` called inside the task goroutine | Nondeterministic original-target order, visible as `.gitignore` write order | Call it synchronously in `queueTask`, in walk order | - -## A. A failed package parse strands every other waiter -# DONE in 89ff16978efcc727a8d1272eedecd00c2c8288d6 and d2a0d2dd773a8d651d9fac991a04a2a7e67a292b - -**Failing:** `//test/preloaded_subinc:preload_subinc_test` (hangs until the test timeout) - -`BuildGraph.PackageOrWait` goes through `cmap.Map.GetOrWait`: the first caller claims the -slot and everyone else gets a wait channel. If that first caller's parse *fails*, the package -is never `Set`, so the channel is never closed and the other waiters block forever. - -In this test, preload `//plugins:shell` fails with `build file not found: ... there's no BUILD -file in plugins/`, and `///pleasings//k8s:k8s` then waits on package `plugins` indefinitely: - -``` -core.(*BuildGraph).PackageOrWait src/core/graph.go:117 -plz.(*runner).Parse src/plz/plz.go:144 -plz.(*runner).Parse src/plz/plz.go:158 -plz.(*runner).parseTarget src/plz/plz.go:211 -plz.(*runner).Build src/plz/plz.go:337 -plz.(*runner).RegisterPreloads src/plz/plz.go:525 -``` - -The packages map has no error path, unlike `cmap.ErrMap` which `buildOnce` uses — a failing -parse has no way to publish its failure to waiters. Giving `graph.packages` the same -error-carrying treatment looks like the fix. - -`RegisterPreloads` now takes a context (it previously used a bare `errgroup.Group`), so a -failing preload can at least cancel its siblings. That turns the hang into an error but -doesn't fix the underlying stranding — any parse failure anywhere still relies entirely on -context cancellation to release waiters. - -## B. `runner.Parse` lost the `dependent` label -DONE in 0ba3093a522688f55a0025c02ff43a0f622b3809 -**Failing:** `//test/subrepo/same_package_error:same_package_error` (hangs until the test timeout) - -The test asserts the error `subrepo X is not defined in this package yet` for a BUILD file -that subincludes from a subrepo it declares further down: - -```python -subinclude("///subrepo//:foo") - -local_repository(name = "subrepo", path = "subrepo") -``` - -The old `parse.checkSubrepo` guarded this with `inSamePackage(sl, dependent)`. The current -check at `src/plz/plz.go:154` compares `sl` against `label` instead: - -```go -if sl.Subrepo == label.Subrepo && sl.PackageName == label.PackageName { - // TODO(peter): Unsure if this is a legit case or not. - return nil, fmt.Errorf("subinclude from within same package of a subrepo") -} -``` - -For `///subrepo//:foo`, `sl` is `//:subrepo`, so `sl.Subrepo` (`""`) never equals -`label.Subrepo` (`"subrepo"`) and the guard can't fire. We then recurse into `r.Parse(sl)` -and land in `PackageOrWait` for the package this goroutine is *already* parsing. - -The comparison needs the dependent, not the label — which means reversing the -`TODO(peter): can we drop the dependent here?` at `src/plz/plz.go:163` -(`parse.Parse(r.state, label, label)`). The answer appears to be no. - -## C. `CheckArchSubrepo` is consulted first, and it accepts anything -SORT OF DONE in 0ba3093a522688f55a0025c02ff43a0f622b3809 -DONE in ba3fef2921b215d29ced22bb5336e65fd70fbbbc -**Failing:** `//test/proto_plugin:proto_rules_test` - -``` -CRITICAL: Found multiple definitions for subrepo 'go_proto' - (... Target://plugins:go-proto ... Arch:{OS:linux Arch:amd64} IsCrossCompile:false) - (... Target: ... Arch:{OS:go Arch:proto} IsCrossCompile:true) -``` - -`cli.Arch.UnmarshalFlag` accepts any `x_y` string with no `/` or `@`, so `go_proto` parses as -OS=`go`, Arch=`proto`. `src/plz/plz.go:152` checks that *before* trying to resolve the real -subrepo definition, registering a phantom arch subrepo that later collides with the genuine -one from `//plugins:go-proto`. - -Previously `CheckArchSubrepo` was only a fallback, reached from `maybeParseSubrepoPackage` -after the normal subrepo lookup had already failed. Restoring that ordering is the fix, but -it needs care: the failed normal lookup goes through `parse.Parse`, which calls -`state.LogBuildError`, so a naive "try normal, fall back to arch" would log a spurious parse -error for every legitimate arch subrepo. - -## D. Cross-compiling doesn't take effect - -**Failing:** `//test/cross_compile:location_cross_compile_test`, `//test/cross_compile:x86_test` - -``` -Unexpected contents of file: test/cross_compile/arch.txt -unexpected architecture of binary -``` - -Targets under `///linux_x86//` are being built for the host architecture. Plausibly downstream -of C, since the arch subrepo is now registered from a different place, but that is unverified — -this one needs its own investigation. - -## E. `BuildTarget.Dependencies` panics on lazily-resolved deps - -**Failing:** `//test/export:export_src_please_test`, `//test/export/test_go_bin:export_go_target`, -`//test/export/test_native_target_with_go_dep:export_native_target_with_go_dep` - -`src/core/build_target.go:598` resolves each declared dep with `graph.TargetOrDie`. Since -dependency resolution moved out of `build_target`, the graph legitimately may not contain a -declared dep, and the lookup panics. Two callers hit it: - -``` -panic: Target ///third_party/go/github.com_dustin_go-humanize//:go-humanize not found in build graph -core.(*BuildTarget).Dependencies src/core/build_target.go:598 -export.(*export).export src/export/export.go:189 -export.ToDir src/export/export.go:51 -``` - -``` -panic: Target //_please:arcat not found in build graph -core.(*BuildTarget).Dependencies src/core/build_target.go:598 -core.(*cycleDetector).Check.func1 src/core/cycle_detector.go:38 -core.(*BuildState).checkForCycles src/core/state.go:537 -``` - -Note the second one fires from a background goroutine, so it takes the process down -regardless of what the build is doing. - -## F. Post-build-added runtime deps are never built - -**Failing:** `//test/runtime_deps:runtime_deps_test`, `//test/plz_run:_add_data_target_test` - -``` -ERROR: //test:runtime_deps_test_case failed: - cannot calculate hash for plz-out/gen/test/post_build_runtime_dep: file does not exist -ERROR: //src:_add_data_file_and_target failed: - cannot calculate hash for plz-out/gen/src/foo.txt: file does not exist -``` - -`buildOne` snapshots the deps *before* the target itself is built: - -```go -deps := slices.Collect(target.RuntimeAndDataDependencies()) -``` - -so anything a post-build function adds via `add_runtime_dep` / `add_data` is missed. This is -the existing `TODO(peter): Need to handle targets getting extra deps added by post-build -functions here.` at `src/plz/plz.go:277` — the collect needs redoing once the target has -finished building. - -## G. `--keep_going` is not implemented - -**Failing:** `//test/keep_going:plz_build_all` - -``` -Build stopped after 40ms. 2 targets failed: //package:fail, //package:fail2 -plz-out/gen/package/dep_pass doesnt exist -``` - -`state.KeepGoing` is set in `src/please.go:1149` and read nowhere in `src/plz/plz.go`. The -errgroups short-circuit on the first error, so unrelated targets like `//package:dep_pass` -never get built. An `errgroup` can't express this on its own — the group needs to run -everything and collect errors when `KeepGoing` is set. - -## Suggested order - -A and B first: both are hangs, and both come down to the parse path waiting on a channel that -nothing guarantees will be closed. C then D, since D may fall out of C. From f494fc1abb198f4c98892cb5ca3a875fcc37951a Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 12:46:14 +0100 Subject: [PATCH 39/44] Fix target counts --- src/plz/plz.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plz/plz.go b/src/plz/plz.go index 44b1d9f148..b1f0195ee6 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -442,7 +442,9 @@ func (r *runner) Build(ctx context.Context, label, dependent core.BuildLabel) (* // testOne tests one single target func (r *runner) testOne(ctx context.Context, target *core.BuildTarget, dependent core.BuildLabel) error { - r.progress.numTotal.Add(int64(r.state.NumTestRuns)) + if target.IsTest() { + r.progress.numTotal.Add(int64(r.state.NumTestRuns)) + } if _, err := r.Build(ctx, target.Label, dependent); err != nil { return err } From 5cf207ce3bc31a1f9a3dfb8ba9cdcd9a9faa7215 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 13:22:10 +0100 Subject: [PATCH 40/44] Drop WaitForTarget, it is not used outside tests any more --- src/core/graph.go | 22 ---------------------- src/core/graph_benchmark_test.go | 16 +++------------- tools/build_langserver/lsp/lsp_test.go | 9 ++++----- 3 files changed, 7 insertions(+), 40 deletions(-) diff --git a/src/core/graph.go b/src/core/graph.go index 8a21eeaf75..c30824c9f6 100644 --- a/src/core/graph.go +++ b/src/core/graph.go @@ -71,28 +71,6 @@ func (graph *BuildGraph) TargetOrDie(label BuildLabel) *BuildTarget { return target } -// WaitForTarget returns the given target, waiting for it to be added if it isn't yet. -// It returns nil if the target finally turns out not to exist. -func (graph *BuildGraph) WaitForTarget(label BuildLabel) *BuildTarget { - t, tch, _ := graph.targets.GetOrWait(label) - if t != nil { - return t - } - p, pch, _ := graph.packages.GetOrWait(packageKey{Name: label.PackageName, Subrepo: label.Subrepo}) - if p != nil { - // Check target again to avoid race conditions - return graph.Target(label) - } - // Now we need to wait for either (hopefully) the target or its package to exist. - // Either the target will, which is fine, or if the package appears but the target doesn't - // we will conclude it doesn't exist. - select { - case <-tch: - case <-pch: - } - return graph.Target(label) -} - // PackageByLabel retrieves a package from the graph using the appropriate parts of the given label. // The Name entry is ignored. func (graph *BuildGraph) PackageByLabel(label BuildLabel) *Package { diff --git a/src/core/graph_benchmark_test.go b/src/core/graph_benchmark_test.go index 0fcfc04a11..f00f9581c3 100644 --- a/src/core/graph_benchmark_test.go +++ b/src/core/graph_benchmark_test.go @@ -35,20 +35,10 @@ func BenchmarkTargetLookup(b *testing.B) { graph.TargetOrDie(targets[i&targetIndexMask].Label) } }) - - // This benchmarks the best case of calling WaitForTarget, where the targets already exist, - // so it should perform identically to Simple above. - b.Run("WaitForTargetFast", func(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - graph.WaitForTarget(targets[i&targetIndexMask].Label) - } - }) } -// BenchmarkWaitForTargetSlow is a more complex benchmark that tests targets being added as they are -// being waited on. -func BenchmarkWaitForTargetSlow(b *testing.B) { +// BenchmarkConcurrentTargetLookup tests targets being looked up at the same time as they're added. +func BenchmarkConcurrentTargetLookup(b *testing.B) { const parallelism = 8 var wg sync.WaitGroup wg.Add(parallelism * 2) @@ -67,7 +57,7 @@ func BenchmarkWaitForTargetSlow(b *testing.B) { lookupTargets := func() { for _, target := range targets { - graph.WaitForTarget(target.Label) + graph.Target(target.Label) } wg.Done() } diff --git a/tools/build_langserver/lsp/lsp_test.go b/tools/build_langserver/lsp/lsp_test.go index 8d05240f8c..c2f51521f2 100644 --- a/tools/build_langserver/lsp/lsp_test.go +++ b/tools/build_langserver/lsp/lsp_test.go @@ -15,7 +15,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/thought-machine/please/src/cli" - "github.com/thought-machine/please/src/core" ) func init() { @@ -629,10 +628,10 @@ func (h *Handler) CurrentContent(doc string) string { // WaitForPackage blocks until the given package has been parsed. func (h *Handler) WaitForPackage(pkg string) { - // this can only be described as 'grotty', but it is test code - h.state.Graph.WaitForTarget(core.BuildLabel{PackageName: pkg}) - if h.state.Graph.Package(pkg, "") == nil { - log.Fatalf("package %s doesn't exist", pkg) + // As with WaitForPackageTree below, polling is a bit yucky, but it's the only way of syncing + // up to this without interfering with the parse we're waiting on. + for h.state.Graph.Package(pkg, "") == nil { + time.Sleep(5 * time.Millisecond) } } From b7a83fe770ef4f04ec1cda6b9fde30336642e161 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 13:25:01 +0100 Subject: [PATCH 41/44] Fix another hang --- src/cmap/cmap.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/cmap/cmap.go b/src/cmap/cmap.go index f8058ef732..db2e8d0826 100644 --- a/src/cmap/cmap.go +++ b/src/cmap/cmap.go @@ -66,9 +66,10 @@ func (m *Map[K, V]) Set(key K, val V) { } // Get returns the value corresponding to the given key, or its zero value if the key doesn't exist in the map. +// N.B. Unlike GetOrWait this never inserts anything; a caller that isn't going to set the value must not +// leave an entry behind for others to wait on. func (m *Map[K, V]) Get(key K) V { - v, _, _ := m.shards[m.hasher(key)&m.mask].Get(key) - return v + return m.shards[m.hasher(key)&m.mask].get(key) } func (m *Map[K, V]) Contains(key K) bool { @@ -159,6 +160,14 @@ func (s *shard[K, V]) LazySet(key K, f func() V) (V, bool) { return v, true } +// get returns the value for a key, or its zero value if it isn't present. +// Unlike Get it never inserts anything, so it's safe for callers that only want to read. +func (s *shard[K, V]) get(key K) V { + s.l.RLock() + defer s.l.RUnlock() + return s.m[key].Val +} + // Get returns the value for a key or, if not present, a channel that it can be waited // on for. // Exactly one of the target or channel will be returned. From 186f8ee637448cac8ba212c158d50c7459884f0c Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 13:28:49 +0100 Subject: [PATCH 42/44] Refactor cmap since some functions are unused now --- src/cmap/cerrmap.go | 17 ++--------------- src/cmap/cmap.go | 44 +++++-------------------------------------- src/cmap/cmap_test.go | 42 +++++++++++++++-------------------------- 3 files changed, 22 insertions(+), 81 deletions(-) diff --git a/src/cmap/cerrmap.go b/src/cmap/cerrmap.go index c2cd946c1a..bccde4a9a7 100644 --- a/src/cmap/cerrmap.go +++ b/src/cmap/cerrmap.go @@ -36,13 +36,6 @@ func (m *ErrMap[K, V]) Add(key K, val V) bool { return m.m.Add(key, errV[V]{Val: val}) } -// AddOrGet either adds a new item (if the key doesn't exist) or gets the existing one. -// It returns true if the item was inserted, false if it already existed (in which case it won't be inserted) -func (m *ErrMap[K, V]) AddOrGet(key K, f func() V) (V, bool, error) { - v, present := m.m.AddOrGet(key, func() errV[V] { return errV[V]{Val: f()} }) - return v.Val, present, v.Err -} - // Set is the equivalent of `map[key] = val`. // It always overwrites any key that existed before. func (m *ErrMap[K, V]) Set(key K, val V) { @@ -61,16 +54,10 @@ func (m *ErrMap[K, V]) Get(key K) (V, error) { return v.Val, v.Err } -// GetOrWait returns the value corresponding to the given key, or a channel to wait on if it doesn't exist in the map. -func (m *ErrMap[K, V]) GetOrWait(key K) (V, <-chan struct{}, error) { - v, ch, _ := m.m.GetOrWait(key) - return v.Val, ch, v.Err -} - // GetOrSet returns the value if set, or an error if one has been set. // If nothing has been set for the key, it runs the given function to generate the value and then sets it. func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) { - v, wait, first := m.m.GetOrWait(key) + v, wait, first := m.m.getOrWait(key) if v.Err != nil { return v.Val, v.Err } else if first { @@ -91,7 +78,7 @@ func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) { // GetOrSetCtx is like GetOrSet but accepts a context that can be cancelled. func (m *ErrMap[K, V]) GetOrSetCtx(ctx context.Context, key K, f func() (V, error)) (V, error) { - v, wait, first := m.m.GetOrWait(key) + v, wait, first := m.m.getOrWait(key) if v.Err != nil { return v.Val, v.Err } else if first { diff --git a/src/cmap/cmap.go b/src/cmap/cmap.go index db2e8d0826..31ad6504b8 100644 --- a/src/cmap/cmap.go +++ b/src/cmap/cmap.go @@ -53,12 +53,6 @@ func (m *Map[K, V]) Add(key K, val V) bool { return m.shards[m.hasher(key)&m.mask].Set(key, val, false) } -// AddOrGet either adds a new item (if the key doesn't exist, calling the given function to create it) or gets the existing one. -// It returns true if the item was inserted, false if it already existed (in which case it won't be inserted) -func (m *Map[K, V]) AddOrGet(key K, f func() V) (V, bool) { - return m.shards[m.hasher(key)&m.mask].LazySet(key, f) -} - // Set is the equivalent of `map[key] = val`. // It always overwrites any key that existed before. func (m *Map[K, V]) Set(key K, val V) { @@ -66,23 +60,16 @@ func (m *Map[K, V]) Set(key K, val V) { } // Get returns the value corresponding to the given key, or its zero value if the key doesn't exist in the map. -// N.B. Unlike GetOrWait this never inserts anything; a caller that isn't going to set the value must not -// leave an entry behind for others to wait on. func (m *Map[K, V]) Get(key K) V { - return m.shards[m.hasher(key)&m.mask].get(key) + return m.shards[m.hasher(key)&m.mask].Get(key) } func (m *Map[K, V]) Contains(key K) bool { return m.shards[m.hasher(key)&m.mask].Contains(key) } -// GetOrWait returns the value or, if the key isn't present, a channel that it can be waited -// on for. The caller will need to call Get again after the channel closes. -// If the channel is non-nil, then val will exist in the map; otherwise it will have its zero value. -// The third return value is true if this is the first call that is awaiting this key. -// It's always false if the key exists. -func (m *Map[K, V]) GetOrWait(key K) (val V, wait <-chan struct{}, first bool) { - return m.shards[m.hasher(key)&m.mask].Get(key) +func (m *Map[K, V]) getOrWait(key K) (val V, wait <-chan struct{}, first bool) { + return m.shards[m.hasher(key)&m.mask].GetOrWait(key) } // Values returns a slice of all the current values in the map. @@ -139,30 +126,9 @@ func (s *shard[K, V]) Set(key K, val V, overwrite bool) bool { return true } -// LazySet is like Set but calls the given function to construct the object only if needed. -// It also returns the value that is now set in the map (whether overwritten or not). -func (s *shard[K, V]) LazySet(key K, f func() V) (V, bool) { - s.l.Lock() - defer s.l.Unlock() - if existing, present := s.m[key]; present { - if existing.Wait == nil { - return existing.Val, false // already added - } - // Hasn't been added, but something is waiting for it to be. - v := f() - s.m[key] = awaitableValue[V]{Val: v} - close(existing.Wait) - existing.Wait = nil - return v, true - } - v := f() - s.m[key] = awaitableValue[V]{Val: v} - return v, true -} - // get returns the value for a key, or its zero value if it isn't present. // Unlike Get it never inserts anything, so it's safe for callers that only want to read. -func (s *shard[K, V]) get(key K) V { +func (s *shard[K, V]) Get(key K) V { s.l.RLock() defer s.l.RUnlock() return s.m[key].Val @@ -172,7 +138,7 @@ func (s *shard[K, V]) get(key K) V { // on for. // Exactly one of the target or channel will be returned. // The third value is true if it is the first call that is waiting on this value. -func (s *shard[K, V]) Get(key K) (val V, wait <-chan struct{}, first bool) { +func (s *shard[K, V]) GetOrWait(key K) (val V, wait <-chan struct{}, first bool) { s.l.RLock() if v, ok := s.m[key]; ok { s.l.RUnlock() diff --git a/src/cmap/cmap_test.go b/src/cmap/cmap_test.go index 2c4394f8a8..f06b26341b 100644 --- a/src/cmap/cmap_test.go +++ b/src/cmap/cmap_test.go @@ -26,47 +26,38 @@ func TestMap(t *testing.T) { assert.Equal(t, []int{5, 7}, vals) } +// TestWait covers the awaiting primitive directly; it's only reachable through ErrMap now, +// but it's the bit with the interesting concurrency so it's worth pinning down here. func TestWait(t *testing.T) { m := New[int, int](DefaultShardCount, hashInts) - v, ch, first := m.GetOrWait(5) + v, ch, first := m.getOrWait(5) assert.Equal(t, 0, v) // Should be the zero value assert.True(t, first) // We're the first to request it go func() { m.Set(5, 7) }() <-ch - v, ch, first = m.GetOrWait(5) + v, ch, first = m.getOrWait(5) assert.Nil(t, ch) assert.Equal(t, 7, v) assert.False(t, first) } +func TestGetDoesntInsert(t *testing.T) { + m := New[int, int](DefaultShardCount, hashInts) + assert.Equal(t, 0, m.Get(5)) + // A failed lookup must not leave an entry behind; anything that later tries to set this key + // would find something already waiting on it and never get to do the work. + assert.False(t, m.Contains(5)) +} + func TestReAdd(t *testing.T) { m := New[int, int](DefaultShardCount, hashInts) assert.True(t, m.Add(5, 7)) assert.False(t, m.Add(5, 7)) - v, ch, first := m.GetOrWait(5) - assert.Nil(t, ch) - assert.Equal(t, 7, v) - assert.False(t, first) + assert.Equal(t, 7, m.Get(5)) m.Set(5, 8) - v, ch, first = m.GetOrWait(5) - assert.Nil(t, ch) - assert.Equal(t, 8, v) - assert.False(t, first) -} - -func TestAddOrGet(t *testing.T) { - m := New[int, int](DefaultShardCount, hashInts) - x, inserted := m.AddOrGet(5, func() int { return 7 }) - assert.True(t, inserted) - assert.Equal(t, 7, x) - x, inserted = m.AddOrGet(5, func() int { return 8 }) - assert.False(t, inserted) - assert.Equal(t, 7, x) - x, inserted = m.AddOrGet(8, func() int { return 9 }) - assert.True(t, inserted) - assert.Equal(t, 9, x) + assert.Equal(t, 8, m.Get(5)) } func TestShardCount(t *testing.T) { @@ -91,10 +82,7 @@ func TestResize(t *testing.T) { m.Set(i, i) } for i := 0; i < n; i++ { - v, ch, first := m.GetOrWait(i) - assert.Equal(t, i, v, "Key %d appears to be not set or set incorrectly", i) - assert.Nil(t, ch) - assert.False(t, first) + assert.Equal(t, i, m.Get(i), "Key %d appears to be not set or set incorrectly", i) } }) } From b666b5084ef142b882f10c000d04f1e4b8d1907e Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 13:50:44 +0100 Subject: [PATCH 43/44] Add licence --- third_party/python/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/third_party/python/BUILD b/third_party/python/BUILD index ed0b0c2fe4..2d16ec3fc0 100644 --- a/third_party/python/BUILD +++ b/third_party/python/BUILD @@ -73,4 +73,5 @@ python_wheel( pip_library( name = "progress", version = "1.5", + licences = ["ISC"], ) From 265a11779b8671ea742533c3179b467d1346c569 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 9 Aug 2026 14:03:09 +0100 Subject: [PATCH 44/44] Cut out unused build state, and fix up unbuilt deps message --- src/build/build_step_stress_test.go | 1 - src/core/build_target.go | 27 ++++----------------- src/output/shell_output.go | 37 +++++++++++++++++++---------- 3 files changed, 30 insertions(+), 35 deletions(-) diff --git a/src/build/build_step_stress_test.go b/src/build/build_step_stress_test.go index 02d9930909..045db96d91 100644 --- a/src/build/build_step_stress_test.go +++ b/src/build/build_step_stress_test.go @@ -54,7 +54,6 @@ func addTarget(state *core.BuildState, i int) *core.BuildTarget { // Create and add a new target, with a parent and a dependency. target := core.NewBuildTarget(label(i)) target.IsFilegroup = true // Will mean it doesn't have to shell out to anything. - target.SetState(core.Active) target.Test = new(core.TestFields) state.Graph.AddTarget(target) if i <= size { diff --git a/src/core/build_target.go b/src/core/build_target.go index 88e8224e4f..2c5b13125b 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -210,7 +210,8 @@ type BuildTarget struct { // The content of text_file() rules FileContent string `name:"content"` // Represents the state of this build target (see below) - state int32 `print:"false"` + // TODO(peter): we can just make this a public field now, it doesn't require atomics any more. + state BuildTargetState `print:"false"` // If true, the target is needed for a subinclude and therefore we will have to make sure its // outputs are available locally when built. neededForSubinclude atomic.Bool `print:"false"` @@ -344,9 +345,6 @@ type BuildTargetState uint8 // The available states for a target. const ( Inactive BuildTargetState = iota // Target isn't used in current build - Semiactive // Target would be active if we needed a build - Active // Target is going to be used in current build - Pending // Target is ready to be built but not yet started. Building // Target is currently being built Stopped // We stopped building the target because we'd gone as far as needed. Built // Target has been successfully built @@ -364,12 +362,6 @@ func (s BuildTargetState) String() string { switch s { case Inactive: return "Inactive" - case Semiactive: - return "Semiactive" - case Active: - return "Active" - case Pending: - return "Pending" case Building: return "Building" case Stopped: @@ -403,7 +395,7 @@ func (s BuildTargetState) IsBuilt() bool { func NewBuildTarget(label BuildLabel) *BuildTarget { return &BuildTarget{ Label: label, - state: int32(Inactive), + state: Inactive, BuildingDescription: DefaultBuildingDescription, } } @@ -1191,21 +1183,12 @@ func (target *BuildTarget) IsSourceOnlyDep(label BuildLabel) bool { // State returns the target's current state. func (target *BuildTarget) State() BuildTargetState { - return BuildTargetState(atomic.LoadInt32(&target.state)) + return target.state } // SetState sets a target's current state. func (target *BuildTarget) SetState(state BuildTargetState) { - atomic.StoreInt32(&target.state, int32(state)) -} - -// SyncUpdateState oves the target's state from before to after via a lock. -// Returns true if successful, false if not (which implies something else changed the state first). -// The nature of our build graph ensures that most transitions are only attempted by -// one thread simultaneously, but this one can be attempted by several at once -// (eg. if a depends on b and c, which finish building simultaneously, they race to queue a). -func (target *BuildTarget) SyncUpdateState(before, after BuildTargetState) bool { - return atomic.CompareAndSwapInt32(&target.state, int32(before), int32(after)) + target.state = state } // AddLabel adds the given label to this target if it doesn't already have it. diff --git a/src/output/shell_output.go b/src/output/shell_output.go index 707a8561c3..46850dd0e3 100644 --- a/src/output/shell_output.go +++ b/src/output/shell_output.go @@ -86,7 +86,7 @@ loop: } else if (state.NeedHashesOnly || state.PrepareOnly || shell) && target.State() == core.Stopped { // Do nothing, we will output about this shortly. } else if target.State() < core.Built && len(bt.FailedTargets) == 0 && !target.AddedPostBuild { - log.Fatalf("Target %s hasn't built but we have no pending tasks left.\n%s", label, unbuiltTargetsMessage(state.Graph)) + log.Fatalf("Target %s hasn't built but we have no pending tasks left.\n%s", label, unbuiltDepsMessage(state.Graph, target)) } } } @@ -643,20 +643,33 @@ func colouriseError(err error) error { // errorMessageRe is a regex to find lines that look like they're specifying a file. var errorMessageRe = deferredregex.DeferredRegex{Re: `^([^ ]+\.[^: /]+):([0-9]+):(?:([0-9]+):)? *(?:([a-z-_ ]+):)? (.*)$`} -// unbuiltTargetsMessage returns a message for any targets that are supposed to build but haven't yet. -func unbuiltTargetsMessage(graph *core.BuildGraph) string { - var msgBuilder strings.Builder - for _, target := range graph.AllTargets() { - if target.State() == core.Active { - _, _ = fmt.Fprintf(&msgBuilder, " %s", target.Label) - } else if target.State() == core.Pending { - _, _ = fmt.Fprintf(&msgBuilder, " %s (pending build)\n", target.Label) +// unbuiltDepsMessage returns a message describing why the given target hasn't built, by listing +// any of its transitive dependencies that aren't built either. +func unbuiltDepsMessage(graph *core.BuildGraph, target *core.BuildTarget) string { + var b strings.Builder + seen := map[*core.BuildTarget]bool{} + var walk func(*core.BuildTarget) + walk = func(t *core.BuildTarget) { + if seen[t] { + return + } + seen[t] = true + deps, unresolved := t.Dependencies(graph) + for _, l := range unresolved { + fmt.Fprintf(&b, " %s (not in the build graph)\n", l) + } + for _, dep := range deps { + if !dep.State().IsBuilt() { + fmt.Fprintf(&b, " %s (%s)\n", dep.Label, dep.State()) + walk(dep) + } } } - if msgBuilder.Len() == 0 { - return "\nThe following targets have not yet built:\n" + msgBuilder.String() + walk(target) + if b.Len() == 0 { + return "" } - return "" + return "\nThe following dependencies have not built:\n" + b.String() } // shortError returns the message for an error, shortening it if the error supports that.