Add ability to disable diagnostics buffering (--no-diag-buffer)

11 files changed, 110 insertions, 23 deletions

diff --git a/build2/b.cxx b/build2/b.cxx
index f666b86..f91eff5 100644
--- a/build2/b.cxx
+++ b/build2/b.cxx
@@ -434,6 +434,7 @@ main (int argc, char* argv[])
                                ops.match_only (),
                                ops.no_external_modules (),
                                ops.dry_run (),
+                               ops.no_diag_buffer (),
                                !ops.serial_stop () /* keep_going */,
                                cmdl.cmd_vars));
 
diff --git a/libbuild2/b-options.cxx b/libbuild2/b-options.cxx
index 8d82d61..c1e5f23 100644
--- a/libbuild2/b-options.cxx
+++ b/libbuild2/b-options.cxx
@@ -290,6 +290,7 @@ namespace build2
     max_stack_specified_ (false),
     serial_stop_ (),
     dry_run_ (),
+    no_diag_buffer_ (),
     match_only_ (),
     no_external_modules_ (),
     structured_result_ (),
@@ -495,6 +496,12 @@ namespace build2
         this->dry_run_, a.dry_run_);
     }
 
+    if (a.no_diag_buffer_)
+    {
+      ::build2::build::cli::parser< bool>::merge (
+        this->no_diag_buffer_, a.no_diag_buffer_);
+    }
+
     if (a.match_only_)
     {
       ::build2::build::cli::parser< bool>::merge (
@@ -734,7 +741,10 @@ namespace build2
        << "                        build system errors rather than compilation errors." << ::std::endl
        << "                        Note that if you don't want to keep going but still" << ::std::endl
        << "                        want parallel execution, add \033[1m--jobs|-j\033[0m (for example \033[1m-j" << ::std::endl
-       << "                        0\033[0m for default concurrency)." << ::std::endl;
+       << "                        0\033[0m for default concurrency). Note also that during" << ::std::endl
+       << "                        serial execution there is no diagnostics buffering and" << ::std::endl
+       << "                        child process' \033[1mstderr\033[0m is a terminal (unless redirected;" << ::std::endl
+       << "                        see \033[1m--no-diag-buffer\033[0m for details)." << ::std::endl;
 
     os << std::endl
        << "\033[1m--dry-run\033[0m|\033[1m-n\033[0m            Print commands without actually executing them. Note" << ::std::endl
@@ -747,6 +757,18 @@ namespace build2
        << "                        meta-operation supports this mode." << ::std::endl;
 
     os << std::endl
+       << "\033[1m--no-diag-buffer\033[0m        Do not buffer diagnostics from child processes. By" << ::std::endl
+       << "                        default, unless running serially, such diagnostics is" << ::std::endl
+       << "                        buffered and printed all at once after each child exits" << ::std::endl
+       << "                        in order to prevent interleaving. However, this can" << ::std::endl
+       << "                        have side-effects since the child process' \033[1mstderr\033[0m is no" << ::std::endl
+       << "                        longer a terminal. Most notably, the use of color in" << ::std::endl
+       << "                        diagnostics will be disabled by most programs. On the" << ::std::endl
+       << "                        other hand, depending on the platform and programs" << ::std::endl
+       << "                        invoked, the interleaving diagnostics may not break" << ::std::endl
+       << "                        lines and thus could be tolerable." << ::std::endl;
+
+    os << std::endl
        << "\033[1m--match-only\033[0m            Match the rules but do not execute the operation. This" << ::std::endl
        << "                        mode is primarily useful for profiling." << ::std::endl;
 
@@ -1008,6 +1030,8 @@ namespace build2
       &::build2::build::cli::thunk< b_options, &b_options::dry_run_ >;
       _cli_b_options_map_["-n"] =
       &::build2::build::cli::thunk< b_options, &b_options::dry_run_ >;
+      _cli_b_options_map_["--no-diag-buffer"] =
+      &::build2::build::cli::thunk< b_options, &b_options::no_diag_buffer_ >;
       _cli_b_options_map_["--match-only"] =
       &::build2::build::cli::thunk< b_options, &b_options::match_only_ >;
       _cli_b_options_map_["--no-external-modules"] =
diff --git a/libbuild2/b-options.hxx b/libbuild2/b-options.hxx
index 2780a8d..4e85192 100644
--- a/libbuild2/b-options.hxx
+++ b/libbuild2/b-options.hxx
@@ -141,6 +141,9 @@ namespace build2
     dry_run () const;
 
     const bool&
+    no_diag_buffer () const;
+
+    const bool&
     match_only () const;
 
     const bool&
@@ -275,6 +278,7 @@ namespace build2
     bool max_stack_specified_;
     bool serial_stop_;
     bool dry_run_;
+    bool no_diag_buffer_;
     bool match_only_;
     bool no_external_modules_;
     structured_result_format structured_result_;
diff --git a/libbuild2/b-options.ixx b/libbuild2/b-options.ixx
index f43dce2..895831f 100644
--- a/libbuild2/b-options.ixx
+++ b/libbuild2/b-options.ixx
@@ -153,6 +153,12 @@ namespace build2
   }
 
   inline const bool& b_options::
+  no_diag_buffer () const
+  {
+    return this->no_diag_buffer_;
+  }
+
+  inline const bool& b_options::
   match_only () const
   {
     return this->match_only_;
diff --git a/libbuild2/b.cli b/libbuild2/b.cli
index 5ea3325..4b5e459 100644
--- a/libbuild2/b.cli
+++ b/libbuild2/b.cli
@@ -608,7 +608,10 @@ namespace build2
        investigate build failures that are caused by build system errors
        rather than compilation errors. Note that if you don't want to keep
        going but still want parallel execution, add \cb{--jobs|-j} (for
-       example \cb{-j\ 0} for default concurrency)."
+       example \cb{-j\ 0} for default concurrency). Note also that during
+       serial execution there is no diagnostics buffering and child
+       process' \cb{stderr} is a terminal (unless redirected; see
+       \cb{--no-diag-buffer} for details)."
     }
 
     bool --dry-run|-n
@@ -622,6 +625,19 @@ namespace build2
        this mode."
     }
 
+    bool --no-diag-buffer
+    {
+      "Do not buffer diagnostics from child processes. By default, unless
+       running serially, such diagnostics is buffered and printed all at
+       once after each child exits in order to prevent interleaving.
+       However, this can have side-effects since the child process'
+       \cb{stderr} is no longer a terminal. Most notably, the use of
+       color in diagnostics will be disabled by most programs. On the
+       other hand, depending on the platform and programs invoked, the
+       interleaving diagnostics may not break lines and thus could be
+       tolerable."
+    }
+
     bool --match-only
     {
       "Match the rules but do not execute the operation. This mode is primarily
diff --git a/libbuild2/cc/msvc.cxx b/libbuild2/cc/msvc.cxx
index 9aff347..765b1c6 100644
--- a/libbuild2/cc/msvc.cxx
+++ b/libbuild2/cc/msvc.cxx
@@ -187,9 +187,9 @@ namespace build2
         break;
       }
     }
-    catch (const io_error&)
+    catch (const io_error& e)
     {
-      // Let the following diag_buffer::read() call deal with this.
+      fail << "unable to read from " << dbuf.args0 << " stderr: " << e;
     }
 
     void
@@ -229,9 +229,9 @@ namespace build2
         break;
       }
     }
-    catch (const io_error&)
+    catch (const io_error& e)
     {
-      // Let the following diag_buffer::read() call deal with this.
+      fail << "unable to read from " << dbuf.args0 << " stderr: " << e;
     }
 
     void
diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx
index e44a688..0108d27 100644
--- a/libbuild2/context.cxx
+++ b/libbuild2/context.cxx
@@ -67,6 +67,7 @@ namespace build2
            bool mo,
            bool nem,
            bool dr,
+           bool ndb,
            bool kg,
            const strings& cmd_vars,
            optional<context*> mc,
@@ -78,6 +79,7 @@ namespace build2
         match_only (mo),
         no_external_modules (nem),
         dry_run_option (dr),
+        no_diag_buffer (ndb),
         keep_going (kg),
         phase_mutex (*this),
         scopes (data_->scopes),
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 40a8bdd..c16181f 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -258,6 +258,10 @@ namespace build2
     bool dry_run = false;
     bool dry_run_option;
 
+    // Diagnostics buffering flag (--no-diag-buffer).
+    //
+    bool no_diag_buffer;
+
     // Keep going flag.
     //
     // Note that setting it to false is not of much help unless we are running
@@ -644,6 +648,7 @@ namespace build2
              bool match_only = false,
              bool no_external_modules = false,
              bool dry_run = false,
+             bool no_diag_buffer = false,
              bool keep_going = true,
              const strings& cmd_vars = {},
              optional<context*> module_context = nullptr,
diff --git a/libbuild2/diagnostics.cxx b/libbuild2/diagnostics.cxx
index 7bdfea2..629febf 100644
--- a/libbuild2/diagnostics.cxx
+++ b/libbuild2/diagnostics.cxx
@@ -57,9 +57,10 @@ namespace build2
     assert (blocking); // @@ TODO (also in read() and close () below).
 
     serial = ctx_.sched.serial ();
+    nobuf = !serial && ctx_.no_diag_buffer;
 
     process::pipe r;
-    if (!serial || force)
+    if (!(serial || nobuf) || force)
     {
       try
       {
@@ -70,11 +71,11 @@ namespace build2
         //
         r = process::pipe (p.in.get (), move (p.out));
 
-        is.open (move (p.in), fdstream_mode::text, ifdstream::badbit);
+        is.open (move (p.in), fdstream_mode::text);
       }
       catch (const io_error& e)
       {
-        fail << "unable to read from " << args0 << " stderr:" << e;
+        fail << "unable to read from " << args0 << " stderr: " << e;
       }
     }
     else
@@ -97,7 +98,7 @@ namespace build2
       {
         if (is.blocking ())
         {
-          if (serial)
+          if (serial || nobuf)
           {
             // This is the case where we are called after custom processing.
             //
@@ -109,12 +110,28 @@ namespace build2
             //
             if (is.peek () != ifdstream::traits_type::eof ())
             {
-              // Holding the diag lock while waiting for diagnostics from the
-              // child process would be a bad idea in the parallel build. But
-              // it should be harmless in serial.
-              //
-              diag_stream_lock l;
-              *diag_stream << is.rdbuf ();
+              if (serial)
+              {
+                // Holding the diag lock while waiting for diagnostics from
+                // the child process would be a bad idea in the parallel
+                // build. But it should be harmless in serial.
+                //
+                // @@ TODO: do direct buffer copy.
+                //
+                diag_stream_lock dl;
+                *diag_stream << is.rdbuf ();
+              }
+              else
+              {
+                // Read/write one line at a time not to hold the lock for too
+                // long.
+                //
+                for (string l; !eof (std::getline (is, l)); )
+                {
+                  diag_stream_lock dl;
+                  *diag_stream << l << '\n';
+                }
+              }
             }
           }
           else
@@ -156,7 +173,7 @@ namespace build2
         // For now we assume (here and pretty much everywhere else) that the
         // output can't fail.
         //
-        fail << "unable to read from " << args0 << " stderr:" << e;
+        fail << "unable to read from " << args0 << " stderr: " << e;
       }
     }
     else
@@ -173,11 +190,11 @@ namespace build2
   {
     // Similar logic to read() above.
     //
-    if (serial)
+    if (serial || nobuf)
     {
       assert (buf.empty ());
 
-      diag_stream_lock l;
+      diag_stream_lock dl;
       *diag_stream << s;
       if (nl)
         *diag_stream << '\n';
@@ -221,7 +238,7 @@ namespace build2
         }
         catch (const io_error& e)
         {
-          fail << "unable to read from " << args0 << " stderr:" << e;
+          fail << "unable to read from " << args0 << " stderr: " << e;
         }
       }
 
diff --git a/libbuild2/diagnostics.hxx b/libbuild2/diagnostics.hxx
index a56fd64..17996b1 100644
--- a/libbuild2/diagnostics.hxx
+++ b/libbuild2/diagnostics.hxx
@@ -30,7 +30,7 @@ namespace build2
   //
   // - Stream it (if the input can be split into diagnostic records).
   //
-  // - Do nothing (in serial builds).
+  // - Do nothing (in serial builds or if requested not to buffer).
   //
   // This class is also responsible for converting the diagnostics into the
   // structured form (which means it may need to buffer even in serial
@@ -40,7 +40,7 @@ namespace build2
   {
   public:
     explicit
-    diag_buffer (context& c): ctx_ (c) {}
+    diag_buffer (context& c): is (ifdstream::badbit), ctx_ (c) {}
 
   public:
     // If buffering is necessary or force is true, open a pipe and return the
@@ -161,17 +161,28 @@ namespace build2
     // Direct access to the underlying stream and buffer for custom processing
     // (see read() above for details).
     //
+    // If serial is true, then we are running serially. If nobuf is true,
+    // then we are running in parallel but diagnostics buffering has been
+    // disabled (--no-diag-buffer). Note that there is a difference: during
+    // the serial execution we are free to hold the diag_stream_lock for as
+    // long as convenient, for example, for the whole duration of child
+    // process execution. Doing the same during parallel execution is very
+    // bad idea and we should read/write the diagnostics in chunks, normally
+    // one line at a time.
+    //
   public:
     ifdstream    is;
     vector<char> buf;
     const char*  args0;
     bool         serial;
+    bool         nobuf;
 
     // Buffer or stream a fragment of diagnostics as necessary. If newline
     // is true, also add a trailing newline.
     //
     // This function is normally called from a custom diagnostics processing
-    // implementation (see read() above for details).
+    // implementation (see read() above for details). If nobuf is true, then
+    // the fragment should end on the line boundary to avoid interleaving.
     //
     void
     write (const string&, bool newline);
diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx
index f200a63..96901ff 100644
--- a/libbuild2/module.cxx
+++ b/libbuild2/module.cxx
@@ -84,6 +84,7 @@ namespace build2
                    false,                    /* match_only */
                    false,                    /* no_external_modules */
                    false,                    /* dry_run */
+                   ctx.no_diag_buffer,
                    ctx.keep_going,
                    ctx.global_var_overrides, /* cmd_vars */
                    nullopt));                /* module_context */