Split package configuration into two passes in pkg-build

3 files changed, 359 insertions, 203 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index d30555f..2484880 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -5350,14 +5350,37 @@ 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 ());
+
+    // Return the "would be" state of packages that would be configured
+    // by this stage.
+    //
+    function<find_package_state_function> configured_state (
+      [&configure_packages] (const shared_ptr<selected_package>& sp)
+      -> optional<pair<package_state, package_substate>>
+      {
+        for (const configure_package& cp: configure_packages)
+        {
+          const build_package& p (cp.pkg);
+
+          if (p.selected == sp)
+            return make_pair (
+              package_state::configured,
+              p.system ? package_substate::system : package_substate::none);
+        }
+
+        return nullopt;
+      });
 
     for (build_package& p: reverse_iterate (build_pkgs))
     {
@@ -5369,7 +5392,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
@@ -5381,7 +5404,6 @@ namespace bpkg
       assert (sp != nullptr || p.system);
 
       database& pdb (p.db);
-
       transaction t (pdb, !simulate /* start */);
 
       // Show how we got here if things go wrong, for example selecting a
@@ -5401,131 +5423,202 @@ namespace bpkg
         return i != previous_prerequisites.end () ? &i->second : nullptr;
       };
 
-      // Note that pkg_configure() commits the transaction.
-      //
+      configure_prerequisites_result cpr;
       if (p.system)
       {
+        // We have no choice but to configure system packages on the first
+        // pass since otherwise there will be no selected package for
+        // pkg_configure_prerequisites() to find. Luckily they have no
+        // dependencies and so can be configured in any order. We will print
+        // their progress/result on the second pass in the proper order.
+        //
+        // Note: commits the transaction.
+        //
         sp = pkg_configure_system (ap->id.name,
                                    p.available_version (),
                                    pdb,
                                    t);
       }
-      else if (ap != nullptr)
+      else
       {
-        assert (*p.action == build_package::build);
-
-        // If the package prerequisites builds are collected, then use the
-        // resulting package skeleton and the pre-selected dependency
-        // alternatives.
-        //
-        // Note that we may not collect the package prerequisites builds if
-        // the package is already configured but we still need to reconfigure
-        // it due, for example, to an upgrade of its dependency. In this case
-        // we pass to pkg_configure() the newly created package skeleton which
-        // contains the package configuration variables specified on the
-        // command line but (naturally) no reflection configuration variables.
-        // Note, however, that in this case pkg_configure() call will evaluate
-        // the reflect clauses itself and so the proper reflection variables
-        // will still end up in the package configuration.
-        //
-        // @@ Note that if we ever allow the user to override the alternative
-        //    selection, this will break (and also if the user re-configures
-        //    the package manually). Maybe that a good reason not to allow
-        //    this? Or we could store this information in the database.
-        //
-        if (p.dependencies)
+        if (ap != nullptr)
         {
-          assert (p.skeleton);
+          assert (*p.action == build_package::build);
 
-          pkg_configure (o,
-                         pdb,
-                         t,
-                         sp,
-                         *p.dependencies,
-                         &*p.alternatives,
-                         move (*p.skeleton),
-                         nullptr /* previous_prerequisites */,
-                         p.disfigure,
-                         simulate,
-                         fdb);
+          // If the package prerequisites builds are collected, then use the
+          // resulting package skeleton and the pre-selected dependency
+          // alternatives.
+          //
+          // Note that we may not collect the package prerequisites builds if
+          // the package is already configured but we still need to
+          // reconfigure it due, for example, to an upgrade of its dependency.
+          // In this case we pass to pkg_configure() the newly created package
+          // skeleton which contains the package configuration variables
+          // specified on the command line but (naturally) no reflection
+          // configuration variables. Note, however, that in this case
+          // pkg_configure() call will evaluate the reflect clauses itself and
+          // so the proper reflection variables will still end up in the
+          // package configuration.
+          //
+          // @@ Note that if we ever allow the user to override the
+          //    alternative selection, this will break (and also if the user
+          //    re-configures the package manually). Maybe that a good reason
+          //    not to allow this? Or we could store this information in the
+          //    database.
+          //
+          if (p.dependencies)
+          {
+            assert (p.skeleton);
+
+            cpr = pkg_configure_prerequisites (o,
+                                               pdb,
+                                               t,
+                                               *p.dependencies,
+                                               &*p.alternatives,
+                                               move (*p.skeleton),
+                                               nullptr /* prev_prerequisites */,
+                                               simulate,
+                                               fdb,
+                                               configured_state);
+          }
+          else
+          {
+            assert (sp != nullptr); // See above.
+
+            // Note that the skeleton can be present if, for example, this is
+            // a dependency which configuration has been negotiated but it is
+            // not collected recursively since it has no buildfile clauses.
+            //
+            if (!p.skeleton)
+              p.init_skeleton (o);
+
+            cpr = pkg_configure_prerequisites (o,
+                                               pdb,
+                                               t,
+                                               ap->dependencies,
+                                               nullptr /* alternatives */,
+                                               move (*p.skeleton),
+                                               prereqs (),
+                                               simulate,
+                                               fdb,
+                                               configured_state);
+          }
         }
-        else
+        else // Dependent.
         {
-          assert (sp != nullptr); // See above.
+          // This is an adjustment of a dependent which cannot be system
+          // (otherwise it wouldn't be a dependent) and cannot become system
+          // (otherwise it would be a build).
+          //
+          assert (*p.action == build_package::adjust && !sp->system ());
 
-          // Note that the skeleton can be present if, for example, this is a
-          // dependency which configuration has been negotiated but it is not
-          // collected recursively since it has no buildfile clauses.
+          // Must be in the unpacked state since it was disfigured on the
+          // first pass (see above).
           //
+          assert (sp->state == package_state::unpacked);
+
+          // Initialize the skeleton if it is not initialized yet.
+          //
+          // Note that the skeleton can only be present here if it was
+          // initialized during the preparation of the plan and so this plan
+          // execution is not simulated (see above for details).
+          //
+          // Also note that there is no available package specified for the
+          // build package object here and so we need to find it (or create a
+          // transient one).
+          //
+          assert (p.available == nullptr && (!p.skeleton || !simulate));
+
           if (!p.skeleton)
-            p.init_skeleton (o);
+            p.init_skeleton (o, find_available (o, pdb, sp));
 
-          pkg_configure (o,
-                         pdb,
-                         t,
-                         sp,
-                         ap->dependencies,
-                         nullptr /* alternatives */,
-                         move (*p.skeleton),
-                         prereqs (),
-                         p.disfigure,
-                         simulate,
-                         fdb);
+          assert (p.skeleton->available != nullptr); // Can't be system.
+
+          const dependencies& deps (p.skeleton->available->dependencies);
+
+          // @@ Note that on reconfiguration the dependent looses the
+          //    potential configuration variables specified by the user on
+          //    some previous build, which can be quite surprising. Should we
+          //    store this information in the database?
+          //
+          //    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,
+                                             configured_state);
         }
+
+        t.commit ();
       }
-      else // Dependent.
-      {
-        // This is an adjustment of a dependent which cannot be system
-        // (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 ());
 
-        // Must be in the unpacked state since it was disfigured on the first
-        // pass (see above).
-        //
-        assert (sp->state == package_state::unpacked);
+      configure_packages.push_back (configure_package {p, move (cpr)});
+    }
 
-        // Initialize the skeleton if it is not initialized yet.
-        //
-        // Note that the skeleton can only be present here if it was
-        // initialized during the preparation of the plan and so this plan
-        // execution is not simulated (see above for details).
-        //
-        // Also note that there is no available package specified for the
-        // build package object here and so we need to find it (or create a
-        // transient one).
-        //
-        assert (p.available == nullptr && (!p.skeleton || !simulate));
+    if (progress)
+    {
+      prog_i = 0;
+      prog_n = configure_packages.size ();
+      prog_percent = 100;
+    }
 
-        if (!p.skeleton)
-          p.init_skeleton (o, find_available (o, pdb, sp));
+    for (configure_package& cp: configure_packages)
+    {
+      build_package& p (cp.pkg);
 
-        assert (p.skeleton->available != nullptr); // Can't be system.
+      const shared_ptr<selected_package>& sp (p.selected);
 
-        const dependencies& deps (p.skeleton->available->dependencies);
+      // Configure the package (system already configured).
+      //
+      // NOTE: remember to update the preparation of the plan to be presented
+      // to the user if changing anything here.
+      //
+      database& pdb (p.db);
+
+      if (!p.system)
+      {
+        const shared_ptr<available_package>& ap (p.available);
+
+        transaction t (pdb, !simulate /* start */);
 
-        // @@ Note that on reconfiguration the dependent looses the potential
-        //    configuration variables specified by the user on some previous
-        //    build, which can be quite surprising. Should we store this
-        //    information in the database?
+        // Show how we got here if things go wrong.
         //
-        //    I believe this now works for external packages via package
-        //    skeleton (which extracts user configuration).
+        auto g (
+          make_exception_guard (
+            [&p] ()
+            {
+              info << "while configuring " << p.name () << p.db;
+            }));
+
+        // Note that pkg_configure() commits the transaction.
         //
-        pkg_configure (o,
-                       pdb,
-                       t,
-                       sp,
-                       deps,
-                       nullptr /* alternatives */,
-                       move (*p.skeleton),
-                       prereqs (),
-                       false /* disfigured */,
-                       simulate,
-                       fdb);
+        if (ap != nullptr)
+        {
+          pkg_configure (o,
+                         pdb,
+                         t,
+                         sp,
+                         move (cp.res),
+                         p.disfigure,
+                         simulate);
+        }
+        else // Dependent.
+        {
+          pkg_configure (o,
+                         pdb,
+                         t,
+                         sp,
+                         move (cp.res),
+                         false /* disfigured */,
+                         simulate);
+        }
       }
 
       r = true;
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index 20a2567..59a6af0 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&,
@@ -53,8 +28,13 @@ namespace bpkg
                                package_skeleton&& ps,
                                const vector<package_name>* prev_prereqs,
                                bool simulate,
-                               const function<find_database_function>& fdb)
+                               const function<find_database_function>& fdb,
+                               const function<find_package_state_function>& fps)
   {
+    tracer trace ("pkg_configure_prerequisites");
+
+    tracer_guard tg (db, trace);
+
     package_prerequisites prereqs;
     strings               vars;
 
@@ -167,9 +147,15 @@ namespace bpkg
 
             const shared_ptr<selected_package>& dp (spd.first);
 
-            if (dp == nullptr                          ||
-                dp->state != package_state::configured ||
-                !satisfies (dp->version, d.constraint) ||
+            if (dp == nullptr)
+              break;
+
+            optional<pair<package_state, package_substate>> dps;
+            if (fps != nullptr)
+              dps = fps (dp);
+
+            if ((dps ? dps->first : dp->state) != package_state::configured ||
+                !satisfies (dp->version, d.constraint)                      ||
                 (pps != nullptr &&
                  find (pps->begin (), pps->end (), dp->name) == pps->end ()))
               break;
@@ -244,7 +230,13 @@ namespace bpkg
               {
                 shared_ptr<selected_package> sp (pr.first.load ());
 
-                if (!sp->system ())
+                optional<pair<package_state, package_substate>> ps;
+                if (fps != nullptr)
+                  ps = fps (sp);
+
+                if (ps
+                    ? ps->second != package_substate::system
+                    : !sp->system ())
                 {
                   // @@ Note that this doesn't work for build2 modules that
                   //    require bootstrap. For their dependents we need to
@@ -263,7 +255,26 @@ namespace bpkg
                   //    Also note that such modules are marked with `requires:
                   //    bootstrap` in their manifest.
                   //
-                  dir_path od (sp->effective_out_root (pdb.config));
+                  //    Note that we currently don't support global overrides
+                  //    in the shared build2 context (but could probably do,
+                  //    if necessary).
+                  //
+
+                  dir_path od;
+                  if (ps)
+                  {
+                    // There is no out_root for a would-be configured package.
+                    // So we calculate it like in pkg_configure() below (yeah,
+                    // it's an ugly hack).
+                    //
+                    od = sp->external ()
+                      ? pdb.config / dir_path (sp->name.string ())
+                      : pdb.config / dir_path (sp->name.string () + '-' +
+                                               sp->version.string ());
+                  }
+                  else
+                    od = sp->effective_out_root (pdb.config);
+
                   vars.push_back ("config.import." + sp->name.variable () +
                                   "='" + od.representation () + '\'');
                 }
@@ -339,13 +350,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");
 
@@ -359,6 +366,8 @@ namespace bpkg
 
     // Calculate package's out_root.
     //
+    // Note: see a version of this in pkg_configure_prerequisites().
+    //
     dir_path out_root (
       p->external ()
       ? c / dir_path (p->name.string ())
@@ -367,41 +376,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 +405,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 +430,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 +448,34 @@ 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,
+                                   nullptr));
+
+    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..a7409b9 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
@@ -31,7 +39,14 @@ namespace bpkg
                                             const package_name&,
                                             bool buildtime);
 
-  // Configure the package, update its state, 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).
   //
   // The package dependency constraints are expected to be complete.
   //
@@ -50,6 +65,54 @@ namespace bpkg
   // dependency decisions" mode). Failed that, select an alternative as if no
   // prerequisites are specified (the "make dependency decisions" mode).
   //
+  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.
+  };
+
+  // Return the "would be" state for packages that would be configured
+  // by this stage.
+  //
+  using find_package_state_function =
+    optional<pair<package_state, package_substate>> (
+      const shared_ptr<selected_package>&);
+
+  // 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>&,
+                               const function<find_package_state_function>&);
+
+  // Configure the package, update its state, and commit the transaction.
+  //
+  void
+  pkg_configure (const common_options&,
+                 database&,
+                 transaction&,
+                 const shared_ptr<selected_package>&,
+                 configure_prerequisites_result&&,
+                 bool disfigured,
+                 bool simulate);
+
+  // Note: loads selected packages.
+  //
   void
   pkg_configure (const common_options&,
                  database&,
@@ -58,18 +121,10 @@ 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.
-  //
-  shared_ptr<selected_package>
-  pkg_configure_system (const package_name&,
-                        const version&,
-                        database&,
-                        transaction&);
 }
 
 #endif // BPKG_PKG_CONFIGURE_HXX