Fix loss of user configuration on reconfigure failure (GH issue #359)

4 files changed, 169 insertions, 28 deletions

diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index e02f678..f0e66f2 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -404,6 +404,8 @@ namespace bpkg
                                       ? override
                                       : available);
 
+    const shared_ptr<selected_package>& sp (selected);
+
     assert (!skeleton && ap != nullptr);
 
     package_key pk (db, ap->id.name);
@@ -430,11 +432,12 @@ namespace bpkg
 
     if (ap != nullptr)
     {
-      bool src_conf (selected != nullptr                          &&
-                     selected->state == package_state::configured &&
-                     selected->substate != package_substate::system);
+      bool src_conf (sp != nullptr                          &&
+                     sp->state == package_state::configured &&
+                     sp->substate != package_substate::system);
 
       database& pdb (db);
+      const dir_path& c (pdb.config);
 
       // If the package is being reconfigured, then specify {src,out}_root as
       // the existing source and output root directories not to create the
@@ -447,26 +450,74 @@ namespace bpkg
       // configuration? Yes we can, since load_config_flags stays 0 in this
       // case and all the variables in config.build will be ignored.
       //
-      if (src_conf && ap->version == selected->version)
+      // Note that the failure to configure a package may result in the whole
+      // bunch of packages in the unpacked state but having the build2
+      // configuration, potentially configured by the user (think of a being
+      // reconfigured external dependency package for which `b configure` has
+      // failed, leaving it and all it external dependent packages in the
+      // described state). For such packages we would prefer to preserve the
+      // user configuration rather than to configure them from scratch. Thus,
+      // here we treat them (unpacked_conf is present) similar to packages
+      // configured from source (src_conf is true). (Note that if this change
+      // causes any negative consequences, we could invent a special
+      // unpackged-with-configuration state/substate or some such).
+      //
+      // @@ Also note that regardless whether the packages is in the
+      //    configured or unpacked state, we currently do not preserve the
+      //    build2 user configuration for the package subprojects on the
+      //    package reconfiguration (skeleton just ignores the subprojects but
+      //    we configure passing the config.config.disfigure='config.<pkg>**'
+      //    variable to build2).
+      //
+      optional<dir_path> unpacked_conf;
+
+      if (src_conf && ap->version == sp->version)
       {
-        src_root = selected->effective_src_root (pdb.config);
-        out_root = selected->effective_out_root (pdb.config);
+        src_root = sp->effective_src_root (c);
+        out_root = sp->effective_out_root (c);
       }
-      else
+      else if (sp != nullptr && sp->state == package_state::unpacked)
+      {
+        // Check if the config.build file is present and, if that's the case,
+        // assume this package to be configured. And let's handle all packages
+        // uniformly, regardless if they are external or not.
+        //
+        dir_path d (
+          sp->external ()
+          ? c / dir_path (sp->name.string ())
+          : c / dir_path (sp->name.string () + '-' + sp->version.string ()));
+
+        if (exists (d / alt_config_file) || exists (d / std_config_file))
+        {
+          unpacked_conf = move (d);
+
+          if (ap->version == sp->version)
+          {
+            src_root = sp->effective_src_root (c);
+            out_root = unpacked_conf;
+          }
+        }
+      }
+
+      if (!src_root)
       {
         src_root = external_dir ();
 
         if (src_root)
-          out_root = dir_path (pdb.config) /= name ().string ();
+          out_root = dir_path (c) /= name ().string ();
       }
 
       // Specify old_{src,out}_root paths and set load_config_flags if the old
       // configuration is present and is requested to be loaded.
       //
-      if (src_conf && (!disfigure || load_old_dependent_config))
+      if ((src_conf || unpacked_conf) &&
+          (!disfigure || load_old_dependent_config))
       {
-        old_src_root = selected->effective_src_root (pdb.config);
-        old_out_root = selected->effective_out_root (pdb.config);
+        old_src_root = sp->effective_src_root (c);
+
+        old_out_root = src_conf
+                       ? sp->effective_out_root (c)
+                       : move (unpacked_conf);
 
         if (!disfigure)
           load_config_flags |= package_skeleton::load_config_user;
@@ -483,7 +534,7 @@ namespace bpkg
       move (ap),
       config_vars, // @@ Maybe make optional<strings> and move?
       disfigure,
-      (selected != nullptr ? &selected->config_variables : nullptr),
+      (sp != nullptr ? &sp->config_variables : nullptr),
       move (src_root),
       move (out_root),
       move (old_src_root),
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index eb5b85b..84bd51a 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -632,6 +632,9 @@ namespace bpkg
       // (but not with the % project visibility -- they must still be visible
       // in subprojects).
       //
+      // Also note that we didn't bother adding configuration saving (see
+      // below) for the outproc version.
+      //
 #ifdef BPKG_OUTPROC_CONFIGURE
       // Form the buildspec.
       //
@@ -680,6 +683,46 @@ namespace bpkg
         print_b (o, verb_b::quiet, cpr.config_variables, bspec);
       }
 
+      // If failed to configure the package, we try to revert it to the
+      // original state, as close as possible.
+      //
+      // Specifically, if the (unpacked) package has the build2 configuration
+      // (external package disfigured for reconfiguration during pkg-build,
+      // etc), then we just stash its configuration files before running `b
+      // configure` and restore them on failure. Otherwise, we disfigure the
+      // package on failure. Since the configuration files are unlikely to be
+      // large, let's stash them in memory.
+      //
+      // See disfigure_project() in build2's config module for background.
+      //
+      const small_vector<pair<const path*, const path*>, 2> cfs ({
+        {&std_config_file,   &alt_config_file},
+        {&std_src_root_file, &alt_src_root_file}});
+
+      small_vector<pair<path, string /* content */>, 2> cfg;
+
+      auto stash_cfg = [&cfg, &cfs] (const dir_path& prj_out)
+      {
+        for (const auto& f: cfs)
+        {
+          path cf;
+
+          if (exists (cf = prj_out / *f.second) ||
+              exists (cf = prj_out / *f.first))
+          try
+          {
+            ifdstream ifs (cf);
+            cfg.emplace_back (move (cf), ifs.read_text ());
+          }
+          catch (const io_error& e)
+          {
+            fail << "unable to read from " << cf << ": " << e;
+          }
+        }
+      };
+
+      stash_cfg (out_root);
+
       try
       {
         // Note: no bpkg::failed should be thrown from this block.
@@ -748,6 +791,14 @@ namespace bpkg
                        c.relative (out_root) /* amalgamation */,
                        true                  /* subprojects */);
 
+        // Note: do as early as possible since subsequent actions may fail.
+        //
+        if (const subprojects* ps = *rs.root_extra->subprojects)
+        {
+          for (const auto& p: *ps)
+            stash_cfg (out_root / p.second);
+        }
+
         create_bootstrap_outer (rs, true /* subprojects */);
         bootstrap_post (rs);
 
@@ -848,25 +899,51 @@ namespace bpkg
       {
         // Assume the diagnostics has already been issued.
 
-        // If we failed to configure the package, make sure we revert
-        // it back to the unpacked state by running disfigure (it is
-        // valid to run disfigure on an un-configured build). And if
-        // disfigure fails as well, then the package will be set into
-        // the broken state.
-
-        // Indicate to pkg_disfigure() we are partially configured.
+        // If the build2 configuration is stashed, then restore it and leave
+        // the package in the current (unpacked) state (note: the transaction
+        // will be rolled back when the failed exception is thrown).
+        // Otherwise, revert it back to the unpacked state by running
+        // disfigure (it is valid to run disfigure on an un-configured
+        // build). And if disfigure fails as well, then the package will be
+        // set into the broken state.
         //
-        p->out_root = out_root.leaf ();
-        p->state = package_state::broken;
+        if (!cfg.empty ())
+        {
+          for (const auto& cf: cfg)
+          {
+            const path& f (cf.first);
 
-        // Commits the transaction.
-        //
-        pkg_disfigure (o, db, t,
-                       p,
-                       true /* clean */,
-                       true /* disfigure */,
-                       false /* simulate */);
+            // Make sure the original file directory is present.
+            //
+            mk_p (f.directory ());
 
+            try
+            {
+              ofdstream ofs (f);
+              ofs << cf.second;
+              ofs.close ();
+            }
+            catch (const io_error& e)
+            {
+              fail << "unable to write to " << f << ": " << e;
+            }
+          }
+        }
+        else
+        {
+          // Indicate to pkg_disfigure() we are partially configured.
+          //
+          p->out_root = out_root.leaf ();
+          p->state = package_state::broken;
+
+          // Commits the transaction.
+          //
+          pkg_disfigure (o, db, t,
+                         p,
+                         true /* clean */,
+                         true /* disfigure */,
+                         false /* simulate */);
+        }
 
         throw bpkg::failed ();
       }
diff --git a/bpkg/utility.cxx b/bpkg/utility.cxx
index d084b76..c68cab0 100644
--- a/bpkg/utility.cxx
+++ b/bpkg/utility.cxx
@@ -34,6 +34,9 @@ namespace bpkg
   const dir_path std_config_dir     (dir_path (std_build_dir) /= "config");
   const path     std_bootstrap_file (dir_path (std_build_dir) /= "bootstrap.build");
   const path     std_root_file      (dir_path (std_build_dir) /= "root.build");
+  const path     std_config_file    (dir_path (std_build_dir) /= "config.build");
+  const dir_path std_bootstrap_dir  (dir_path (std_build_dir) /= "bootstrap");
+  const path     std_src_root_file  (dir_path (std_bootstrap_dir) /= "src-root.build");
   const string   std_build_ext      ("build");
 
   // build2:
@@ -42,6 +45,9 @@ namespace bpkg
   const dir_path alt_config_dir     (dir_path (alt_build_dir) /= "config");
   const path     alt_bootstrap_file (dir_path (alt_build_dir) /= "bootstrap.build2");
   const path     alt_root_file      (dir_path (alt_build_dir) /= "root.build2");
+  const path     alt_config_file    (dir_path (alt_build_dir) /= "config.build2");
+  const dir_path alt_bootstrap_dir  (dir_path (alt_build_dir) /= "bootstrap");
+  const path     alt_src_root_file  (dir_path (alt_bootstrap_dir) /= "src-root.build2");
   const string   alt_build_ext      ("build2");
 
   const dir_path current_dir (".");
diff --git a/bpkg/utility.hxx b/bpkg/utility.hxx
index 7a51948..ef04272 100644
--- a/bpkg/utility.hxx
+++ b/bpkg/utility.hxx
@@ -94,12 +94,19 @@ namespace bpkg
   extern const dir_path std_config_dir;     // build/config/
   extern const path     std_bootstrap_file; // build/bootstrap.build
   extern const path     std_root_file;      // build/root.build
+  extern const path     std_config_file;    // build/config.build
+  extern const dir_path std_bootstrap_dir;  // build/bootstrap/
+  extern const path     std_src_root_file;  // build/bootstrap/src-root.build
   extern const string   std_build_ext;      // build
 
+
   extern const dir_path alt_build_dir;      // build2/
   extern const dir_path alt_config_dir;     // build2/config/
   extern const path     alt_bootstrap_file; // build2/bootstrap.build2
   extern const path     alt_root_file;      // build2/root.build2
+  extern const path     alt_config_file;    // build2/config.build2
+  extern const dir_path alt_bootstrap_dir;  // build2/bootstrap/
+  extern const path     alt_src_root_file;  // build2/bootstrap/src-root.build2
   extern const string   alt_build_ext;      // build2
 
   extern const dir_path current_dir;        // ./