Restore original error/ignore_exit semantics in run<>() overloads

3 files changed, 64 insertions, 74 deletions

diff --git a/libbuild2/utility.cxx b/libbuild2/utility.cxx
index d4d632c..75791ee 100644
--- a/libbuild2/utility.cxx
+++ b/libbuild2/utility.cxx
@@ -291,6 +291,16 @@ namespace build2
     const process_exit& pe (*pr.exit);
     bool ne (pe.normal ());
 
+    // Even if the user redirected the diagnostics, one error that we want to
+    // let through is the inability to execute the program itself. We cannot
+    // reserve a special exit status to signal this so we will just have to
+    // compare the output. In a sense, we treat this as a special case of
+    // abnormal termination. This particular situation will result in a single
+    // error line printed by run_start() above.
+    //
+    if (ne && l.compare (0, 18, "unable to execute ") == 0)
+      fail (loc) << l;
+
     if (omit_normal && ne)
     {
       // While we assume diagnostics has already been issued (to stderr), if
@@ -302,19 +312,6 @@ namespace build2
     }
     else
     {
-      // Even if the user redirected the diagnostiscs, one error that we want
-      // to let through is the inability to execute the program itself. We
-      // cannot reserve a special exit status to signal this so we will just
-      // have to compare the output. This particular situation will result in
-      // a single error line printed by run_start() above.
-      //
-      // Shouldn't we treat this as abnormal termination? But if we didn't
-      // redirect stderr to stdout, then this will be handled as normal exit.
-      // So let's be consistent even if wrong.
-      //
-      if (ne && l.compare (0, 18, "unable to execute ") == 0)
-        error (loc) << l;
-
       // It's unclear whether we should print this only if printing the
       // command line (we could also do things differently for normal/abnormal
       // exit). Let's print this always and see how it wears. Note that we
@@ -414,15 +411,19 @@ namespace build2
        bool ignore_exit,
        sha256* checksum)
   {
+    assert (!err || !ignore_exit);
+
     if (err && ctx.phase != run_phase::load)
     {
       diag_buffer dbuf (ctx);
-      return run (dbuf,
-                  verbosity,
-                  pe, args,
-                  finish_verbosity,
-                  f,
-                  tr, ignore_exit, checksum);
+      run (dbuf,
+           verbosity,
+           pe, args,
+           finish_verbosity,
+           f,
+           tr,
+           checksum);
+      return true;
     }
 
     process pr (run_start (verbosity,
@@ -472,13 +473,16 @@ namespace build2
       // caused by that and let run_finish() deal with it.
     }
 
-    if (!(run_finish_impl (args, pr, err, l, finish_verbosity) || ignore_exit))
+    // Omit normal exit code diagnostics if err is false.
+    //
+    if (!(run_finish_impl (args, pr, err, l, finish_verbosity, !err) ||
+          ignore_exit))
       return false;
 
     return true;
   }
 
-  bool
+  void
   run (diag_buffer& dbuf,
        uint16_t verbosity,
        const process_env& pe,
@@ -486,7 +490,6 @@ namespace build2
        uint16_t finish_verbosity,
        const function<bool (string&, bool)>& f,
        bool tr,
-       bool ignore_exit,
        sha256* checksum)
   {
     // We have to use the non-blocking setup since we have to read from stdout
@@ -601,7 +604,7 @@ namespace build2
       // caused by that and let run_finish() deal with it.
     }
 
-    return run_finish_impl (dbuf, args, pr, !ignore_exit, finish_verbosity);
+    run_finish_impl (dbuf, args, pr, true /* fail */, finish_verbosity);
   }
 
   fdpipe
diff --git a/libbuild2/utility.hxx b/libbuild2/utility.hxx
index 87d325b..0271642 100644
--- a/libbuild2/utility.hxx
+++ b/libbuild2/utility.hxx
@@ -563,9 +563,10 @@ namespace build2
   // If verbosity is specified, print the process commands line at that level
   // (with the verbosite-1 value passed run_finish()).
   //
-  // If error is false, then redirecting stderr to stdout (can be used to
-  // suppress and/or analyze diagnostics from the child process). Otherwise,
-  // buffer diagnostics unless in the load phase.
+  // If error is false, then redirecting stderr to stdout and don't fail if
+  // the process exits normally but with non-0 code (can be used to suppress
+  // and/or analyze diagnostics from the child process). Otherwise, buffer
+  // diagnostics unless in the load phase.
   //
   // The predicate can move the value out of the passed string but, if error
   // is false, only in case of a "content match" (so that any diagnostics
@@ -573,9 +574,9 @@ namespace build2
   //
   // T (string& line, bool last)
   //
-  // If ignore_exit is true, then the program's normal exit status is ignored
-  // (if it is false and the program exits with the non-zero status, then an
-  // empty T instance is returned).
+  // If, in addition to error being false, ignore_exit is true, then the
+  // program's normal exit status is ignored (if it is false and the program
+  // exits with the non-zero status, then an empty T instance is returned).
   //
   // If checksum is not NULL, then feed it the content of each trimmed line
   // (including those that come after the callback returns non-empty object).
@@ -616,7 +617,6 @@ namespace build2
        const process_env&,
        const char* const* args,
        F&&,
-       bool ignore_exit = false,
        sha256* checksum = nullptr);
 
   template <typename T, typename F>
@@ -626,14 +626,13 @@ namespace build2
        const process_env& pe,
        const cstrings& args,
        F&& f,
-       bool ignore_exit = false,
        sha256* checksum = nullptr)
   {
     return run<T> (dbuf,
                    verbosity,
                    pe, args.data (),
                    forward<F> (f),
-                   ignore_exit, checksum);
+                   checksum);
   }
 
   template <typename T, typename F>
@@ -672,7 +671,6 @@ namespace build2
        const char* const* args,
        uint16_t finish_verbosity,
        F&&,
-       bool ignore_exit = false,
        sha256* checksum = nullptr);
 
   template <typename T, typename F>
@@ -682,14 +680,13 @@ namespace build2
        const cstrings& args,
        uint16_t finish_verbosity,
        F&& f,
-       bool ignore_exit = false,
        sha256* checksum = nullptr)
   {
     return run<T> (dbuf,
                    pe, args.data (),
                    finish_verbosity,
                    forward<F> (f),
-                   ignore_exit, checksum);
+                   checksum);
   }
 
   template <typename T, typename F>
@@ -733,7 +730,6 @@ namespace build2
        uint16_t verbosity,
        const char* args[],
        F&& f,
-       bool ignore_exit = false,
        sha256* checksum = nullptr)
   {
     process_path pp (run_search (args[0]));
@@ -741,7 +737,7 @@ namespace build2
                    verbosity,
                    pp, args,
                    forward<F> (f),
-                   ignore_exit, checksum);
+                   checksum);
   }
 
   template <typename T, typename F>
@@ -750,14 +746,13 @@ namespace build2
        uint16_t verbosity,
        cstrings& args,
        F&& f,
-       bool ignore_exit = false,
        sha256* checksum = nullptr)
   {
     return run<T> (dbuf,
                    verbosity,
                    args.data (),
                    forward<F> (f),
-                   ignore_exit, checksum);
+                   checksum);
   }
 
   // As above but run a program without any arguments or with one argument.
@@ -850,8 +845,9 @@ namespace build2
   // (i.e., the object is still empty in the T & F interface) and false
   // otherwise.
   //
-  // Ruturn true on success and false on failure (only if ignore_exit is
-  // true). (In the latter case, the T & F interface makes the resulting
+  // The first version ruturn true if the result is usable and false
+  // otherwise, depending on the process exit code and error/ignore_exit
+  // values. (In the latter case, the T & F interface makes the resulting
   // object empty).
   //
   LIBBUILD2_SYMEXPORT bool
@@ -862,11 +858,11 @@ namespace build2
        uint16_t finish_verbosity,
        const function<bool (string& line, bool last)>&,
        bool trim = true,
-       bool err = true,
+       bool error = true,
        bool ignore_exit = false,
        sha256* checksum = nullptr);
 
-  LIBBUILD2_SYMEXPORT bool
+  LIBBUILD2_SYMEXPORT void
   run (diag_buffer& dbuf,
        uint16_t verbosity,
        const process_env&,
@@ -874,7 +870,6 @@ namespace build2
        uint16_t finish_verbosity,
        const function<bool (string& line, bool last)>&,
        bool trim = true,
-       bool ignore_exit = false,
        sha256* checksum = nullptr);
 
   // File descriptor streams.
diff --git a/libbuild2/utility.ixx b/libbuild2/utility.ixx
index 2447303..3948623 100644
--- a/libbuild2/utility.ixx
+++ b/libbuild2/utility.ixx
@@ -174,24 +174,20 @@ namespace build2
        const process_env& pe,
        const char* const* args,
        F&& f,
-       bool ignore_exit,
        sha256* checksum)
   {
     T r;
-    if (!run (dbuf,
-              verbosity,
-              pe, args,
-              verbosity - 1,
-              [&r, &f] (string& l, bool last) // Small function optimmization.
-              {
-                r = f (l, last);
-                return r.empty ();
-              },
-              true /* trim */,
-              ignore_exit,
-              checksum))
-      r = T ();
-
+    run (dbuf,
+         verbosity,
+         pe, args,
+         verbosity - 1,
+         [&r, &f] (string& l, bool last) // Small function optimmization.
+         {
+           r = f (l, last);
+           return r.empty ();
+         },
+         true /* trim */,
+         checksum);
     return r;
   }
 
@@ -232,24 +228,20 @@ namespace build2
        const char* const* args,
        uint16_t finish_verbosity,
        F&& f,
-       bool ignore_exit,
        sha256* checksum)
   {
     T r;
-    if (!run (dbuf,
-              verb_never,
-              pe, args,
-              finish_verbosity,
-              [&r, &f] (string& l, bool last)
-              {
-                r = f (l, last);
-                return r.empty ();
-              },
-              true /* trim */,
-              ignore_exit,
-              checksum))
-      r = T ();
-
+    run (dbuf,
+         verb_never,
+         pe, args,
+         finish_verbosity,
+         [&r, &f] (string& l, bool last)
+         {
+           r = f (l, last);
+           return r.empty ();
+         },
+         true /* trim */,
+         checksum);
     return r;
   }