keep going flag (#2932)

In this PR we introduce a flag --keep_going which will allow us to continue as far as we can with tasks. This can be useful if we want to collect all build failures for reporting at once rather than just the first one encountered. --------- Co-authored-by: rgodden <[email protected]>
thought-machine · Oct 26, 2023 · 3a20640 · 3a20640
1 parent b744937
commit 3a20640
Show file tree

Hide file tree

Showing 11 changed files with 122 additions and 16 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,9 @@
+Version 17.4.0
+--------------
+    * added --keep_going which continues exhasting tasks from the queue even
+      on failures. This can be useful if you want to collect all possible errors
+      in a single please build.
+
 Version 17.3.1
 --------------
     * Fixed `plz query somepath` to not hang in the face of a graph cycle (#2893)

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-17.3.1
+17.4.0-beta.1
diff --git a/src/build/build_step.go b/src/build/build_step.go
@@ -70,6 +70,7 @@ func Build(state *core.BuildState, target *core.BuildTarget, remote bool) {
 			log.Errorf("Failed to remove outputs for %s: %s", target.Label, err)
 		}
 		target.SetState(core.Failed)
+		target.FinishBuild()
 		return
 	}
 	if remote {

diff --git a/src/core/build_target.go b/src/core/build_target.go
@@ -309,19 +309,20 @@ 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
-	Cached                                 // Target has been retrieved from the cache
-	Unchanged                              // Target has been built but hasn't changed since last build
-	Reused                                 // Outputs of previous build have been reused.
-	BuiltRemotely                          // Target has been built but outputs are not necessarily local.
-	ReusedRemotely                         // Outputs of previous remote action have been reused.
-	Failed                                 // Target failed for some reason
+	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
+	Cached                                   // Target has been retrieved from the cache
+	Unchanged                                // Target has been built but hasn't changed since last build
+	Reused                                   // Outputs of previous build have been reused.
+	BuiltRemotely                            // Target has been built but outputs are not necessarily local.
+	ReusedRemotely                           // Outputs of previous remote action have been reused.
+	DependencyFailed                         // At least one dependency of this target has failed.
+	Failed                                   // Target failed for some reason
 )
 
 // String implements the fmt.Stringer interface.
@@ -346,6 +347,8 @@ func (s BuildTargetState) String() string {
 		return "Unchanged"
 	} else if s == Reused {
 		return "Reused"
+	} else if s == DependencyFailed {
+		return "Dependency Failed"
 	} else if s == Failed {
 		return "Failed"
 	} else if s == BuiltRemotely {

diff --git a/src/core/state.go b/src/core/state.go
@@ -173,6 +173,8 @@ type BuildState struct {
 	Arch cli.Arch
 	// Aggregated coverage for this run
 	Coverage TestCoverage
+	// True if we want to keep going on build failures and not exit early on the first error encountered
+	KeepGoing bool
 	// True if we require rule hashes to be correctly verified (usually the case).
 	VerifyHashes bool
 	// True if tests should calculate coverage metrics
@@ -1141,6 +1143,13 @@ func (state *BuildState) queueTargetAsync(target *BuildTarget, forceBuild, build
 		if building {
 			for _, t := range target.Dependencies() {
 				t.WaitForBuild()
+				if t.State() >= DependencyFailed { // Either the target failed or its dependencies failed
+					// Give up and set the original target as dependency failed
+					target.SetState(DependencyFailed)
+					state.LogBuildResult(target, TargetBuilt, "Dependency failed")
+					target.FinishBuild()
+					return
+				}
 			}
 		}
 		if !called.Value() {

diff --git a/src/output/targets.go b/src/output/targets.go
@@ -80,7 +80,9 @@ func (bt *buildingTargets) handleOutput(result *core.BuildResult) {
 		if result.Status != core.TargetTestFailed {
 			// Reset colour so the entire compiler error output doesn't appear red.
 			log.Errorf("%s failed:\x1b[0m\n%s", label, shortError(result.Err))
-			bt.state.Stop()
+			if !bt.state.KeepGoing {
+				bt.state.Stop()
+			}
 		} else if msg := shortError(result.Err); msg != "" {
 			log.Errorf("%s failed: %s", result.Label, msg)
 		} else {

diff --git a/src/please.go b/src/please.go
@@ -87,6 +87,7 @@ var opts struct {
 		KeepWorkdirs       bool    `long:"keep_workdirs" description:"Don't clean directories in plz-out/tmp after successfully building targets."`
 		HTTPProxy          cli.URL `long:"http_proxy" env:"HTTP_PROXY" description:"HTTP proxy to use for downloads"`
 		Debug              bool    `long:"debug" description:"When enabled, Please will enter into an interactive debugger when breakpoint() is called during parsing."`
+		KeepGoing          bool    `long:"keep_going" description:"Continue as much as possible after an error. While the target that failed and those that depend on it cannot be build, other prerequisites of these targets can be."`
 	} `group:"Options that enable / disable certain behaviors"`
 
 	HelpFlags struct {
@@ -1084,6 +1085,7 @@ func Please(targets []core.BuildLabel, config *core.Configuration, shouldBuild,
 		config.Build.Config = "dbg"
 	}
 	state := core.NewBuildState(config)
+	state.KeepGoing = opts.BehaviorFlags.KeepGoing
 	state.VerifyHashes = !opts.BehaviorFlags.NoHashVerification
 	// Only one of these two can be passed
 	state.NumTestRuns = uint16(opts.Test.NumRuns)

diff --git a/test/build_defs/test.build_defs b/test/build_defs/test.build_defs
@@ -29,7 +29,7 @@ def please_repo_e2e_test(
         test_cmd += [f'_STR="$(cat {o})" _SUBSTR="{c}" && if [ "${_STR##*$_SUBSTR*}" ]; then echo "$_STR"; exit 1; fi' for o, c in expect_output_contains.items()]
 
     if expect_output_doesnt_contain:
-        test_cmd += [f'_STR="$(cat {o})" _SUBSTR="{c}" && if [ -z "${_STR##*$_SUBSTR*}" ]; then echo "$_STR"; exit 1; fi' for o, c in expect_output_contains.items()]
+        test_cmd += [f'_STR="$(cat {o})" _SUBSTR="{c}" && if [ -z "${_STR##*$_SUBSTR*}" ]; then echo "$_STR"; exit 1; fi' for o, c in expect_output_doesnt_contain.items()]
 
     test_cmd = ' && '.join(test_cmd)
 

diff --git a/test/keep_going/BUILD b/test/keep_going/BUILD
@@ -0,0 +1,27 @@
+subinclude("//test/build_defs")
+
+please_repo_e2e_test(
+    name = "plz_build_all",
+    expected_failure = True,
+    expected_output = {
+        "plz-out/gen/package/pass": "",
+        "plz-out/gen/package/dep_pass": "",
+        "plz-out/gen/package/fail_test_pass_dep": "",
+    },
+    plz_command = "plz build --keep_going //package:all",
+    repo = "test_repo",
+)
+
+please_repo_e2e_test(
+    name = "plz_test_all",
+    expect_output_contains = {
+        "plz-out/log/test_results.xml": "fail_test_pass_dep",
+    },
+    expect_output_doesnt_contain = {
+        "plz-out/log/test_results.xml": "fail_test_fail_dep",
+    },
+    expected_failure = True,
+    plz_command = "plz test --keep_going //package:all",
+    repo = "test_repo",
+)
+
diff --git a/test/keep_going/test_repo/.plzconfig b/test/keep_going/test_repo/.plzconfig
diff --git a/test/keep_going/test_repo/package/BUILD_FILE b/test/keep_going/test_repo/package/BUILD_FILE
@@ -0,0 +1,56 @@
+build_rule(
+    name = "fail",
+    cmd = "exit 1",
+    outs = ["fail"],
+)
+
+build_rule(
+    name = "fail2",
+    cmd = "exit 1",
+    outs = ["fail2"],
+)
+
+build_rule(
+    name = "pass",
+    cmd = "touch $OUTS",
+    outs = ["pass"],
+)
+
+build_rule(
+    name = "dep_pass",
+    deps = [":pass"],
+    cmd = "touch $OUTS",
+    outs = ["dep_pass"],
+)
+
+build_rule(
+    name = "dep_fail",
+    deps = [":fail"],
+    cmd = "touch $OUTS",
+    outs = ["dep_fail"],
+)
+
+build_rule(
+    name = "dep_dep_fail",
+    deps = [":dep_fail"],
+    cmd = "touch $OUTS",
+    outs = ["dep_dep_fail"],
+)
+
+build_rule(
+    test = True,
+    name = "fail_test_pass_dep",
+    cmd = "touch $OUTS",
+    test_cmd = "exit 1",
+    outs = ["fail_test_pass_dep"],
+    deps = [":pass"],
+)
+
+build_rule(
+    test = True,
+    name = "pass_test_fail_dep",
+    cmd = "touch $OUTS",
+    test_cmd = "exit 0",
+    outs = ["pass_test_fail_deps"],
+    deps = [":fail"],
+)