Split package configuration into two passes in pkg-build

3 files changed, 219 insertions, 138 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index d30555f..adbcd8f 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -5350,14 +5350,17 @@ namespace bpkg
       return true;
     };
 
-    if (progress)
+    // On the first pass collect all the build_package's to be configured and
+    // calculate their configure_prerequisites_result's.
+    //
+    struct configure_package
     {
-      prog_i = 0;
-      prog_n = static_cast<size_t> (count_if (build_pkgs.begin (),
-                                              build_pkgs.end (),
-                                              configure_pred));
-      prog_percent = 100;
-    }
+      reference_wrapper<build_package> pkg;
+      configure_prerequisites_result   res; // Unused for system package.
+    };
+    vector<configure_package> configure_packages;
+
+    configure_packages.reserve (build_pkgs.size ());
 
     for (build_package& p: reverse_iterate (build_pkgs))
     {
@@ -5369,7 +5372,7 @@ namespace bpkg
       shared_ptr<selected_package>& sp (p.selected);
       const shared_ptr<available_package>& ap (p.available);
 
-      // Configure the package.
+      // Collect the package.
       //
       // At this stage the package is either selected, in which case it's a
       // source code one, or just available, in which case it is a system
@@ -5380,8 +5383,13 @@ namespace bpkg
       //
       assert (sp != nullptr || p.system);
 
-      database& pdb (p.db);
+      if (p.system)
+      {
+        configure_packages.push_back (configure_package {p, {}});
+        continue;
+      }
 
+      database& pdb (p.db);
       transaction t (pdb, !simulate /* start */);
 
       // Show how we got here if things go wrong, for example selecting a
@@ -5401,16 +5409,8 @@ namespace bpkg
         return i != previous_prerequisites.end () ? &i->second : nullptr;
       };
 
-      // Note that pkg_configure() commits the transaction.
-      //
-      if (p.system)
-      {
-        sp = pkg_configure_system (ap->id.name,
-                                   p.available_version (),
-                                   pdb,
-                                   t);
-      }
-      else if (ap != nullptr)
+      configure_prerequisites_result cpr;
+      if (ap != nullptr)
       {
         assert (*p.action == build_package::build);
 
@@ -5437,17 +5437,15 @@ namespace bpkg
         {
           assert (p.skeleton);
 
-          pkg_configure (o,
-                         pdb,
-                         t,
-                         sp,
-                         *p.dependencies,
-                         &*p.alternatives,
-                         move (*p.skeleton),
-                         nullptr /* previous_prerequisites */,
-                         p.disfigure,
-                         simulate,
-                         fdb);
+          cpr = pkg_configure_prerequisites (o,
+                                             pdb,
+                                             t,
+                                             *p.dependencies,
+                                             &*p.alternatives,
+                                             move (*p.skeleton),
+                                             nullptr /* prev_prerequisites */,
+                                             simulate,
+                                             fdb);
         }
         else
         {
@@ -5460,17 +5458,15 @@ namespace bpkg
           if (!p.skeleton)
             p.init_skeleton (o);
 
-          pkg_configure (o,
-                         pdb,
-                         t,
-                         sp,
-                         ap->dependencies,
-                         nullptr /* alternatives */,
-                         move (*p.skeleton),
-                         prereqs (),
-                         p.disfigure,
-                         simulate,
-                         fdb);
+          cpr = pkg_configure_prerequisites (o,
+                                             pdb,
+                                             t,
+                                             ap->dependencies,
+                                             nullptr /* alternatives */,
+                                             move (*p.skeleton),
+                                             prereqs (),
+                                             simulate,
+                                             fdb);
         }
       }
       else // Dependent.
@@ -5479,9 +5475,7 @@ namespace bpkg
         // (otherwise it wouldn't be a dependent) and cannot become system
         // (otherwise it would be a build).
         //
-        assert (*p.action == build_package::adjust &&
-                !p.system &&
-                !sp->system ());
+        assert (*p.action == build_package::adjust && !sp->system ());
 
         // Must be in the unpacked state since it was disfigured on the first
         // pass (see above).
@@ -5512,20 +5506,84 @@ namespace bpkg
         //    build, which can be quite surprising. Should we store this
         //    information in the database?
         //
-        //    I believe this now works for external packages via package
-        //    skeleton (which extracts user configuration).
+        //    Note: this now works for external packages via package skeleton
+        //    (which extracts user configuration).
         //
+        cpr = pkg_configure_prerequisites (o,
+                                           pdb,
+                                           t,
+                                           deps,
+                                           nullptr /* alternatives */,
+                                           move (*p.skeleton),
+                                           prereqs (),
+                                           simulate,
+                                           fdb);
+      }
+
+      configure_packages.push_back (configure_package {p, move (cpr)});
+
+      t.commit ();
+    }
+
+    if (progress)
+    {
+      prog_i = 0;
+      prog_n = configure_packages.size ();
+      prog_percent = 100;
+    }
+
+    for (configure_package& cp: configure_packages)
+    {
+      build_package& p (cp.pkg);
+
+      shared_ptr<selected_package>& sp (p.selected);
+      const shared_ptr<available_package>& ap (p.available);
+
+      // Configure the package.
+      //
+      // NOTE: remember to update the preparation of the plan to be presented
+      // to the user if changing anything here.
+      //
+      database& pdb (p.db);
+      transaction t (pdb, !simulate /* start */);
+
+      // Show how we got here if things go wrong.
+      //
+      auto g (
+        make_exception_guard (
+          [&p] ()
+          {
+            info << "while configuring " << p.name () << p.db;
+          }));
+
+      // Note that pkg_configure*() commits the transaction.
+      //
+      if (p.system)
+      {
+        sp = pkg_configure_system (ap->id.name,
+                                   p.available_version (),
+                                   pdb,
+                                   t);
+      }
+      else if (ap != nullptr)
+      {
+        pkg_configure (o,
+                       pdb,
+                       t,
+                       sp,
+                       move (cp.res),
+                       p.disfigure,
+                       simulate);
+      }
+      else // Dependent.
+      {
         pkg_configure (o,
                        pdb,
                        t,
                        sp,
-                       deps,
-                       nullptr /* alternatives */,
-                       move (*p.skeleton),
-                       prereqs (),
+                       move (cp.res),
                        false /* disfigured */,
-                       simulate,
-                       fdb);
+                       simulate);
       }
 
       r = true;
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index 20a2567..c440877 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -19,32 +19,7 @@ using namespace butl;
 
 namespace bpkg
 {
-  // Given dependencies of a package, return its prerequisite packages,
-  // configuration variables that resulted from selection of these
-  // prerequisites (import, reflection, etc), and sources of the configuration
-  // variables resulted from evaluating the reflect clauses. See
-  // pkg_configure() for the semantics of the dependency list. Fail if for
-  // some of the dependency alternative lists there is no satisfactory
-  // alternative (all its dependencies are configured, satisfy the respective
-  // constraints, etc).
-  //
-  struct configure_prerequisites_result
-  {
-    package_prerequisites   prerequisites;
-    strings                 config_variables; // Note: name and value.
-
-    // Only contains sources of configuration variables collected using the
-    // package skeleton, excluding those user-specified variables which are
-    // not the project variables for the specified package (module
-    // configuration variables, etc). Thus, it is not parallel to the
-    // config_variables member.
-    //
-    vector<config_variable> config_sources; // Note: name and source.
-  };
-
-  // Note: loads selected packages.
-  //
-  static configure_prerequisites_result
+  configure_prerequisites_result
   pkg_configure_prerequisites (const common_options& o,
                                database& db,
                                transaction&,
@@ -55,6 +30,10 @@ namespace bpkg
                                bool simulate,
                                const function<find_database_function>& fdb)
   {
+    tracer trace ("pkg_configure_prerequisites");
+
+    tracer_guard tg (db, trace);
+
     package_prerequisites prereqs;
     strings               vars;
 
@@ -263,6 +242,10 @@ namespace bpkg
                   //    Also note that such modules are marked with `requires:
                   //    bootstrap` in their manifest.
                   //
+                  //    Note that we currently don't support global overrides
+                  //    in the shared build2 context (but could probably do,
+                  //    if necessary).
+                  //
                   dir_path od (sp->effective_out_root (pdb.config));
                   vars.push_back ("config.import." + sp->name.variable () +
                                   "='" + od.representation () + '\'');
@@ -339,13 +322,9 @@ namespace bpkg
                  database& db,
                  transaction& t,
                  const shared_ptr<selected_package>& p,
-                 const dependencies& deps,
-                 const vector<size_t>* alts,
-                 package_skeleton&& ps,
-                 const vector<package_name>* pps,
+                 configure_prerequisites_result&& cpr,
                  bool disfigured,
-                 bool simulate,
-                 const function<find_database_function>& fdb)
+                 bool simulate)
   {
     tracer trace ("pkg_configure");
 
@@ -367,41 +346,13 @@ namespace bpkg
     l4 ([&]{trace << "src_root: " << src_root << ", "
                   << "out_root: " << out_root;});
 
-    // Verify all our prerequisites are configured and populate the
-    // prerequisites list.
-    //
     assert (p->prerequisites.empty ());
-
-    configure_prerequisites_result cpr (
-      pkg_configure_prerequisites (o,
-                                   db,
-                                   t,
-                                   deps,
-                                   alts,
-                                   move (ps),
-                                   pps,
-                                   simulate,
-                                   fdb));
-
     p->prerequisites = move (cpr.prerequisites);
 
+    // Configure.
+    //
     if (!simulate)
     {
-      // Form the buildspec.
-      //
-      string bspec;
-
-      // Use path representation to get canonical trailing slash.
-      //
-      if (src_root == out_root)
-        bspec = "configure('" + out_root.representation () + "')";
-      else
-        bspec = "configure('" +
-          src_root.representation () + "'@'" +
-          out_root.representation () + "')";
-
-      l4 ([&]{trace << "buildspec: " << bspec;});
-
       // Unless this package has been completely disfigured, disfigure all the
       // package configuration variables to reset all the old values to
       // defaults (all the new user/dependent/reflec values, including old
@@ -424,8 +375,21 @@ namespace bpkg
         dvar += "**'";
       }
 
-      // Configure.
+      // Form the buildspec.
+      //
+      string bspec;
+
+      // Use path representation to get canonical trailing slash.
       //
+      if (src_root == out_root)
+        bspec = "configure('" + out_root.representation () + "')";
+      else
+        bspec = "configure('" +
+          src_root.representation () + "'@'" +
+          out_root.representation () + "')";
+
+      l4 ([&]{trace << "buildspec: " << bspec;});
+
       try
       {
         run_b (o,
@@ -436,25 +400,11 @@ namespace bpkg
       }
       catch (const failed&)
       {
-        // 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.
+        // See below for comments.
         //
         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 */);
+        pkg_disfigure (o, db, t, p, true, true, false);
         throw;
       }
 
@@ -468,6 +418,33 @@ namespace bpkg
     t.commit ();
   }
 
+  void
+  pkg_configure (const common_options& o,
+                 database& db,
+                 transaction& t,
+                 const shared_ptr<selected_package>& p,
+                 const dependencies& deps,
+                 const vector<size_t>* alts,
+                 package_skeleton&& ps,
+                 const vector<package_name>* pps,
+                 bool disfigured,
+                 bool simulate,
+                 const function<find_database_function>& fdb)
+  {
+    configure_prerequisites_result cpr (
+      pkg_configure_prerequisites (o,
+                                   db,
+                                   t,
+                                   deps,
+                                   alts,
+                                   move (ps),
+                                   pps,
+                                   simulate,
+                                   fdb));
+
+    pkg_configure (o, db, t, p, move (cpr), disfigured, simulate);
+  }
+
   shared_ptr<selected_package>
   pkg_configure_system (const package_name& n,
                         const version& v,
diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx
index 16ed96f..7587713 100644
--- a/bpkg/pkg-configure.hxx
+++ b/bpkg/pkg-configure.hxx
@@ -21,6 +21,14 @@ namespace bpkg
   int
   pkg_configure (const pkg_configure_options&, cli::scanner& args);
 
+  // Configure a system package and commit the transaction.
+  //
+  shared_ptr<selected_package>
+  pkg_configure_system (const package_name&,
+                        const version&,
+                        database&,
+                        transaction&);
+
   // The custom search function. If specified, it is called by pkg_configure()
   // to obtain the database to search for the prerequisite in, instead of
   // searching for it in the linked databases, recursively. If the function
@@ -58,18 +66,56 @@ namespace bpkg
                  const dependencies&,
                  const vector<size_t>* alternatives,
                  package_skeleton&&,
-                 const vector<package_name>* prerequisites,
+                 const vector<package_name>* prev_prerequisites,
                  bool disfigured,
                  bool simulate,
                  const function<find_database_function>& = {});
 
-  // Configure a system package and commit the transaction.
+
+  // Given dependencies of a package, return its prerequisite packages,
+  // configuration variables that resulted from selection of these
+  // prerequisites (import, reflection, etc), and sources of the configuration
+  // variables resulted from evaluating the reflect clauses. See
+  // pkg_configure() for the semantics of the dependency list. Fail if for
+  // some of the dependency alternative lists there is no satisfactory
+  // alternative (all its dependencies are configured, satisfy the respective
+  // constraints, etc).
   //
-  shared_ptr<selected_package>
-  pkg_configure_system (const package_name&,
-                        const version&,
-                        database&,
-                        transaction&);
+  struct configure_prerequisites_result
+  {
+    package_prerequisites   prerequisites;
+    strings                 config_variables; // Note: name and value.
+
+    // Only contains sources of configuration variables collected using the
+    // package skeleton, excluding those user-specified variables which are
+    // not the project variables for the specified package (module
+    // configuration variables, etc). Thus, it is not parallel to the
+    // config_variables member.
+    //
+    vector<config_variable> config_sources; // Note: name and source.
+  };
+
+  // Note: loads selected packages.
+  //
+  configure_prerequisites_result
+  pkg_configure_prerequisites (const common_options&,
+                               database&,
+                               transaction&,
+                               const dependencies&,
+                               const vector<size_t>* alternatives,
+                               package_skeleton&&,
+                               const vector<package_name>* prev_prerequisites,
+                               bool simulate,
+                               const function<find_database_function>&);
+
+  void
+  pkg_configure (const common_options&,
+                 database&,
+                 transaction&,
+                 const shared_ptr<selected_package>&,
+                 configure_prerequisites_result&&,
+                 bool disfigured,
+                 bool simulate);
 }
 
 #endif // BPKG_PKG_CONFIGURE_HXX