Fix build cache restore (#329)

* restructure file deletion * extract files which are not part of invalidFiles --------- Co-authored-by: equanox <[email protected]> Co-authored-by: Matthias Nösner <[email protected]>
benchkram · Apr 20, 2023 · 867c65f · 867c65f
1 parent 40dafaf
commit 867c65f
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 39 deletions.
diff --git a/bob/playbook/build_internal.go b/bob/playbook/build_internal.go
@@ -53,7 +53,7 @@ func (p *Playbook) build(ctx context.Context, task *bobtask.Task) (pt *processed
 	errz.Fatal(err)
 	boblog.Log.V(2).Info(fmt.Sprintf("TaskNeedsRebuild [rebuildRequired: %t] [cause:%s]", rebuild.IsRequired, rebuild.Cause))
 
-	// task might need a rebuild due to an input change.
+	// Task might need a rebuild due to an input change.
 	// Could still be possible to load the targets from the artifact store.
 	// If a task needs a rebuild due to a dependency change => rebuild.
 	if rebuild.IsRequired {
@@ -111,7 +111,7 @@ func (p *Playbook) build(ctx context.Context, task *bobtask.Task) (pt *processed
 		return pt, p.TaskNoRebuildRequired(task.TaskID)
 	}
 
-	err = task.Clean(rebuild.VerifyResult.InvalidFiles)
+	err = task.CleanTargetsWithReason(rebuild.VerifyResult.InvalidFiles)
 	errz.Fatal(err)
 
 	err = task.Run(ctx, p.namePad)

diff --git a/bob/playbook/playbook.go b/bob/playbook/playbook.go
@@ -114,7 +114,7 @@ func (rc *RebuildCause) String() string {
 }
 
 const (
-	InputNotFoundInBuildInfo RebuildCause = "input-not-in-build-info"
+	InputNotFoundInBuildInfo RebuildCause = "input-not-in-build-info" // aka local cache miss
 	TaskForcedRebuild        RebuildCause = "forced"
 	DependencyChanged        RebuildCause = "dependency-changed"
 	TargetInvalid            RebuildCause = "target-invalid"

diff --git a/bob/playbook/rebuild.go b/bob/playbook/rebuild.go
@@ -42,7 +42,7 @@ func (p *Playbook) TaskNeedsRebuild(taskID int) (rebuildInfo RebuildInfo, err er
 			IsRequired: true,
 			Cause:      TaskForcedRebuild,
 			VerifyResult: target.VerifyResult{
-				TargetIsValid: len(invalidFiles) > 0,
+				TargetIsValid: len(invalidFiles) == 0,
 				InvalidFiles:  invalidFiles,
 			},
 		}, nil
@@ -81,7 +81,7 @@ func (p *Playbook) TaskNeedsRebuild(taskID int) (rebuildInfo RebuildInfo, err er
 		}
 
 		return RebuildInfo{IsRequired: true, Cause: InputNotFoundInBuildInfo, VerifyResult: target.VerifyResult{
-			TargetIsValid: len(invalidFiles) > 0,
+			TargetIsValid: len(invalidFiles) == 0,
 			InvalidFiles:  invalidFiles,
 		}}, nil
 	}

diff --git a/bobtask/artifact_extract.go b/bobtask/artifact_extract.go
@@ -35,8 +35,8 @@ func (t *Task) ArtifactExtract(artifactName hash.In, invalidFiles map[string][]t
 	}
 	defer artifact.Close()
 
-	// Assure tasks is cleaned up before extracting
-	err = t.Clean(invalidFiles)
+	// Assure task is cleaned up before extracting
+	err = t.CleanTargetsWithReason(invalidFiles)
 	errz.Fatal(err)
 
 	archiveReader := newArchiveReader()
@@ -135,8 +135,9 @@ func (t *Task) ArtifactExtract(artifactName hash.In, invalidFiles map[string][]t
 // shouldFetchFromCache checks if a file should be brought back from cache inside the target
 // A file will be brought back from cache if it's missing or was changed
 func shouldFetchFromCache(filename string, invalidFiles map[string][]target.Reason) bool {
+	// FIXME: accessing a hashmap twice is inefficent.
 	if _, ok := invalidFiles[filename]; !ok {
-		return false
+		return true
 	}
 	for _, reason := range invalidFiles[filename] {
 		if reason == target.ReasonSizeChanged || reason == target.ReasonHashChanged || reason == target.ReasonMissing {

diff --git a/bobtask/clean.go b/bobtask/clean.go
@@ -9,10 +9,10 @@ import (
 	"github.com/logrusorgru/aurora"
 )
 
-// Clean the targets defined by this task.
-// This assures that we can be sure a target was correctly created
+// CleanTargetsWithReason removes files assosiated to a target but considering
+// the reason for a deletion. This assures that we can be sure a target was correctly created
 // and has not been there before the task ran.
-func (t *Task) Clean(invalidFiles map[string][]target.Reason, verbose ...bool) error {
+func (t *Task) CleanTargetsWithReason(invalidFiles map[string][]target.Reason, verbose ...bool) error {
 	var vb bool
 	if len(verbose) > 0 {
 		vb = verbose[0]
@@ -62,3 +62,52 @@ func (t *Task) Clean(invalidFiles map[string][]target.Reason, verbose ...bool) e
 
 	return nil
 }
+
+// Clean all filesystem targets as given in the Bobfile
+func (t *Task) Clean(verbose ...bool) error {
+	var vb bool
+	if len(verbose) > 0 {
+		vb = verbose[0]
+	}
+
+	if t.target != nil {
+		boblog.Log.V(5).Info(fmt.Sprintf("Cleaning targets for task %s", t.Name()))
+
+		homeDir, err := os.UserHomeDir()
+		if err != nil {
+			return err
+		}
+
+		if vb {
+			fmt.Printf("Cleaning targets of task %s \n", t.name)
+		}
+
+		if t.dir == "" {
+			return fmt.Errorf("task dir not set")
+		}
+
+		files := t.target.FilesystemEntriesRaw()
+		for _, filename := range files {
+			if vb {
+				fmt.Printf(" %s ", filename)
+			}
+
+			if filename == "/" || filename == homeDir {
+				return fmt.Errorf("Cleanup of %s is not allowed", filename)
+			}
+
+			err := os.RemoveAll(filename)
+			if err != nil {
+				if vb {
+					fmt.Printf("%s\n", aurora.Red("failed"))
+				}
+				return err
+			}
+			if vb {
+				fmt.Printf("%s\n", aurora.Green("done"))
+			}
+		}
+	}
+
+	return nil
+}
diff --git a/bobtask/target/target.go b/bobtask/target/target.go
@@ -15,7 +15,6 @@ type Target interface {
 	Resolve() error
 
 	FilesystemEntries() []string
-	FilesystemEntriesPlain() []string
 	FilesystemEntriesRaw() []string
 	FilesystemEntriesRawPlain() []string
 
@@ -73,19 +72,14 @@ func New(opts ...Option) *T {
 
 // FilesystemEntries in relation to the umrella bobfile
 func (t *T) FilesystemEntries() []string {
-
 	if len(*t.filesystemEntries) == 0 {
 		return []string{}
 	}
-
-	var pathsWithDir []string
-	for _, v := range *t.filesystemEntries {
-		pathsWithDir = append(pathsWithDir, filepath.Join(t.dir, v))
-	}
-
-	return pathsWithDir
+	return *t.filesystemEntries
 }
 
+// FilesystemEntriesRaw returns the filesystem entries
+// relative to the umbrella bobfile.
 func (t *T) FilesystemEntriesRaw() []string {
 	var pathsWithDir []string
 	for _, v := range t.filesystemEntriesRaw {
@@ -95,12 +89,6 @@ func (t *T) FilesystemEntriesRaw() []string {
 	return pathsWithDir
 }
 
-// FilesystemEntriesPlain does return the pure path
-// as given in the bobfile.
-func (t *T) FilesystemEntriesPlain() []string {
-	return append([]string{}, *t.filesystemEntries...)
-}
-
 func (t *T) FilesystemEntriesRawPlain() []string {
 	return append([]string{}, t.filesystemEntriesRaw...)
 }
@@ -121,7 +109,7 @@ func (t *T) DockerImages() []string {
 func (t *T) AsInvalidFiles(reason Reason) map[string][]Reason {
 	invalidFiles := make(map[string][]Reason)
 
-	for _, v := range t.FilesystemEntriesRaw() {
+	for _, v := range t.FilesystemEntries() {
 		invalidFiles[v] = []Reason{reason}
 	}
 	return invalidFiles

diff --git a/cli/cmd_clean.go b/cli/cmd_clean.go
@@ -5,7 +5,6 @@ import (
 	"os"
 
 	"github.com/benchkram/bob/bob"
-	"github.com/benchkram/bob/bobtask/target"
 	"github.com/benchkram/bob/pkg/boblog"
 	"github.com/benchkram/bob/pkg/usererror"
 	"github.com/benchkram/errz"
@@ -82,17 +81,7 @@ func runCleanTargets() {
 			continue
 		}
 
-		taskTarget, err := t.Target()
+		err = t.Clean(true)
 		errz.Fatal(err)
-
-		invalidFiles := make(map[string][]target.Reason)
-		for _, v := range taskTarget.FilesystemEntriesRawPlain() {
-			invalidFiles[v] = append(invalidFiles[v], target.ReasonCreatedAfterBuild)
-		}
-
-		err = t.Clean(invalidFiles, true)
-		if err != nil {
-			boblog.Log.Error(err, "Unable to clean target")
-		}
 	}
 }