Add support for soft and hard rebuilds

2 files changed, 179 insertions, 27 deletions

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<unsigned int> (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 -<toolchain> 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<regex>;
 // 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 <typename... A>
 static result_status
 run_cmd (step_id step,
          tracer& t,
-         string& log, const regexes& warn_detect,
+         string& log, optional<string>* out, const regexes& warn_detect,
          const string& name,
          const optional<step_id>& bkp_step,
          const optional<result_status>& 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> (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<string>* out, const regexes& warn_detect,
           const optional<step_id>& bkp_step,
           const optional<result_status>& 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> (a)...);
 }
 
+template <typename V, typename... A>
+static result_status
+run_bpkg (step_id step,
+          const V& envvars,
+          tracer& t,
+          string& log, const regexes& warn_detect,
+          const optional<step_id>& bkp_step,
+          const optional<result_status>& 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> (a)...);
+}
+
+template <typename... A>
+static result_status
+run_bpkg (step_id step,
+          tracer& t,
+          string& log, optional<string>* out, const regexes& warn_detect,
+          const optional<step_id>& bkp_step,
+          const optional<result_status>& 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> (a)...);
+}
+
 template <typename... A>
 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<task_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<string> 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