From 7be84c43231c2dc59e44e4c783729d6cb4f5431b Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Thu, 30 Sep 2021 21:34:43 +0300 Subject: Add support for soft and hard rebuilds --- bbot/agent/agent.cxx | 23 ++++++- bbot/worker/worker.cxx | 183 ++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 179 insertions(+), 27 deletions(-) (limited to 'bbot') diff --git a/bbot/agent/agent.cxx b/bbot/agent/agent.cxx index cfff5e4..cfd1e7d 100644 --- a/bbot/agent/agent.cxx +++ b/bbot/agent/agent.cxx @@ -803,7 +803,9 @@ try tm.name, tm.version, result_status::abort, - operation_results {}}; + operation_results {}, + nullopt /* worker_checksum */, + nullopt /* dependency_checksum */}; if (ops.fake_build ()) return r; @@ -1087,6 +1089,8 @@ handle_signal (int sig) } } +static const string agent_checksum ("1"); // Logic version. + int main (int argc, char* argv[]) try @@ -1296,6 +1300,8 @@ try // 4. If a build task is returned, do it, upload the result, and go to #1 // (immediately). // + // NOTE: consider updating agent_checksum if making any logic changes. + // auto rand_sleep = [g = std::mt19937 (std::random_device {} ())] () mutable { return std::uniform_int_distribution (50, 60) (g); @@ -1365,6 +1371,7 @@ try "fake-session", // Dummy session. nullopt, // No challenge. url, // Empty result URL. + agent_checksum, move (t)}; url = "http://example.org"; @@ -1515,6 +1522,15 @@ try if (!ops.trust ().empty ()) t.trust = ops.trust (); + // Reset the worker checksum if the task's agent checksum doesn't match + // the current one. + // + // Note that since the checksums are hierarchical, such reset will trigger + // resets of the "subordinate" checksums (dependency checksum, etc). + // + if (!tr.agent_checksum || *tr.agent_checksum != agent_checksum) + t.worker_checksum = nullopt; + const dir_path& d (); // The - directory. const bootstrapped_machine_manifest& m (); @@ -1562,7 +1578,10 @@ try // Upload the result. // - result_request_manifest rq {tr.session, move (challenge), move (r)}; + result_request_manifest rq {tr.session, + move (challenge), + agent_checksum, + move (r)}; { const string& u (*tr.result_url); diff --git a/bbot/worker/worker.cxx b/bbot/worker/worker.cxx index 250d0d1..4688373 100644 --- a/bbot/worker/worker.cxx +++ b/bbot/worker/worker.cxx @@ -311,6 +311,10 @@ using regexes = vector; // expressions and return the warning result status (instead of success) in // case of a match. Save the executed command into last_cmd. // +// Redirect stdout to stderr if the out argument is NULL. Otherwise, save the +// process output into the referenced variable. Note: currently assumes that +// the output will always fit into the pipe buffer. +// // If bkp_step is present and is equal to the command step, then prior to // running this command ask the user if to continue or abort the task // execution. If bkp_status is present, then ask for that if the command @@ -323,7 +327,7 @@ template static result_status run_cmd (step_id step, tracer& t, - string& log, const regexes& warn_detect, + string& log, optional* out, const regexes& warn_detect, const string& name, const optional& bkp_step, const optional& bkp_status, @@ -438,22 +442,28 @@ run_cmd (step_id step, { try { - fdpipe pipe (fdopen_pipe ()); // Text mode seems appropriate. + // Redirect stdout to stderr, if the caller is not interested in it. + // + // Text mode seems appropriate. + // + fdpipe out_pipe (out != nullptr ? fdopen_pipe () : fdpipe ()); + fdpipe err_pipe (fdopen_pipe ()); process pr ( process_start_callback (cmdc, fdopen_null (), // Never reads from stdin. - 2, // 1>&2 - pipe, + out != nullptr ? out_pipe.out.get () : 2, + err_pipe, pe, forward (a)...)); - pipe.out.close (); + out_pipe.out.close (); + err_pipe.out.close (); { // Skip on exception. // - ifdstream is (move (pipe.in), fdstream_mode::skip); + ifdstream is (move (err_pipe.in), fdstream_mode::skip); for (string l; is.peek () != ifdstream::traits_type::eof (); ) { @@ -494,6 +504,16 @@ run_cmd (step_id step, r = e.normal () ? result_status::error : result_status::abnormal; } + // Only read the buffered output if the process terminated normally. + // + if (out != nullptr && pr.exit->normal ()) + { + // Note: shouldn't throw since the output is buffered. + // + ifdstream is (move (out_pipe.in)); + *out = is.read_text (); + } + last_cmd = move (next_cmd); if (bkp_status && r >= *bkp_status) @@ -530,7 +550,7 @@ static result_status run_bpkg (step_id step, const V& envvars, tracer& t, - string& log, const regexes& warn_detect, + string& log, optional* out, const regexes& warn_detect, const optional& bkp_step, const optional& bkp_status, string& last_cmd, @@ -539,13 +559,54 @@ run_bpkg (step_id step, { return run_cmd (step, t, - log, warn_detect, + log, out, warn_detect, "bpkg " + cmd, bkp_step, bkp_status, last_cmd, process_env ("bpkg", envvars), verbosity, cmd, forward (a)...); } +template +static result_status +run_bpkg (step_id step, + const V& envvars, + tracer& t, + string& log, const regexes& warn_detect, + const optional& bkp_step, + const optional& bkp_status, + string& last_cmd, + const char* verbosity, + const string& cmd, A&&... a) +{ + return run_bpkg (step, + envvars, + t, + log, nullptr /* out */, warn_detect, + bkp_step, bkp_status, last_cmd, + verbosity, cmd, forward (a)...); +} + +template +static result_status +run_bpkg (step_id step, + tracer& t, + string& log, optional* out, const regexes& warn_detect, + const optional& bkp_step, + const optional& bkp_status, + string& last_cmd, + const char* verbosity, + const string& cmd, A&&... a) +{ + const char* const* envvars (nullptr); + + return run_bpkg (step, + envvars, + t, + log, out, warn_detect, + bkp_step, bkp_status, last_cmd, + verbosity, cmd, forward (a)...); +} + template static result_status run_bpkg (step_id step, @@ -590,7 +651,7 @@ run_b (step_id step, return run_cmd (step, t, - log, warn_detect, + log, nullptr /* out */, warn_detect, name, bkp_step, bkp_status, last_cmd, process_env ("b", envvars), @@ -611,7 +672,7 @@ run_b (step_id step, { return run_cmd (step, t, - log, warn_detect, + log, nullptr /* out */, warn_detect, "b " + buildspec, bkp_step, bkp_status, last_cmd, process_env ("b", envvars), @@ -723,6 +784,8 @@ upload_manifest (tracer& trace, } } +static const string worker_checksum ("1"); // Logic version. + static int bbot:: build (size_t argc, const char* argv[]) { @@ -743,6 +806,8 @@ build (size_t argc, const char* argv[]) // // 3. Upload the result manifest. // + // NOTE: consider updating agent_checksum if making any logic changes. + // // Note also that we are being "watched" by the startup version of us which // will upload an appropriate result in case we exit with an error. So here // for abnormal situations (like a failure to parse the manifest), we just @@ -751,11 +816,19 @@ build (size_t argc, const char* argv[]) task_manifest tm ( parse_manifest (path ("task.manifest"), "task")); + // Reset the dependency checksum if the task's worker checksum doesn't match + // the current one. + // + if (!tm.worker_checksum || *tm.worker_checksum != worker_checksum) + tm.dependency_checksum = nullopt; + result_manifest rm { tm.name, tm.version, result_status::success, - operation_results {} + operation_results {}, + worker_checksum, + nullopt /* dependency_checksum */ }; auto add_result = [&rm] (string o) -> operation_result& @@ -779,12 +852,14 @@ build (size_t argc, const char* argv[]) for (;;) // The "breakout" loop. { - auto abort_operation = [&trace] (operation_result& r, const string& e) + auto fail_operation = [&trace] (operation_result& r, + const string& e, + result_status s) { l3 ([&]{trace << e;}); r.log += "error: " + e + '\n'; - r.status = result_status::abort; + r.status = s; }; // Regular expressions that detect different forms of build2 toolchain @@ -826,8 +901,9 @@ build (size_t argc, const char* argv[]) if (!bkp_step && !bkp_status) { - abort_operation (add_result ("configure"), - "invalid interactive build breakpoint '" + b + "'"); + fail_operation (add_result ("configure"), + "invalid interactive build breakpoint '" + b + "'", + result_status::abort); break; } @@ -1213,9 +1289,10 @@ build (size_t argc, const char* argv[]) // if (target_pkg && has_buildtime_tests) { - abort_operation ( + fail_operation ( add_result ("configure"), - "build-time tests in package not marked with `requires: host`"); + "build-time tests in package not marked with `requires: host`", + result_status::abort); break; } @@ -1755,22 +1832,22 @@ build (size_t argc, const char* argv[]) // Finally, configure all the packages. // - // Note that in the future we can run this command with the - // --rebuild-checksum option first to obtain the dependency tree - // checksum and bail out if it didn't change since the previous build. - // { step_id b (step_id::bpkg_configure_build); step_id s (step_id::bpkg_global_configure_build); + optional dependency_checksum; + r.status |= run_bpkg ( b, - trace, r.log, wre, + trace, r.log, &dependency_checksum, wre, bkp_step, bkp_status, last_cmd, "-v", "build", "--configure-only", "--checkout-root", dist_root, + "--rebuild-checksum", + tm.dependency_checksum ? *tm.dependency_checksum : "", "--yes", "-d", root_conf, step_args (env_args, s), @@ -1778,6 +1855,59 @@ build (size_t argc, const char* argv[]) "--", pkg_args); + // The dependency checksum is tricky, here are the possibilities: + // + // - absent: bpkg terminated abnormally (or was not executed due to + // a breakpoint) -- nothing to do here. + // + // - empty: bpkg terminated normally with error before calculating the + // checksum -- nothing to do here either. + // + // - one line: bpkg checksum that we want. + // + // - many lines: someone else (e.g., buildfile) printed to stdout, + // which we consider an error. + // + if (dependency_checksum && !dependency_checksum->empty ()) + { + string& s (*dependency_checksum); + + // Make sure that the output contains a single line, and bail out + // with the error status if that's not the case. + // + if (s.find ('\n') == s.size () - 1) + { + s.pop_back (); + + // If the dependency checksum didn't change, then save it to the + // result manifest, clean the logs and bail out with the skip + // result status. + // + if (tm.dependency_checksum && *tm.dependency_checksum == s) + { + l3 ([&]{trace << "skip";}); + + rm.status = result_status::skip; + rm.dependency_checksum = move (s); + rm.results.clear (); + break; + } + + // Save the (new) dependency checksum to the result manifest. + // + // Also note that we save the checksum if bpkg failed after the + // checksum was printed. As a result, we won't be rebuilding the + // package until the error is fixed (in a package or worker) and + // the checksum changes, which feels like a proper behavior. + // + rm.dependency_checksum = move (s); + } + else + fail_operation (r, + "unexpected bpkg output:\n'" + s + "'", + result_status::error); + } + if (!r.status) break; } @@ -2756,7 +2886,7 @@ build (size_t argc, const char* argv[]) if (!error) { r.status |= run_cmd (step_id::end, - trace, r.log, regexes (), + trace, r.log, nullptr /* out */, regexes (), "" /* name */, bkp_step, bkp_status, last_cmd, process_env ()); @@ -2805,7 +2935,8 @@ build (size_t argc, const char* argv[]) } } else - assert (rm.status == result_status::abort); + assert (rm.status == result_status::abort || + rm.status == result_status::skip); if (!rwd.empty ()) change_wd (trace, nullptr /* log */, rwd); @@ -2999,7 +3130,9 @@ startup () tm.name.empty () ? bpkg::package_name ("unknown") : tm.name, tm.version.empty () ? bpkg::version ("0") : tm.version, result_status::abnormal, - operation_results {} + operation_results {}, + worker_checksum, + nullopt /* dependency_checksum */ }; try -- cgit v1.1