engine: roll forward of #3362, re-using image builds from other resou…

…rces (#3553)

internal/engine/buildcontroller_test.go

@@ -1265,6 +1265,136 @@ func TestErrorHandlingWithMultipleBuilds(t *testing.T) {
 	f.assertAllBuildsConsumed()
 }
 
+func TestManifestsWithSameTwoImages(t *testing.T) {
+	f := newTestFixture(t)
+	m1, m2 := NewManifestsWithSameTwoImages(f)
+	f.Start([]model.Manifest{m1, m2})
+
+	f.waitForCompletedBuildCount(2)
+
+	call := f.nextCall("m1 build1")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	call = f.nextCall("m2 build1")
+	assert.Equal(t, m2.K8sTarget(), call.k8s())
+
+	aPath := f.JoinPath("common", "a.txt")
+	f.fsWatcher.Events <- watch.NewFileEvent(aPath)
+
+	f.waitForCompletedBuildCount(4)
+
+	// Make sure that both builds are triggered, and that they
+	// are triggered in a particular order.
+	call = f.nextCall("m1 build2")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	state := call.state[m1.ImageTargets[0].ID()]
+	assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet)
+
+	// Make sure that when the second build is triggered, we did the bookkeeping
+	// correctly around marking the first and second image built and only deploying
+	// the k8s resources.
+	call = f.nextCall("m2 build2")
+	assert.Equal(t, m2.K8sTarget(), call.k8s())
+
+	id := m2.ImageTargets[0].ID()
+	result := f.b.resultsByID[id]
+	assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id])
+
+	id = m2.ImageTargets[1].ID()
+	result = f.b.resultsByID[id]
+	assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id])
+
+	err := f.Stop()
+	assert.NoError(t, err)
+	f.assertAllBuildsConsumed()
+}
+
+func TestManifestsWithTwoCommonAncestors(t *testing.T) {
+	f := newTestFixture(t)
+	m1, m2 := NewManifestsWithTwoCommonAncestors(f)
+	f.Start([]model.Manifest{m1, m2})
+
+	f.waitForCompletedBuildCount(2)
+
+	call := f.nextCall("m1 build1")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	call = f.nextCall("m2 build1")
+	assert.Equal(t, m2.K8sTarget(), call.k8s())
+
+	aPath := f.JoinPath("base", "a.txt")
+	f.fsWatcher.Events <- watch.NewFileEvent(aPath)
+
+	f.waitForCompletedBuildCount(4)
+
+	// Make sure that both builds are triggered, and that they
+	// are triggered in a particular order.
+	call = f.nextCall("m1 build2")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	state := call.state[m1.ImageTargets[0].ID()]
+	assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet)
+
+	// Make sure that when the second build is triggered, we did the bookkeeping
+	// correctly around marking the first and second image built, and only
+	// rebuilding the third image and k8s deploy.
+	call = f.nextCall("m2 build2")
+	assert.Equal(t, m2.K8sTarget(), call.k8s())
+
+	id := m2.ImageTargets[0].ID()
+	result := f.b.resultsByID[id]
+	assert.Equal(t,
+		store.NewBuildState(result, nil, nil),
+		call.state[id])
+
+	id = m2.ImageTargets[1].ID()
+	result = f.b.resultsByID[id]
+	assert.Equal(t,
+		store.NewBuildState(result, nil, nil),
+		call.state[id])
+
+	id = m2.ImageTargets[2].ID()
+	result = f.b.resultsByID[id]
+
+	// Assert the 3rd image was not reused from the previous build.
+	assert.NotEqual(t, result, call.state[id].LastResult)
+	assert.Equal(t,
+		map[model.TargetID]bool{m2.ImageTargets[1].ID(): true},
+		call.state[id].DepsChangedSet)
+
+	err := f.Stop()
+	assert.NoError(t, err)
+	f.assertAllBuildsConsumed()
+}
+
+func TestManifestsWithCommonAncestorAndTrigger(t *testing.T) {
+	f := newTestFixture(t)
+	m1, m2 := NewManifestsWithCommonAncestor(f)
+	f.Start([]model.Manifest{m1, m2})
+
+	f.waitForCompletedBuildCount(2)
+
+	call := f.nextCall("m1 build1")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	call = f.nextCall("m2 build1")
+	assert.Equal(t, m2.K8sTarget(), call.k8s())
+
+	f.store.Dispatch(server.AppendToTriggerQueueAction{Name: m1.Name})
+	f.waitForCompletedBuildCount(3)
+
+	// Make sure that only one build was triggered.
+	call = f.nextCall("m1 build2")
+	assert.Equal(t, m1.K8sTarget(), call.k8s())
+
+	f.assertNoCall("m2 should not be rebuilt")
+
+	err := f.Stop()
+	assert.NoError(t, err)
+	f.assertAllBuildsConsumed()
+}
+
 func (f *testFixture) waitUntilManifestBuilding(name model.ManifestName) {
 	msg := fmt.Sprintf("manifest %q is building", name)
 	f.WaitUntilManifestState(msg, name, func(ms store.ManifestState) bool {

internal/engine/upper.go

@@ -234,22 +234,91 @@ func handleBuildResults(engineState *store.EngineState,
 	isBuildSuccess := br.Error == nil
 
 	ms := mt.State
+	mn := mt.Manifest.Name
 	for id, result := range results {
 		ms.MutableBuildStatus(id).LastResult = result
 	}
 
 	// Remove pending file changes that were consumed by this build.
 	for _, status := range ms.BuildStatuses {
-		for file, modTime := range status.PendingFileChanges {
-			if store.BeforeOrEqual(modTime, br.StartTime) {
-				delete(status.PendingFileChanges, file)
-			}
-		}
+		status.ClearPendingChangesBefore(br.StartTime)
 	}
 
 	if isBuildSuccess {
 		ms.LastSuccessfulDeployTime = br.FinishTime
 	}
+
+	// Update build statuses for duplicated image targets in other manifests.
+	// This ensures that those images targets aren't redundantly rebuilt.
+	for _, currentMT := range engineState.TargetsBesides(mn) {
+		// We only want to update image targets for Manifests that are already queued
+		// for rebuild and not currently building. This has two benefits:
+		//
+		// 1) If there's a bug in Tilt's image caching (e.g.,
+		//    https://github.com/tilt-dev/tilt/pull/3542), this prevents infinite
+		//    builds.
+		//
+		// 2) If the current manifest build was kicked off by a trigger, we don't
+		//    want to queue manifests with the same image.
+		if currentMT.NextBuildReason() == model.BuildReasonNone ||
+			engineState.IsCurrentlyBuilding(currentMT.Manifest.Name) {
+			continue
+		}
+
+		currentMS := currentMT.State
+		idSet := currentMT.Manifest.TargetIDSet()
+		updatedIDSet := make(map[model.TargetID]bool)
+
+		for id, result := range results {
+			_, ok := idSet[id]
+			if !ok {
+				continue
+			}
+
+			// We can only reuse image update, not live-updates or other kinds of
+			// deploys.
+			_, isImageResult := result.(store.ImageBuildResult)
+			if !isImageResult {
+				continue
+			}
+
+			currentStatus := currentMS.MutableBuildStatus(id)
+			currentStatus.LastResult = result
+			currentStatus.ClearPendingChangesBefore(br.StartTime)
+			updatedIDSet[id] = true
+		}
+
+		if len(updatedIDSet) == 0 {
+			continue
+		}
+
+		// Suppose we built manifestA, which contains imageA depending on imageCommon.
+		//
+		// We also have manifestB, which contains imageB depending on imageCommon.
+		//
+		// We need to mark imageB as dirty, because it was not built in the manifestA
+		// build but its dependency was built.
+		//
+		// Note that this logic also applies to deploy targets depending on image
+		// targets. If we built manifestA, which contains imageX and deploy target
+		// k8sA, and manifestB contains imageX and deploy target k8sB, we need to mark
+		// target k8sB as dirty so that Tilt actually deploys the changes to imageX.
+		rDepsMap := currentMT.Manifest.ReverseDependencyIDs()
+		for updatedID := range updatedIDSet {
+
+			// Go through each target depending on an image we just built.
+			for _, rDepID := range rDepsMap[updatedID] {
+
+				// If that target was also built, it's up-to-date.
+				if updatedIDSet[rDepID] {
+					continue
+				}
+
+				// Otherwise, we need to mark it for rebuild to pick up the new image.
+				currentMS.MutableBuildStatus(rDepID).PendingDependencyChanges[updatedID] = br.StartTime
+			}
+		}
+	}
 }
 
 func handleBuildCompleted(ctx context.Context, engineState *store.EngineState, cb buildcontrol.BuildCompleteAction) {

internal/engine/upper_test.go

@@ -2827,8 +2827,10 @@ func TestWatchManifestsWithCommonAncestor(t *testing.T) {
 	call = f.nextCall("m2 build1")
 	assert.Equal(t, m2.K8sTarget(), call.k8s())
 
-	f.WriteFile("common/a.txt", "hello world")
-	f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("common/a.txt"))
+	f.WriteFile(filepath.Join("common", "a.txt"), "hello world")
+
+	aPath := f.JoinPath("common", "a.txt")
+	f.fsWatcher.Events <- watch.NewFileEvent(aPath)
 
 	f.waitForCompletedBuildCount(4)
 
@@ -2837,9 +2839,30 @@ func TestWatchManifestsWithCommonAncestor(t *testing.T) {
 	call = f.nextCall("m1 build2")
 	assert.Equal(t, m1.K8sTarget(), call.k8s())
 
+	state := call.state[m1.ImageTargets[0].ID()]
+	assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet)
+
+	// Make sure that when the second build is triggered, we did the bookkeeping
+	// correctly around reusing the image and propagating DepsChanged when
+	// we deploy the second k8s target.
 	call = f.nextCall("m2 build2")
 	assert.Equal(t, m2.K8sTarget(), call.k8s())
 
+	id := m2.ImageTargets[0].ID()
+	result := f.b.resultsByID[id]
+	assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id])
+
+	id = m2.ImageTargets[1].ID()
+	result = f.b.resultsByID[id]
+
+	// Assert the 2nd image was not re-used from the previous result.
+	assert.NotEqual(t, result, call.state[id].LastResult)
+	assert.Equal(t, map[model.TargetID]bool{m2.ImageTargets[0].ID(): true},
+		call.state[id].DepsChangedSet)
+
+	err := f.Stop()
+	assert.NoError(t, err)
+	f.assertAllBuildsConsumed()
 }
 
 func TestConfigChangeThatChangesManifestIsIncludedInManifestsChangedFile(t *testing.T) {

internal/store/engine_state.go

@@ -169,7 +169,6 @@ func (e *EngineState) BuildStatus(id model.TargetID) BuildStatus {
 		}
 	}
 	return BuildStatus{}
-
 }
 
 func (e *EngineState) AvailableBuildSlots() int {
@@ -246,6 +245,19 @@ func (e EngineState) Targets() []*ManifestTarget {
 	return result
 }
 
+func (e EngineState) TargetsBesides(mn model.ManifestName) []*ManifestTarget {
+	targets := e.Targets()
+	result := make([]*ManifestTarget, 0, len(targets))
+	for _, mt := range targets {
+		if mt.Manifest.Name == mn {
+			continue
+		}
+
+		result = append(result, mt)
+	}
+	return result
+}
+
 func (e *EngineState) ManifestInTriggerQueue(mn model.ManifestName) bool {
 	for _, queued := range e.TriggerQueue {
 		if queued == mn {
@@ -340,6 +352,19 @@ func (s BuildStatus) IsEmpty() bool {
 		s.LastResult == nil
 }
 
+func (s *BuildStatus) ClearPendingChangesBefore(startTime time.Time) {
+	for file, modTime := range s.PendingFileChanges {
+		if BeforeOrEqual(modTime, startTime) {
+			delete(s.PendingFileChanges, file)
+		}
+	}
+	for file, modTime := range s.PendingDependencyChanges {
+		if BeforeOrEqual(modTime, startTime) {
+			delete(s.PendingDependencyChanges, file)
+		}
+	}
+}
+
 type ManifestState struct {
 	Name model.ManifestName
 

internal/store/engine_state_test.go

@@ -154,11 +154,18 @@ func TestNextBuildReason(t *testing.T) {
 
 	iTargetID := model.ImageID(container.MustParseSelector("sancho"))
 	status := mt.State.MutableBuildStatus(kTarget.ID())
+	assert.Equal(t, "Initial Build",
+		mt.NextBuildReason().String())
+
 	status.PendingDependencyChanges[iTargetID] = time.Now()
-	assert.Equal(t, "Initial Build | Dependency Updated",
+	assert.Equal(t, "Initial Build",
+		mt.NextBuildReason().String())
+
+	mt.State.AddCompletedBuild(model.BuildRecord{StartTime: time.Now(), FinishTime: time.Now()})
+	assert.Equal(t, "Dependency Updated",
 		mt.NextBuildReason().String())
 
 	status.PendingFileChanges["a.txt"] = time.Now()
-	assert.Equal(t, "Initial Build | Changed Files | Dependency Updated",
+	assert.Equal(t, "Changed Files | Dependency Updated",
 		mt.NextBuildReason().String())
 }

pkg/model/build_reason.go

@@ -93,6 +93,11 @@ func (r BuildReason) String() string {
 		}
 	}
 
+	// The Init build reason should be listed alone too.
+	if r.Has(BuildReasonFlagInit) {
+		return translations[BuildReasonFlagInit]
+	}
+
 	// Use an array to iterate over the translations to ensure the iteration order
 	// is consistent.
 	for _, v := range allBuildReasons {

pkg/model/manifest.go

@@ -63,6 +63,22 @@ func (m Manifest) DependencyIDs() []TargetID {
 	return result
 }
 
+// A map from each target id to the target IDs that depend on it.
+func (m Manifest) ReverseDependencyIDs() map[TargetID][]TargetID {
+	result := make(map[TargetID][]TargetID)
+	for _, iTarget := range m.ImageTargets {
+		for _, depID := range iTarget.DependencyIDs() {
+			result[depID] = append(result[depID], iTarget.ID())
+		}
+	}
+	if !m.deployTarget.ID().Empty() {
+		for _, depID := range m.deployTarget.DependencyIDs() {
+			result[depID] = append(result[depID], m.deployTarget.ID())
+		}
+	}
+	return result
+}
+
 func (m Manifest) WithImageTarget(iTarget ImageTarget) Manifest {
 	m.ImageTargets = []ImageTarget{iTarget}
 	return m