Fallback to output directory removal for --keep-out on disfigure failure

15 files changed, 286 insertions, 267 deletions

diff --git a/bpkg/archive.cxx b/bpkg/archive.cxx
index cc96cd1..8ebd4cd 100644
--- a/bpkg/archive.cxx
+++ b/bpkg/archive.cxx
@@ -6,6 +6,7 @@
 
 #include <libbutl/process.mxx>
 
+#include <bpkg/utility.hxx>
 #include <bpkg/diagnostics.hxx>
 
 using namespace std;
diff --git a/bpkg/cfg-create.cxx b/bpkg/cfg-create.cxx
index 04d97f4..c55623a 100644
--- a/bpkg/cfg-create.cxx
+++ b/bpkg/cfg-create.cxx
@@ -75,10 +75,9 @@ namespace bpkg
     // Run quiet. Use path representation to get canonical trailing slash.
     //
     run_b (o,
-           c,
-           "create('" + c.representation () + "'" + mods + ")",
            verb_b::quiet,
-           vars);
+           vars,
+           "create('" + c.representation () + "'" + mods + ")");
 
     // Create .bpkg/ and its subdirectories.
     //
diff --git a/bpkg/diagnostics.cxx b/bpkg/diagnostics.cxx
index bb175d8..5dd59e5 100644
--- a/bpkg/diagnostics.cxx
+++ b/bpkg/diagnostics.cxx
@@ -12,6 +12,8 @@
 #include <libbutl/process.mxx>
 #include <libbutl/process-io.mxx> // operator<<(ostream, process_arg)
 
+#include <bpkg/utility.hxx>
+
 using namespace std;
 using namespace butl;
 
diff --git a/bpkg/diagnostics.hxx b/bpkg/diagnostics.hxx
index cbce2f2..ca277d4 100644
--- a/bpkg/diagnostics.hxx
+++ b/bpkg/diagnostics.hxx
@@ -5,12 +5,13 @@
 #ifndef BPKG_DIAGNOSTICS_HXX
 #define BPKG_DIAGNOSTICS_HXX
 
+#include <utility> // forward()
+
 #include <odb/tracer.hxx>
 
 #include <libbutl/diagnostics.mxx>
 
-#include <bpkg/types.hxx>
-#include <bpkg/utility.hxx>
+#include <bpkg/types.hxx> // Note: not <bpkg/utility.hxx>
 
 namespace bpkg
 {
@@ -159,7 +160,9 @@ namespace bpkg
         epilogue_,
         type_,
         name_,
-        location (forward<F> (f), forward<L> (l), forward<C> (c)));
+        location (std::forward<F> (f),
+                  std::forward<L> (l),
+                  std::forward<C> (c)));
     }
 
   protected:
diff --git a/bpkg/manifest-utility.cxx b/bpkg/manifest-utility.cxx
index 0f64b30..0e0c792 100644
--- a/bpkg/manifest-utility.cxx
+++ b/bpkg/manifest-utility.cxx
@@ -6,8 +6,6 @@
 
 #include <libbutl/url.mxx>
 #include <libbutl/sha256.mxx>
-#include <libbutl/process.mxx>
-#include <libbutl/process-io.mxx> // operator<<(ostream, process_path)
 
 #include <bpkg/diagnostics.hxx>
 #include <bpkg/common-options.hxx>
@@ -200,92 +198,68 @@ namespace bpkg
   optional<version>
   package_version (const common_options& o, const dir_path& d)
   {
-    const char* b (name_b (o));
+    fdpipe pipe (open_pipe ());
+
+    process pr (start_b (o,
+                         pipe, 2 /* stderr */,
+                         verb_b::quiet,
+                         "info:",
+                         d.representation ()));
+
+    // Shouldn't throw, unless something is severely damaged.
+    //
+    pipe.out.close ();
 
     try
     {
-      process_path pp (process::path_search (b, exec_dir));
-
-      fdpipe pipe (open_pipe ());
-
-      process pr (
-        process_start_callback (
-          [] (const char* const args[], size_t n)
-          {
-            if (verb >= 2)
-              print_process (args, n);
-          },
-          0 /* stdin */, pipe /* stdout */, 2 /* stderr */,
-          pp,
-
-          verb < 2
-          ? strings ({"-q"})
-          : verb == 2
-          ? strings ({"-v"})
-          : strings ({"--verbose", to_string (verb)}),
-
-          o.build_option (),
-          "info:",
-          d.representation ()));
-
-      // Shouldn't throw, unless something is severely damaged.
-      //
-      pipe.out.close ();
+      optional<version> r;
 
-      try
+      ifdstream is (move (pipe.in),
+                    fdstream_mode::skip,
+                    ifdstream::badbit);
+
+      for (string l; !eof (getline (is, l)); )
       {
-        optional<version> r;
+        if (l.compare (0, 9, "version: ") == 0)
+        try
+        {
+          string v (l, 9);
 
-        ifdstream is (move (pipe.in),
-                      fdstream_mode::skip,
-                      ifdstream::badbit);
+          // An empty version indicates that the version module is not
+          // enabled for the project.
+          //
+          if (!v.empty ())
+            r = version (v);
 
-        for (string l; !eof (getline (is, l)); )
+          break;
+        }
+        catch (const invalid_argument&)
         {
-          if (l.compare (0, 9, "version: ") == 0)
-          try
-          {
-            string v (l, 9);
-
-            // An empty version indicates that the version module is not
-            // enabled for the project.
-            //
-            if (!v.empty ())
-              r = version (v);
-
-            break;
-          }
-          catch (const invalid_argument&)
-          {
-            fail << "no package version in '" << l << "'" <<
-              info << "produced by '" << pp << "'; use --build to override";
-          }
+          fail << "no package version in '" << l << "'" <<
+            info << "produced by '" << name_b (o) << "'; use --build to "
+                 << "override";
         }
-
-        is.close ();
-
-        if (pr.wait ())
-          return r;
-
-        // Fall through.
       }
-      catch (const io_error&)
-      {
-        if (pr.wait ())
-          fail << "unable to read '" << b << "' output";
 
-        // Fall through.
-      }
+      is.close ();
 
-      // We should only get here if the child exited with an error status.
-      //
-      assert (!pr.wait ());
+      if (pr.wait ())
+        return r;
 
-      fail << "unable to obtain version using '" << b << "'" << endf;
+      // Fall through.
     }
-    catch (const process_error& e)
+    catch (const io_error&)
     {
-      fail << "unable to execute '" << b << "': " << e << endf;
+      if (pr.wait ())
+        fail << "unable to read '" << name_b (o) << "' output";
+
+      // Fall through.
     }
+
+    // We should only get here if the child exited with an error status.
+    //
+    assert (!pr.wait ());
+
+    fail << "unable to obtain version using '" << name_b (o) << "'" << endf;
   }
 }
diff --git a/bpkg/pkg-checkout.cxx b/bpkg/pkg-checkout.cxx
index 18ce2b9..e78dd9b 100644
--- a/bpkg/pkg-checkout.cxx
+++ b/bpkg/pkg-checkout.cxx
@@ -198,10 +198,9 @@ namespace bpkg
         text << "distributing " << n << '/' << v;
 
       run_b (o,
-             c,
-             bspec,
              verb_b::progress,
-             strings ({"config.dist.root=" + c.representation ()}));
+             strings ({"config.dist.root=" + c.representation ()}),
+             bspec);
 
       mc = sha256 (o, d / manifest_file);
     }
diff --git a/bpkg/pkg-command.cxx b/bpkg/pkg-command.cxx
index b28c99a..bf7107e 100644
--- a/bpkg/pkg-command.cxx
+++ b/bpkg/pkg-command.cxx
@@ -28,20 +28,39 @@ namespace bpkg
 
     l4 ([&]{trace << "command: " << cmd;});
 
+    // Set common vars on the configuration scope.
+    //
+    cstrings gvars;
+    strings  lvars;
+    lvars.reserve (cvars.size ());
+
+    for (const string& v: cvars)
+    {
+      // Don't scope-qualify global variables.
+      //
+      if (v[0] == '!')
+        gvars.push_back (v.c_str ());
+
+      // Use path representation to get canonical trailing slash.
+      //
+      else
+        lvars.push_back (c.representation () + ':' + v);
+    }
+
     // This one is a bit tricky: we can only update all the packages at once if
     // they don't have any package-specific variables. But let's try to handle
     // this with the same logic (being clever again).
     //
     string bspec;
 
-    auto run =
-      [&trace, &c, &o, &cvars, &bspec] ( const strings& vars = strings ())
+    auto run = [&trace, &c, &o, &lvars, &gvars, &bspec] (
+      const strings& vars = strings ())
     {
       if (!bspec.empty ())
       {
         bspec += ')';
         l4 ([&]{trace << "buildspec: " << bspec;});
-        run_b (o, c, bspec, verb_b::normal, vars, cvars);
+        run_b (o, verb_b::normal, gvars, lvars, vars, bspec);
         bspec.clear ();
       }
     };
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index e351cfd..cfa3d37 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -162,7 +162,7 @@ namespace bpkg
       //
       try
       {
-        run_b (o, c, bspec, verb_b::quiet, vars);
+        run_b (o, verb_b::quiet, vars, bspec);
       }
       catch (const failed&)
       {
diff --git a/bpkg/pkg-disfigure.cxx b/bpkg/pkg-disfigure.cxx
index 4a0bd72..0e82f18 100644
--- a/bpkg/pkg-disfigure.cxx
+++ b/bpkg/pkg-disfigure.cxx
@@ -123,10 +123,45 @@ namespace bpkg
           // So in this case we just remove the output directory manually
           // rather then running 'b clean disfigure'.
           //
-          if (clean && p->external ())
-            rm_r (out_root);
+          // It may also happen that we can not disfigure the external
+          // package' output directory (the source directory have moved, etc.).
+          // If that's the case, then we fallback to the output directory
+          // removal.
+          //
+          if (p->external ())
+          {
+            if (!clean)
+            {
+              auto_fd dev_null (open_dev_null ());
+
+              // Redirect STDERR to /dev/null. Note that we don't expect
+              // anything to be written to STDOUT.
+              //
+              process pr (start_b (o,
+                                   1 /* stdout */, dev_null /* stderr */,
+                                   verb_b::quiet,
+                                   bspec));
+
+              // If the disfigure meta-operation failed then we report the
+              // abnormal termination and fallback to the output directory
+              // removal otherwise.
+              //
+              if (!pr.wait ())
+              {
+                const process_exit& e (*pr.exit);
+
+                if (!e.normal ())
+                  fail << "process " << name_b (o) << " " << e;
+
+                clean = true;
+              }
+            }
+
+            if (clean)
+              rm_r (out_root);
+          }
           else
-            run_b (o, c, bspec, verb_b::quiet);
+            run_b (o, verb_b::quiet, bspec);
         }
 
         // Make sure the out directory is gone unless it is the same as src,
diff --git a/bpkg/satisfaction.cxx b/bpkg/satisfaction.cxx
index 9211ac0..8a61145 100644
--- a/bpkg/satisfaction.cxx
+++ b/bpkg/satisfaction.cxx
@@ -6,7 +6,6 @@
 
 #include <libbutl/process.mxx>
 
-#include <bpkg/utility.hxx>
 #include <bpkg/package-odb.hxx>
 #include <bpkg/diagnostics.hxx>
 
@@ -100,7 +99,7 @@ namespace bpkg
   static version build2_version;
 
   void
-  satisfy_build2 (const common_options& co,
+  satisfy_build2 (const common_options& o,
                   const string& pkg,
                   const dependency& d)
   {
@@ -110,53 +109,43 @@ namespace bpkg
     //
     if (build2_version.empty ())
     {
-      const char* args[] = {name_b (co), "--version", nullptr};
+      fdpipe pipe (open_pipe ());
 
-      try
-      {
-        process_path pp (process::path_search (args[0], exec_dir));
+      process pr (start_b (o,
+                           pipe, 2 /* stderr */,
+                           verb_b::quiet,
+                           "--version"));
 
-        if (verb >= 3)
-          print_process (args);
+      // Shouldn't throw, unless something is severely damaged.
+      //
+      pipe.out.close ();
 
-        process pr (pp, args, 0, -1); // Redirect STDOUT to pipe.
+      string l;
+      try
+      {
+        ifdstream is (move (pipe.in), fdstream_mode::skip);
+        getline (is, l);
+        is.close ();
 
-        string l;
-        try
+        if (pr.wait () && l.compare (0, 7, "build2 ") == 0)
         {
-          ifdstream is (move (pr.in_ofd), fdstream_mode::skip);
-          getline (is, l);
-          is.close ();
-
-          if (pr.wait () && l.compare (0, 7, "build2 ") == 0)
+          try
           {
-            try
-            {
-              build2_version = version (string (l, 7));
-            }
-            catch (const invalid_argument&) {} // Fall through.
+            build2_version = version (string (l, 7));
           }
-
-          // Fall through.
-        }
-        catch (const io_error&)
-        {
-          pr.wait ();
-          // Fall through.
+          catch (const invalid_argument&) {} // Fall through.
         }
 
-        if (build2_version.empty ())
-          fail << "unable to determine build2 version of " << args[0];
+        // Fall through.
       }
-      catch (const process_error& e)
+      catch (const io_error&)
       {
-        error << "unable to execute " << args[0] << ": " << e;
-
-        if (e.child)
-          exit (1);
-
-        throw failed ();
+        pr.wait ();
+        // Fall through.
       }
+
+      if (build2_version.empty ())
+        fail << "unable to determine build2 version of " << name_b (o);
     }
 
     if (!satisfies (build2_version, d.constraint))
diff --git a/bpkg/types.hxx b/bpkg/types.hxx
index 0433e92..e4e950d 100644
--- a/bpkg/types.hxx
+++ b/bpkg/types.hxx
@@ -23,6 +23,7 @@
 #include <odb/lazy-ptr.hxx>
 
 #include <libbutl/path.mxx>
+#include <libbutl/process.mxx>
 #include <libbutl/utility.mxx>  // compare_reference_target
 #include <libbutl/optional.mxx>
 #include <libbutl/fdstream.mxx>
@@ -91,6 +92,13 @@ namespace bpkg
   using paths = std::vector<path>;
   using dir_paths = std::vector<dir_path>;
 
+  // <libbutl/process.mxx>
+  //
+  using butl::process;
+  using butl::process_path;
+  using butl::process_exit;
+  using butl::process_error;
+
   // <libbutl/fdstream.mxx>
   //
   using butl::auto_fd;
diff --git a/bpkg/utility.cxx b/bpkg/utility.cxx
index 693c07e..a990d32 100644
--- a/bpkg/utility.cxx
+++ b/bpkg/utility.cxx
@@ -277,42 +277,6 @@ namespace bpkg
 
   dir_path exec_dir;
 
-  void
-  run (const char* args[], const dir_path& fallback)
-  {
-    try
-    {
-      process_path pp (process::path_search (args[0], fallback));
-
-      if (verb >= 2)
-        print_process (args);
-
-      process pr (pp, args);
-
-      if (pr.wait ())
-        return;
-
-      assert (pr.exit);
-      const process_exit& pe (*pr.exit);
-
-      if (pe.normal ())
-        throw failed (); // Assume the child issued diagnostics.
-
-      diag_record dr (fail);
-      print_process (dr, args);
-      dr << " " << pe;
-    }
-    catch (const process_error& e)
-    {
-      error << "unable to execute " << args[0] << ": " << e;
-
-      if (e.child)
-        exit (1);
-
-      throw failed ();
-    }
-  }
-
   const char*
   name_b (const common_options& co)
   {
@@ -320,82 +284,4 @@ namespace bpkg
       ? co.build ().string ().c_str ()
       : "b" BPKG_EXE_SUFFIX;
   }
-
-  void
-  run_b (const common_options& co,
-         const dir_path& c,
-         const string& bspec,
-         verb_b v,
-         const strings& pvars,
-         const strings& cvars)
-  {
-    cstrings args {name_b (co)};
-
-    // Map verbosity level. If we are running quiet or at level 1,
-    // then run build2 quiet. Otherwise, run it at the same level
-    // as us.
-    //
-    string vl;
-
-    if (verb == 0)
-      args.push_back ("-q");
-    else if (verb == 1)
-    {
-      if (v != verb_b::normal)
-      {
-        args.push_back ("-q");
-
-        if (v == verb_b::progress && stderr_term)
-          args.push_back ("--progress");
-      }
-    }
-    else if (verb == 2)
-      args.push_back ("-v");
-    else
-    {
-      vl = to_string (verb);
-      args.push_back ("--verbose");
-      args.push_back (vl.c_str ());
-    }
-
-    // Add user options.
-    //
-    for (const string& o: co.build_option ())
-      args.push_back (o.c_str ());
-
-    // Add config vars.
-    //
-    strings storage;
-    storage.reserve (cvars.size ());
-    for (const string& v: cvars)
-    {
-      // Don't scope-qualify global variables.
-      //
-      if (v[0] != '!')
-      {
-        // Use path representation to get canonical trailing slash.
-        //
-        storage.push_back (c.representation () + ':' + v);
-        args.push_back (storage.back ().c_str ());
-      }
-      else
-        args.push_back (v.c_str ());
-    }
-
-    for (const string& v: pvars)
-      args.push_back (v.c_str ());
-
-    // Add buildspec.
-    //
-    args.push_back (bspec.c_str ());
-
-    args.push_back (nullptr);
-
-    // Use our executable directory as a fallback search since normally the
-    // entire toolchain is installed into one directory. This way, for
-    // example, if we installed into /opt/build2 and run bpkg with absolute
-    // path (and without PATH), then bpkg will be able to find "its" b.
-    //
-    run (args, exec_dir);
-  }
 }
diff --git a/bpkg/utility.hxx b/bpkg/utility.hxx
index 85382ef..ae9736d 100644
--- a/bpkg/utility.hxx
+++ b/bpkg/utility.hxx
@@ -19,6 +19,7 @@
 
 #include <bpkg/types.hxx>
 #include <bpkg/version.hxx>
+#include <bpkg/common-options.hxx>
 
 namespace bpkg
 {
@@ -130,34 +131,14 @@ namespace bpkg
   auto_fd
   open_dev_null ();
 
-  // Process.
-  //
-  // By default the process command line is printed for verbosity >= 2
-  // (essential command lines).
-  //
-  // If fallback is specified, then this directory is searched for the
-  // executable as a last resort.
-  //
-  void
-  run (const char* args[], const dir_path& fallback = dir_path ());
-
-  inline void
-  run (cstrings& args, const dir_path& fallback = dir_path ())
-  {
-    run (args.data (), fallback);
-  }
-
   // Directory extracted from argv[0] (i.e., this process' recall directory)
   // or empty if there is none. Can be used as a search fallback.
   //
   extern dir_path exec_dir;
 
   // Run build2, mapping verbosity levels. If quiet is true, then run build2
-  // quiet if our verbosity level is 1. Common vars (cvars) are set on the
-  // configuration scope.
+  // quiet if our verbosity level is 1.
   //
-  class common_options;
-
   const char*
   name_b (const common_options&);
 
@@ -170,13 +151,15 @@ namespace bpkg
     normal    // Run normally (at verbosity 1).
   };
 
+  template <typename O, typename E, typename... A>
+  process
+  start_b (const common_options&, O&& out, E&& err, verb_b, A&&... args);
+
+  template <typename... A>
   void
-  run_b (const common_options&,
-         const dir_path& configuration,
-         const string& buildspec,
-         verb_b = verb_b::normal,
-         const strings& pvars = strings (),
-         const strings& cvars = strings ());
+  run_b (const common_options&, verb_b, A&&... args);
 }
 
+#include <bpkg/utility.txx>
+
 #endif // BPKG_UTILITY_HXX
diff --git a/bpkg/utility.txx b/bpkg/utility.txx
new file mode 100644
index 0000000..f830478
--- /dev/null
+++ b/bpkg/utility.txx
@@ -0,0 +1,98 @@
+// file      : bpkg/utility.txx -*- C++ -*-
+// copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
+// license   : MIT; see accompanying LICENSE file
+
+#include <libbutl/small-vector.mxx>
+
+#include <bpkg/diagnostics.hxx>
+
+namespace bpkg
+{
+  // *_b()
+  //
+  template <typename O, typename E, typename... A>
+  process
+  start_b (const common_options& co,
+           O&& out,
+           E&& err,
+           verb_b v,
+           A&&... args)
+  {
+    const char* b (name_b (co));
+
+    try
+    {
+      // Use our executable directory as a fallback search since normally the
+      // entire toolchain is installed into one directory. This way, for
+      // example, if we installed into /opt/build2 and run bpkg with absolute
+      // path (and without PATH), then bpkg will be able to find "its" b.
+      //
+      process_path pp (process::path_search (b, exec_dir));
+
+      butl::small_vector<const char*, 1> ops;
+
+      // Map verbosity level. If we are running quiet or at level 1,
+      // then run build2 quiet. Otherwise, run it at the same level
+      // as us.
+      //
+      string vl;
+
+      if (verb == 0)
+        ops.push_back ("-q");
+      else if (verb == 1)
+      {
+        if (v != verb_b::normal)
+        {
+          ops.push_back ("-q");
+
+          if (v == verb_b::progress && stderr_term)
+            ops.push_back ("--progress");
+        }
+      }
+      else if (verb == 2)
+        ops.push_back ("-v");
+      else
+      {
+        vl = to_string (verb);
+        ops.push_back ("--verbose");
+        ops.push_back (vl.c_str ());
+      }
+
+      return process_start_callback (
+        [] (const char* const args[], size_t n)
+        {
+          if (verb >= 2)
+            print_process (args, n);
+        },
+        0 /* stdin */,
+        forward<O> (out),
+        forward<E> (err),
+        pp,
+        ops,
+        co.build_option (),
+        forward<A> (args)...);
+    }
+    catch (const process_error& e)
+    {
+      fail << "unable to execute " << b << ": " << e << endf;
+    }
+  }
+
+  template <typename... A>
+  void
+  run_b (const common_options& co, verb_b v, A&&... args)
+  {
+    process pr (
+      start_b (co, 1 /* stdout */, 2 /* stderr */, v, forward<A> (args)...));
+
+    if (!pr.wait ())
+    {
+      const process_exit& e (*pr.exit);
+
+      if (e.normal ())
+        throw failed (); // Assume the child issued diagnostics.
+
+      fail << "process " << name_b (co) << " " << e;
+    }
+  }
+}
diff --git a/tests/pkg-configure.test b/tests/pkg-configure.test
index 3b455ff..f950f68 100644
--- a/tests/pkg-configure.test
+++ b/tests/pkg-configure.test
@@ -383,3 +383,26 @@ if ($cxx.target.class != 'windows')
     $pkg_purge libbar 2>'purged libbar/1.3.0'
   }
 }
+
+: keep-out
+:
+{
+  : fallback
+  :
+  : Test that pkg-disfigure falls back to the external package output directory
+  : removal if the source directory have gone.
+  :
+  {
+    $clone_root_cfg;
+
+    # Configure libhello as an external package.
+    #
+    cp --no-cleanup -r $src/libhello-1.0.0 ./libhello;
+    $pkg_unpack -e ./libhello;
+    $* libhello 2>!;
+
+    rm -r ./libhello;
+    $pkg_disfigure --keep-out libhello 2>'disfigured libhello/1.0.0';
+    test -d cfg/libhello != 0
+  }
+}