From 3a206403c962c2bff914bc1d6b1a1acc34c742a2 Mon Sep 17 00:00:00 2001 From: Richard Godden <7768980+goddenrich@users.noreply.github.com> Date: Thu, 26 Oct 2023 17:46:06 +0100 Subject: [PATCH] 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 --- ChangeLog | 6 +++ VERSION | 2 +- src/build/build_step.go | 1 + src/core/build_target.go | 29 +++++----- src/core/state.go | 9 ++++ src/output/targets.go | 4 +- src/please.go | 2 + test/build_defs/test.build_defs | 2 +- test/keep_going/BUILD | 27 ++++++++++ test/keep_going/test_repo/.plzconfig | 0 test/keep_going/test_repo/package/BUILD_FILE | 56 ++++++++++++++++++++ 11 files changed, 122 insertions(+), 16 deletions(-) create mode 100644 test/keep_going/BUILD create mode 100644 test/keep_going/test_repo/.plzconfig create mode 100644 test/keep_going/test_repo/package/BUILD_FILE diff --git a/ChangeLog b/ChangeLog index b822c00359..8b6fc35472 100644 --- 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 index 1f33a6f48f..26a12aeb5f 100644 --- 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 index 25e2873e0e..944bd329ca 100644 --- 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 index 1ec22f527d..f8aa20b131 100644 --- 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 index aa735fecbc..65410ead66 100644 --- 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 index ffaab80b06..dcc99a5c00 100644 --- 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 index 5f72bd82df..528da95f14 100644 --- 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 index c03ee70bd5..4745faba10 100644 --- 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 new file mode 100644 index 0000000000..250c7c9847 --- /dev/null +++ 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 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/keep_going/test_repo/package/BUILD_FILE b/test/keep_going/test_repo/package/BUILD_FILE new file mode 100644 index 0000000000..2a91f436fa --- /dev/null +++ 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"], +)