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/build/build_step_stress_test.go b/src/build/build_step_stress_test.go index 6a4830e802..045db96d91 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,14 +47,13 @@ func TestBuildLotsOfTargets(t *testing.T) { } }() - plz.RunHost(nil, state) + plz.RunHost(targets, state) } 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 { @@ -68,9 +67,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 @@ -101,12 +97,8 @@ 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, 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 +117,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..ba0a4a36c4 100644 --- a/src/build/build_step_test.go +++ b/src/build/build_step_test.go @@ -609,12 +609,8 @@ 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, 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 +628,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/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/cmap/cerrmap.go b/src/cmap/cerrmap.go index 78c7dc8ed6..bccde4a9a7 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() @@ -32,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) { @@ -60,7 +57,7 @@ func (m *ErrMap[K, V]) Get(key K) (V, error) { // 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 { @@ -79,6 +76,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/cmap/cmap.go b/src/cmap/cmap.go index f8058ef732..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) { @@ -67,21 +61,15 @@ 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. 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 { 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. @@ -138,32 +126,19 @@ 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 { + 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. // 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) } }) } diff --git a/src/core/build_target.go b/src/core/build_target.go index 675c98b6ee..2c5b13125b 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -205,14 +205,13 @@ 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 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"` @@ -252,6 +251,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 @@ -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,9 +395,8 @@ func (s BuildTargetState) IsBuilt() bool { func NewBuildTarget(label BuildLabel) *BuildTarget { return &BuildTarget{ Label: label, - state: int32(Inactive), + state: Inactive, BuildingDescription: DefaultBuildingDescription, - finishedBuilding: make(chan struct{}), } } @@ -586,7 +577,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 { @@ -594,20 +589,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 { @@ -615,19 +620,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). @@ -693,6 +707,23 @@ func (target *BuildTarget) RuntimeDependencies() iter.Seq[BuildLabel] { } } +// 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() + for _, deps := range target.dependencies { + if deps.Runtime || deps.Data { + if !yield(deps.Label) { + break + } + } + } + } +} + // IterAllRuntimeDependencies returns an iterator over the transitive run-time dependencies of this target. // Require/provide relationships between pairs of targets are resolved as they are with build-time dependencies. func (target *BuildTarget) IterAllRuntimeDependencies(graph *BuildGraph) iter.Seq[BuildLabel] { @@ -777,16 +808,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 { @@ -1003,14 +1024,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) + } } } } @@ -1161,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/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/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.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/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/graph.go b/src/core/graph.go index 9aa32abdbb..c30824c9f6 100644 --- a/src/core/graph.go +++ b/src/core/graph.go @@ -6,6 +6,7 @@ package core import ( "context" + "fmt" "maps" "slices" "sort" @@ -31,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 @@ -65,33 +66,11 @@ 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 } -// 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 { @@ -100,25 +79,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. @@ -172,12 +140,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 } @@ -185,7 +154,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/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/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/core/state.go b/src/core/state.go index 122fe3bade..700f07ccc1 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" @@ -75,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. @@ -217,13 +218,15 @@ 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 func(BuildLabel) (*BuildTarget, error) - // Test runs tests on a single target - Test func(BuildLabel) error + // 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 + // 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. + // 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() // initOnce is used to control loading the subrepo .plzconfig initOnce *sync.Once @@ -323,6 +326,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) @@ -460,6 +468,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() { @@ -526,7 +537,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() } } @@ -653,19 +664,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) @@ -732,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/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/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/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. diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 589bbf89c6..fb7359b38b 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,8 +381,8 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget { Subrepo: subrepoLabel.Subrepo, Name: "all", } - if _, err := s.state.Parse(subrepoPackageLabel); err != nil { - s.Error("Failed to parse subrepo target: %v", err) + if _, err := s.state.Parse(subrepoPackageLabel, pkgLabel); err != nil { + 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 @@ -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)) } @@ -1219,6 +1224,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 +1259,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/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)) 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/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..8bef84d787 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,15 +89,10 @@ 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)) - 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 dd5749b2b5..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 @@ -59,7 +36,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 { @@ -80,10 +57,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 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 796d3bcee8..b1f0195ee6 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -2,9 +2,11 @@ package plz import ( "context" + "errors" "fmt" "iter" "path/filepath" + "slices" "strings" "sync" "sync/atomic" @@ -20,6 +22,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" ) @@ -40,302 +43,494 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress go state.UpdateResources() } - parse.InitParser(state) + parser := parse.InitParser(state) - localLimiter := make(limiter, state.Config.Please.NumThreads) - remoteLimiter := make(limiter, state.Config.NumRemoteExecutors()) - anyRemote := state.Config.NumRemoteExecutors() > 0 - - limiter := func(remote bool) limiter { - if remote { - return remoteLimiter + // 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) } - return localLimiter + state.CloseResults() + metrics.Push(state.Config.Metrics, state.Config.IsRemoteExecution()) + }() + + ctx, cancel := context.WithCancel(context.Background()) + g, ctx := group(state, ctx) + state.Cancel = cancel + + 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), + 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, } - // 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, dependent core.BuildLabel) (*core.BuildTarget, error) { + return r.Build(ctx, label, dependent) + } + state.Parse = func(label, dependent core.BuildLabel) (*core.Package, error) { + return r.Parse(ctx, label, dependent) + } - 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) + // Register the preloaded targets with the parser + if err := r.RegisterPreloads(ctx, state, parser); err != nil { + return err } - // 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 + 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"), core.OriginalTarget); err != nil { + return err } - pkg, err := state.Parse(label) - if err != nil { - return nil, 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 } - if target := pkg.Target(label.Name); target != nil { - return target, nil + // Reset the group & context for next time + ctx, cancel := context.WithCancel(context.Background()) + g, ctx = group(state, ctx) + state.Cancel = cancel + 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)) } - return nil, fmt.Errorf("Parsed build file %s but it doesn't contain target %s%s", pkg.Filename, label.Name, pkg.SuggestTargets(label, label)) + g.Go(func() error { + return r.queueTargetsForDebug(ctx, targets[0]) + }) } - // 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 + return g.Wait() +} + +// 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] + parseOnce *cmap.Map[core.BuildLabel, struct{}] + localLimiter limiter + remoteLimiter limiter + anyRemote bool +} + +// Parse parses for a target. It can be called more than once for the same build label. +// 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) + 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 pkg, 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 - } - } +// 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) +} - 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() { +// 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 { + 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, dependent core.BuildLabel) error { + if !label.IsAllTargets() { + return r.recursiveParse(ctx, label, dependent) + } + pkg, err := r.Parse(ctx, label, dependent) + if err != nil { + return err + } + g, gctx := group(r.state, ctx) + for _, target := range pkg.AllTargets() { + for dep := range target.DeclaredDependencies() { g.Go(func() error { - return buildDep(dep, target) + // N.B. No need to deduplicate these; recursiveParse does that for the whole walk. + return r.recursiveParse(gctx, dep, target.Label) }) } - for _, src := range target.AllSources() { - if l, ok := src.Label(); ok { - g.Go(func() error { - return buildDep(l, target) - }) - } - } - if err := g.Wait(); err != nil { - return err - } + } + return g.Wait() +} - // TODO(peter): Need to handle targets getting extra deps added by post-build functions here. +// 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, gctx := group(r.state, ctx) + for dep := range target.DeclaredDependencies() { + g.Go(func() error { + return r.recursiveParse(gctx, dep, target.Label) + }) + } + return g.Wait() +} - // 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, dependent core.BuildLabel) (*core.BuildTarget, error) { + if target := r.state.Graph.Target(label); target != nil { + return target, nil } + 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, dependent)) + r.state.LogBuildError(label, core.ParseFailed, err, "%s", err) + return nil, err +} - 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, dependent.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) + // TODO(peter): Would parallelism here be useful? + for _, p := range provided { + if !yield(r.parseTarget(ctx, p, dependent.Label)) { + break } - progress.numTotal.Add(1) - defer progress.numDone.Add(1) - target, err := parseTarget(label) - if err != nil { - return nil, err - } - 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, target.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, gctx := group(r.state, ctx) + for dep := range target.BuildDependencyLabels() { + g.Go(func() error { + return r.buildDep(gctx, 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(gctx, l, target) + }) + } + } + if err := g.Wait(); err != nil { + return err + } + + if target.ModifiedByCallback { + // A pre- or post-build function modified this target post parse, so we need to check its dependencies again. + g, gctx := group(r.state, ctx) + for dep := range target.BuildDependencyLabels() { + g.Go(func() error { + return r.buildDep(gctx, dep, 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 + } + + // Okay, now the runtime dependencies can happen in parallel with the target itself. + // N.B. Even when there are none we can't just build the target and return; its own callbacks + // can add some, which we won't know about until it's built. + if deps := slices.Collect(target.RuntimeAndDataDependencies()); len(deps) == 0 { + if err := r.buildJustOne(target); err != nil { + return err } - 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) + } else { + g, gctx = group(r.state, ctx) + g.Go(func() error { + return r.buildJustOne(target) + }) + for _, dep := range deps { + g.Go(func() error { + return r.buildDep(gctx, dep, target) }) } - wg.Wait() + 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 = group(r.state, ctx) + for dep := range target.RuntimeAndDataDependencies() { + g.Go(func() error { + return r.buildDep(gctx, dep, target) + }) + } + return g.Wait() +} - // Register the preloaded targets with the parser - if err := registerPreloads(state); err != nil { +// 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() + 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, dependent core.BuildLabel) error { + pkg, err := r.Parse(ctx, label, dependent) + if err != nil { return err } + g, gctx := group(r.state, ctx) + for _, target := range pkg.AllTargets() { + if r.state.ShouldInclude(target) { + 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, dependent) + return err + }) + } + } + 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, 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, dependent) + }) } - if err := tf.FindOriginalTasks(preTargets, targets); err != nil { - return err + // 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, dependent) + 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) + return target, r.buildOne(ctx, target) + }) +} + +// testOne tests one single target +func (r *runner) testOne(ctx context.Context, target *core.BuildTarget, dependent core.BuildLabel) error { + if target.IsTest() { + r.progress.numTotal.Add(int64(r.state.NumTestRuns)) } - if err := g.Wait(); err != nil { + if _, err := r.Build(ctx, target.Label, dependent); err != nil { return err } - if state.Cache != nil { - state.Cache.Shutdown() + 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 + 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 -} - -// findOriginalTasks finds the original parse tasks for the original set of targets. -func (tf *taskFinder) FindOriginalTasks(preTargets, targets []core.BuildLabel) error { - log.Debug("Original target scan beginning...") - if tf.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 { +// Test is the main entrypoint to run tests for a label +func (r *runner) Test(ctx context.Context, label, dependent core.BuildLabel) error { + if !label.IsAllTargets() { + target, err := r.parseTarget(ctx, label, dependent) + if err != nil { return err } + return r.testOne(ctx, target, dependent) } - if tf.arch.Arch != "" && tf.arch != cli.HostArch() { - // Set up a new subrepo for this architecture. - tf.state.Graph.AddSubrepo(core.SubrepoForArch(tf.state, tf.arch)) - } - if len(preTargets) > 0 { - tf.findOriginalTaskSet(preTargets, false, true) - if err := tf.tasks.Wait(); err != nil { - return err - } - tf.tasks = &errgroup.Group{} + pkg, err := r.Parse(ctx, label, dependent) + if err != nil { + return err } - tf.findOriginalTaskSet(targets, tf.state.NeedTests, tf.state.NeedBuild) - if tf.state.NeedDebugDeps { - if len(targets) != 1 { - return fmt.Errorf("expected exactly 1 target in debug mode; got %d", len(targets)) + g, ctx := group(r.state, ctx) + for _, target := range pkg.AllTargets() { + if r.state.ShouldInclude(target) { + g.Go(func() error { + return r.testOne(ctx, target, dependent) + }) } - tf.tasks.Go(func() error { - return tf.queueTargetsForDebug(targets[0]) - }) } - if err := tf.tasks.Wait(); err != nil { - return err + 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 { + return r.remoteLimiter } - log.Debug("Original target scan complete") - return nil + return r.localLimiter } -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, core.OriginalTarget); 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 +556,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,64 +570,69 @@ 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, core.OriginalTarget); 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, core.OriginalTarget) 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 { +func (r *runner) queueTask(ctx context.Context, target core.BuildLabel, needTest, needBuild bool) { + r.state.AddOriginalTarget(target) + r.tasks.Go(func() error { if needTest { - return tf.state.Test(target) + return r.Test(ctx, target, core.OriginalTarget) } else if needBuild { - _, err := tf.state.Build(target) + _, err := r.Build(ctx, target, core.OriginalTarget) // TODO(peter): Ensure this gets downloaded if needed return err } - _, err := tf.state.Parse(target) - return err + return r.RecursiveParse(ctx, target, core.OriginalTarget) }) } -// 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 - for _, inc := range state.GetPreloadedSubincludes() { +func (r *runner) RegisterPreloads(ctx context.Context, state *core.BuildState, parser *asp.Parser) error { + g, ctx := group(r.state, ctx) + preloads := state.GetPreloadedSubincludes() + for _, inc := range preloads { 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, core.OriginalTarget); 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 eg.Wait() + if err := g.Wait(); err != nil { + return err + } + parser.RegisterPreloads(preloads) + return nil } // FindAllBuildFiles finds all BUILD files under a particular path. @@ -538,3 +738,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 +} 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/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)) diff --git a/src/test/coverage.go b/src/test/coverage.go index b52870f0d9..1386b094fc 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 } @@ -74,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/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) } 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) diff --git a/third_party/python/BUILD b/third_party/python/BUILD index 2e85c368d6..2d16ec3fc0 100644 --- a/third_party/python/BUILD +++ b/third_party/python/BUILD @@ -65,12 +65,13 @@ python_wheel( python_wheel( name = "absl", package_name = "absl_py", - hashes = ["c106f6ef0ae86c1273b0858b40ee15b99fad1c223838387b9d11446a033bbcb1"], - version = "0.9.0", + hashes = ["0f17b89f2a4eaaedc4f28c622998aa690564b3012a396a4ffad0821007fe03ba"], + version = "2.5.0", deps = [":six"], ) pip_library( name = "progress", version = "1.5", + licences = ["ISC"], ) 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) } }