Fix holes in previous commit

3 files changed, 218 insertions, 144 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index adbcd8f..2484880 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -5362,6 +5362,26 @@ namespace bpkg
 
     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))
     {
       assert (p.action);
@@ -5383,12 +5403,6 @@ namespace bpkg
       //
       assert (sp != nullptr || p.system);
 
-      if (p.system)
-      {
-        configure_packages.push_back (configure_package {p, {}});
-        continue;
-      }
-
       database& pdb (p.db);
       transaction t (pdb, !simulate /* start */);
 
@@ -5410,119 +5424,142 @@ namespace bpkg
       };
 
       configure_prerequisites_result cpr;
-      if (ap != nullptr)
+      if (p.system)
       {
-        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.
+        // 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 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.
+        // Note: commits the transaction.
         //
-        if (p.dependencies)
+        sp = pkg_configure_system (ap->id.name,
+                                   p.available_version (),
+                                   pdb,
+                                   t);
+      }
+      else
+      {
+        if (ap != nullptr)
         {
-          assert (p.skeleton);
+          assert (*p.action == build_package::build);
 
-          cpr = pkg_configure_prerequisites (o,
-                                             pdb,
-                                             t,
-                                             *p.dependencies,
-                                             &*p.alternatives,
-                                             move (*p.skeleton),
-                                             nullptr /* prev_prerequisites */,
-                                             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 ());
+
+          // Must be in the unpacked state since it was disfigured on the
+          // first pass (see above).
+          //
+          assert (sp->state == package_state::unpacked);
 
-          // 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.
+          // 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));
+
+          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,
-                                             ap->dependencies,
+                                             deps,
                                              nullptr /* alternatives */,
                                              move (*p.skeleton),
                                              prereqs (),
                                              simulate,
-                                             fdb);
+                                             fdb,
+                                             configured_state);
         }
-      }
-      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 && !sp->system ());
 
-        // 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, find_available (o, pdb, sp));
-
-        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);
+        t.commit ();
       }
 
       configure_packages.push_back (configure_package {p, move (cpr)});
-
-      t.commit ();
     }
 
     if (progress)
@@ -5536,54 +5573,52 @@ namespace bpkg
     {
       build_package& p (cp.pkg);
 
-      shared_ptr<selected_package>& sp (p.selected);
-      const shared_ptr<available_package>& ap (p.available);
+      const shared_ptr<selected_package>& sp (p.selected);
 
-      // Configure the package.
+      // 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);
-      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)
+      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,
-                       move (cp.res),
-                       false /* disfigured */,
-                       simulate);
+        const shared_ptr<available_package>& ap (p.available);
+
+        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 (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 c440877..59a6af0 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -28,7 +28,8 @@ 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");
 
@@ -146,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;
@@ -223,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
@@ -246,7 +259,22 @@ namespace bpkg
                   //    in the shared build2 context (but could probably do,
                   //    if necessary).
                   //
-                  dir_path od (sp->effective_out_root (pdb.config));
+
+                  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 () + '\'');
                 }
@@ -338,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 ())
@@ -440,7 +470,8 @@ namespace bpkg
                                    move (ps),
                                    pps,
                                    simulate,
-                                   fdb));
+                                   fdb,
+                                   nullptr));
 
     pkg_configure (o, db, t, p, move (cpr), disfigured, simulate);
   }
diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx
index 7587713..119da64 100644
--- a/bpkg/pkg-configure.hxx
+++ b/bpkg/pkg-configure.hxx
@@ -95,6 +95,13 @@ namespace bpkg
     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
@@ -106,7 +113,8 @@ namespace bpkg
                                package_skeleton&&,
                                const vector<package_name>* prev_prerequisites,
                                bool simulate,
-                               const function<find_database_function>&);
+                               const function<find_database_function>&,
+                               const function<find_package_state_function>&);
 
   void
   pkg_configure (const common_options&,