Implement package drops via plan refinements

3 files changed, 272 insertions, 212 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index ed086ba..59dd2d4 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -394,126 +394,142 @@ namespace bpkg
       //
       if (i != map_.end ())
       {
-        const string& n (i->first);
-
-        // At the end we want p1 to point to the object that we keep
-        // and p2 to the object whose constraints we should copy.
-        //
-        build_package* p1 (&i->second.package);
-        build_package* p2 (&pkg);
+        build_package& bp (i->second.package);
 
         // Can't think of the scenario when this happens.
         //
-        assert (p1->action != build_package::drop);
+        assert (bp.action != build_package::drop);
 
-        if (p1->available_version () != p2->available_version ())
+        // If this is just a dependent reconfiguration then make it the
+        // package build, but propagate the need to reconfigure.
+        //
+        if (bp.action == build_package::reconf)
         {
-          using constraint_type = build_package::constraint_type;
+          bp = move (pkg);
+          bp.reconfigure_ = true;
+        }
+        else
+        {
+          const string& n (i->first);
 
-          // If the versions differ, we have to pick one. Start with the
-          // newest version since if both satisfy, then that's the one we
-          // should prefer. So get the first to try into p1 and the second
-          // to try -- into p2.
+          // At the end we want p1 to point to the object that we keep
+          // and p2 to the object whose constraints we should copy.
           //
-          if (p2->available_version () > p1->available_version ())
-            swap (p1, p2);
+          build_package* p1 (&bp);
+          build_package* p2 (&pkg);
 
-          // See if pv's version satisfies pc's constraints. Return the
-          // pointer to the unsatisfied constraint or NULL if all are
-          // satisfied.
-          //
-          auto test = [] (build_package* pv, build_package* pc)
-            -> const constraint_type*
+          if (p1->available_version () != p2->available_version ())
           {
-            for (const constraint_type& c: pc->constraints)
-              if (!satisfies (pv->available_version (), c.value))
-                return &c;
+            using constraint_type = build_package::constraint_type;
 
-            return nullptr;
-          };
+            // If the versions differ, we have to pick one. Start with the
+            // newest version since if both satisfy, then that's the one we
+            // should prefer. So get the first to try into p1 and the second
+            // to try -- into p2.
+            //
+            if (p2->available_version () > p1->available_version ())
+              swap (p1, p2);
 
-          // First see if p1 satisfies p2's constraints.
-          //
-          if (auto c2 = test (p1, p2))
-          {
-            // If not, try the other way around.
+            // See if pv's version satisfies pc's constraints. Return the
+            // pointer to the unsatisfied constraint or NULL if all are
+            // satisfied.
+            //
+            auto test = [] (build_package* pv, build_package* pc)
+              -> const constraint_type*
+              {
+                for (const constraint_type& c: pc->constraints)
+                  if (!satisfies (pv->available_version (), c.value))
+                    return &c;
+
+                return nullptr;
+              };
+
+            // First see if p1 satisfies p2's constraints.
             //
-            if (auto c1 = test (p2, p1))
+            if (auto c2 = test (p1, p2))
             {
-              const string& d1 (c1->dependent);
-              const string& d2 (c2->dependent);
-
-              fail << "unable to satisfy constraints on package " << n <<
-                info << d1 << " depends on (" << n << " " << c1->value << ")" <<
-                info << d2 << " depends on (" << n << " " << c2->value << ")" <<
-                info << "available " << p1->available_name_version () <<
-                info << "available " << p2->available_name_version () <<
-                info << "explicitly specify " << n << " version to manually "
-                   << "satisfy both constraints";
+              // If not, try the other way around.
+              //
+              if (auto c1 = test (p2, p1))
+              {
+                const string& d1 (c1->dependent);
+                const string& d2 (c2->dependent);
+
+                fail << "unable to satisfy constraints on package " << n <<
+                  info << d1 << " depends on (" << n << " " << c1->value
+                       << ")" <<
+                  info << d2 << " depends on (" << n << " " << c2->value
+                       << ")" <<
+                  info << "available " << p1->available_name_version () <<
+                  info << "available " << p2->available_name_version () <<
+                  info << "explicitly specify " << n << " version to manually "
+                       << "satisfy both constraints";
+              }
+              else
+                swap (p1, p2);
             }
-            else
-              swap (p1, p2);
+
+            l4 ([&]{trace << "pick " << p1->available_name_version ()
+                          << " over " << p2->available_name_version ();});
           }
+          // If versions are the same, then we still need to pick the entry as
+          // one of them can build a package from source while another
+          // configure a system package. We prefer a user-selected entry (if
+          // there is one). If none of them is user-selected we prefer a
+          // source package over a system one. Copy the constraints from the
+          // thrown aways entry to the selected one.
+          //
+          else if (p2->user_selection () ||
+                   (!p1->user_selection () && !p2->system))
+            swap (p1, p2);
 
-          l4 ([&]{trace << "pick " << p1->available_name_version ()
-                        << " over " << p2->available_name_version ();});
-        }
-        // If versions are the same, then we still need to pick the entry as
-        // one of them can build a package from source while another configure
-        // a system package. We prefer a user-selected entry (if there is
-        // one). If none of them is user-selected we prefer a source package
-        // over a system one. Copy the constraints from the thrown aways entry
-        // to the selected one.
-        //
-        else if (p2->user_selection () ||
-                 (!p1->user_selection () && !p2->system))
-          swap (p1, p2);
-
-        // See if we are replacing the object. If not, then we don't
-        // need to collect its prerequisites since that should have
-        // already been done. Remember, p1 points to the object we
-        // want to keep.
-        //
-        bool replace (p1 != &i->second.package);
+          // See if we are replacing the object. If not, then we don't
+          // need to collect its prerequisites since that should have
+          // already been done. Remember, p1 points to the object we
+          // want to keep.
+          //
+          bool replace (p1 != &i->second.package);
 
-        if (replace)
-        {
-          swap (*p1, *p2);
-          swap (p1, p2); // Setup for constraints copying below.
-        }
+          if (replace)
+          {
+            swap (*p1, *p2);
+            swap (p1, p2); // Setup for constraints copying below.
+          }
 
-        p1->constraints.insert (p1->constraints.end (),
-                                make_move_iterator (p2->constraints.begin ()),
-                                make_move_iterator (p2->constraints.end ()));
+          p1->constraints.insert (
+            p1->constraints.end (),
+            make_move_iterator (p2->constraints.begin ()),
+            make_move_iterator (p2->constraints.end ()));
 
-        p1->required_by.insert (p2->required_by.begin (),
-                                p2->required_by.end ());
+          p1->required_by.insert (p2->required_by.begin (),
+                                  p2->required_by.end ());
 
-        // Also copy hold_* flags if they are "stronger".
-        //
-        if (!p1->hold_package ||
-            (p2->hold_package && *p2->hold_package > *p1->hold_package))
-          p1->hold_package = p2->hold_package;
+          // Also copy hold_* flags if they are "stronger".
+          //
+          if (!p1->hold_package ||
+              (p2->hold_package && *p2->hold_package > *p1->hold_package))
+            p1->hold_package = p2->hold_package;
 
-        if (!p1->hold_version ||
-            (p2->hold_version && *p2->hold_version > *p1->hold_version))
-          p1->hold_version = p2->hold_version;
+          if (!p1->hold_version ||
+              (p2->hold_version && *p2->hold_version > *p1->hold_version))
+            p1->hold_version = p2->hold_version;
 
-        // Save the 'keep output directory' flag if specified by the user.
-        //
-        if (p2->user_selection () && p2->keep_out)
-          p1->keep_out = true;
+          // Save the 'keep output directory' flag if specified by the user.
+          //
+          if (p2->user_selection () && p2->keep_out)
+            p1->keep_out = true;
 
-        // Note that we don't copy the build_package::system flag. If it was
-        // set from the command line ("strong system") then we will also have
-        // the '== 0' constraint which means that this build_package object
-        // will never be replaced.
-        //
-        // For other cases ("weak system") we don't want to copy system over
-        // in order not prevent, for example, system to non-system upgrade.
+          // Note that we don't copy the build_package::system flag. If it was
+          // set from the command line ("strong system") then we will also
+          // have the '== 0' constraint which means that this build_package
+          // object will never be replaced.
+          //
+          // For other cases ("weak system") we don't want to copy system over
+          // in order not prevent, for example, system to non-system upgrade.
 
-        if (!replace)
-          return nullptr;
+          if (!replace)
+            return nullptr;
+        }
       }
       else
       {
@@ -536,7 +552,7 @@ namespace bpkg
     void
     collect_dropped (shared_ptr<selected_package> sp)
     {
-      string pn (sp->name);
+      string nm (sp->name);
 
       build_package p {
         build_package::drop,
@@ -551,12 +567,20 @@ namespace bpkg
         {},        // Required by.
         false};    // Reconfigure.
 
-      auto i (map_.find (pn));
+      auto i (map_.find (nm));
 
       if (i != map_.end ())
-        i->second.package = move (p);
+      {
+        build_package& bp (i->second.package);
+
+        // Can't think of the scenario when this happens.
+        //
+        assert (bp.action != build_package::build);
+
+        bp = move (p); // Overwrite the existing (possibly reconf) action.
+      }
       else
-        map_.emplace (move (pn), data_type {end (), move (p)});
+        map_.emplace (move (nm), data_type {end (), move (p)});
     }
 
     // Collect the package prerequisites recursively. But first "prune" this
@@ -883,7 +907,11 @@ namespace bpkg
       const shared_ptr<selected_package>& sp (p.selected);
       const shared_ptr<available_package>& ap (p.available);
 
-      assert (ap != nullptr); // No dependents allowed here.
+      bool build (p.action == build_package::build);
+
+      // Package build must always have the available package associated.
+      //
+      assert (!build || ap != nullptr);
 
       // Unless this package needs something to be before it, add it to
       // the end of the list.
@@ -907,17 +935,21 @@ namespace bpkg
       // map via another way. For example, some can be a part of the initial
       // selection. And in that case we must order things properly.
       //
-      if (p.action == build_package::build && !p.system)
+      // Note that we also need to order prerequisites of the dropped package,
+      // to disfigure it before its prerequisites.
+      //
+      if (!build || !p.system)
       {
         // So here we are going to do things differently depending on
         // whether the package is already configured or not. If it is and
         // not as a system package, then that means we can use its
         // prerequisites list. Otherwise, we use the manifest data.
         //
-        if (sp != nullptr &&
-            sp->state == package_state::configured &&
-            sp->substate != package_substate::system &&
-            sp->version == p.available_version ())
+        bool src_conf (sp != nullptr &&
+                       sp->state == package_state::configured &&
+                       sp->substate != package_substate::system);
+
+        if (src_conf && (!build || sp->version == p.available_version ()))
         {
           for (const auto& p: sp->prerequisites)
           {
@@ -950,6 +982,23 @@ namespace bpkg
 
             update (order (d.name, false));
           }
+
+          // If we end up ordering available package prerequisites, we still
+          // need to order the dropped prerequisites of the selected package
+          // to make sure that it is disfigured before these prerequisites.
+          //
+          if (src_conf)
+          {
+            for (const auto& p: sp->prerequisites)
+            {
+              const string& name (p.first.object_id ());
+              auto i (map_.find (name));
+
+              if (i != map_.end () &&
+                  i->second.package.action == build_package::drop)
+                update (order (name, false));
+            }
+          }
         }
       }
 
@@ -988,7 +1037,7 @@ namespace bpkg
         // Prune if this is not a configured package being up/down-graded
         // or reconfigured.
         //
-        if (p.action == build_package::build && p.reconfigure ())
+        if (p.action != build_package::drop && p.reconfigure ())
           collect_order_dependents (db, i);
       }
     }
@@ -1089,6 +1138,8 @@ namespace bpkg
         {
           build_package& dp (i->second.package);
 
+          // Skip the droped package.
+          //
           if (dp.action == build_package::drop)
             continue;
 
@@ -1098,17 +1149,20 @@ namespace bpkg
 
           // Force reconfiguration in both cases.
           //
-          p.reconf = true;
-
-          if (i->second.position == end ())
+          if (dp.action == build_package::build)
           {
-            // Clean the build_package object up to make sure we don't
-            // inadvertently force up/down-grade.
-            //
-            dp.available = nullptr;
-            dp.repository = nullptr;
+            dp.reconfigure_ = true;
+
+            if (i->second.position == end ())
+            {
+              // Clean the build_package object up to make sure we don't
+              // inadvertently force up/down-grade.
+              //
+              dp.available = nullptr;
+              dp.repository = nullptr;
 
-            i->second.position = insert (pos, dp);
+              i->second.position = insert (pos, dp);
+            }
           }
         }
         else
@@ -1122,7 +1176,7 @@ namespace bpkg
             {
               end (),
               build_package {
-                build_package::build,
+                build_package::reconf,
                 move (dsp),
                 nullptr,
                 nullptr,
@@ -1132,7 +1186,7 @@ namespace bpkg
                 system,
                 false,          // Keep output directory.
                 {n},            // Required by.
-                true}           // Reconfigure.
+                false}          // Reconfigure.
             }).first;
 
           i->second.position = insert (pos, i->second.package);
@@ -1155,6 +1209,9 @@ namespace bpkg
     clear_order ()
     {
       build_package_list::clear ();
+
+      for (auto& p: map_)
+        p.second.position = end ();
     }
 
   private:
@@ -1307,7 +1364,7 @@ namespace bpkg
       set<shared_ptr<repository>> rps;
 
       auto pds (db.query<package_dependent> (
-                  query<package_dependent>::name == sp->name));
+                  query<package_dependent>::name == n));
 
       if (pds.empty ())
       {
@@ -2280,7 +2337,7 @@ namespace bpkg
             db.load<selected_package> (p.name));
 
           if (p.version.empty ())
-            pkgs.collect_dropped (sp);
+            pkgs.collect_dropped (move (sp));
           else
             assert (false); // @@ TMP
         }
@@ -2299,7 +2356,7 @@ namespace bpkg
         // appear (e.g., on the plan) last.
         //
         for (const dep_pkg& p: dep_pkgs)
-          pkgs.order (p.name);
+          pkgs.order (p.name, false);
 
         for (const build_package& p: reverse_iterate (hold_pkgs))
           pkgs.order (p.name ());
@@ -2354,9 +2411,7 @@ namespace bpkg
           if (auto sp = db.find<selected_package> (i->name))
           {
             if (optional<version> v = evaluate_dependency (db, sp))
-            {
               s = (i->version != *v);
-            }
             else
               s = (i->version != sp->version);
           }
@@ -2366,7 +2421,7 @@ namespace bpkg
           if (s)
           {
             scratch = true; // Rebuild the plan from scratch.
-            dep_pkgs.erase (i);
+            i = dep_pkgs.erase (i);
           }
           else
             ++i;
@@ -2378,9 +2433,12 @@ namespace bpkg
           //
           refine = false; // Presumably no more refinments necessary.
 
-          /*
-          for (shared_ptr<selected_package> sp = ...
-            <query all dependency (non-held) packages in the database>)
+          using query = query<selected_package>;
+
+          for (shared_ptr<selected_package> sp:
+                 pointer_result (
+                   db.query<selected_package> (query::state == "configured" &&
+                                               !query::hold_package)))
           {
             if (optional<version> v = evaluate_dependency (db, sp))
             {
@@ -2393,7 +2451,6 @@ namespace bpkg
               refine = true;
             }
           }
-          */
         }
 
         // Rollback the changes to the database and reload the changed
@@ -2472,104 +2529,97 @@ namespace bpkg
 
     if (o.print_only () || !o.yes ())
     {
-      auto add_action = [&o, &plan] (const string& act)
-      {
-        if (o.print_only ())
-          cout << act << endl;
-        else if (verb)
-          // Print indented for better visual separation.
-          //
-          plan += (plan.empty () ? "  " : "\n  ") + act;
-      };
-
       for (const build_package& p: reverse_iterate (pkgs))
       {
-        if (p.action == build_package::drop)
-          continue;
-
         const shared_ptr<selected_package>& sp (p.selected);
 
         string act;
-        string cause;
-        if (p.available == nullptr)
+
+        if (p.action == build_package::drop)
         {
-          // This is a dependent needing reconfiguration.
-          //
-          // This is an implicit reconfiguration which requires the plan to be
-          // printed. Will flag that later when composing the list of
-          // prerequisites.
-          //
-          assert (sp != nullptr && p.reconfigure ());
-          update_dependents = true;
-          act = "reconfigure " + sp->name;
-          cause = "dependent of";
+          act = "drop " + sp->string () + " (unused)";
+          print_plan = true;
         }
         else
         {
-          // Even if we already have this package selected, we have to
-          // make sure it is configured and updated.
-          //
-          if (sp == nullptr)
-            act = p.system ? "configure " : "build ";
-          else if (sp->version == p.available_version ())
+          string cause;
+          if (p.action == build_package::reconf)
           {
-            // If this package is already configured and is not part of the
-            // user selection, then there is nothing we will be explicitly
-            // doing with it (it might still get updated indirectly as part
-            // of the user selection update).
+            // This is a dependent needing reconfiguration.
             //
-            if (!p.reconfigure () &&
-                sp->state == package_state::configured &&
-                !p.user_selection ())
-              continue;
+            // This is an implicit reconfiguration which requires the plan to
+            // be printed. Will flag that later when composing the list of
+            // prerequisites.
+            //
+            assert (sp != nullptr && p.reconfigure ());
+            update_dependents = true;
+            act = "reconfigure " + sp->name;
+            cause = "dependent of";
+          }
+          else
+          {
+            // Even if we already have this package selected, we have to
+            // make sure it is configured and updated.
+            //
+            if (sp == nullptr)
+              act = p.system ? "configure " : "build ";
+            else if (sp->version == p.available_version ())
+            {
+              // If this package is already configured and is not part of the
+              // user selection, then there is nothing we will be explicitly
+              // doing with it (it might still get updated indirectly as part
+              // of the user selection update).
+              //
+              if (!p.reconfigure () &&
+                  sp->state == package_state::configured &&
+                  !p.user_selection ())
+                continue;
 
-            act = p.system
-              ? "reconfigure "
-              : p.reconfigure ()
+              act = p.system
+                ? "reconfigure "
+                : p.reconfigure ()
                 ? "reconfigure/build "
                 : "build ";
+            }
+            else
+            {
+              act = p.system
+                ? "reconfigure "
+                : sp->version < p.available_version ()
+                                ? "upgrade "
+                                : "downgrade ";
+
+              print_plan = true;
+            }
+
+            act += p.available_name_version ();
+            cause = "required by";
           }
-          else
+
+          string rb;
+          if (!p.user_selection ())
           {
-            act = p.system
-              ? "reconfigure "
-              : sp->version < p.available_version ()
-                ? "upgrade "
-                : "downgrade ";
+            for (const string& n: p.required_by)
+              rb += ' ' + n;
+
+            // If not user-selected, then there should be another (implicit)
+            // reason for the action.
+            //
+            assert (!rb.empty ());
 
             print_plan = true;
           }
 
-          act += p.available_name_version ();
-          cause = "required by";
+          if (!rb.empty ())
+            act += " (" + cause + rb + ')';
         }
 
-        string rb;
-        if (!p.user_selection ())
-        {
-          for (const string& n: p.required_by)
-            rb += ' ' + n;
-
-          // If not user-selected, then there should be another (implicit)
-          // reason for the action.
+        if (o.print_only ())
+          cout << act << endl;
+        else if (verb)
+          // Print indented for better visual separation.
           //
-          assert (!rb.empty ());
-
-          print_plan = true;
-        }
-
-        if (!rb.empty ())
-          act += " (" + cause + rb + ')';
-
-        add_action (act);
-      }
-
-      //@@ Merge into previous loop.
-      //
-      for (const build_package& p: reverse_iterate (pkgs))
-      {
-        if (p.action == build_package::drop)
-          add_action ("drop " + p.selected->string () + " (unused)");
+          plan += (plan.empty () ? "  " : "\n  ") + act;
       }
     }
 
@@ -2633,6 +2683,9 @@ namespace bpkg
     //
     for (const build_package& p: reverse_iterate (pkgs))
     {
+      if (p.action == build_package::drop)
+        continue;
+
       const shared_ptr<selected_package>& sp (p.selected);
 
       if (!sp->system () && // System package doesn't need update.
@@ -2647,12 +2700,13 @@ namespace bpkg
     {
       for (const build_package& p: reverse_iterate (pkgs))
       {
+        if (p.action == build_package::drop)
+          continue;
+
         const shared_ptr<selected_package>& sp (p.selected);
 
         if (p.reconfigure () && p.available == nullptr)
-        {
           upkgs.push_back (pkg_command_vars {sp, strings ()});
-        }
       }
     }
 
@@ -2683,7 +2737,7 @@ namespace bpkg
       // We are only interested in configured packages that are either being
       // up/down-graded, need reconfiguration (e.g., dependents), or dropped.
       //
-      if (p.action == build_package::build && !p.reconfigure ())
+      if (p.action != build_package::drop && !p.reconfigure ())
         continue;
 
       shared_ptr<selected_package>& sp (p.selected);
@@ -3027,6 +3081,9 @@ namespace bpkg
     //
     for (const build_package& p: reverse_iterate (build_pkgs))
     {
+      if (p.action == build_package::drop)
+        continue;
+
       const shared_ptr<selected_package>& sp (p.selected);
       assert (sp != nullptr);
 
diff --git a/tests/pkg-build.test b/tests/pkg-build.test
index 8b114cd..18b4813 100644
--- a/tests/pkg-build.test
+++ b/tests/pkg-build.test
@@ -630,7 +630,7 @@ rep_fetch     += -d cfg --auth all --trust-yes 2>!
           info: or explicitly specify package libfoo version to manually satisfy these constraints
         EOE
 
-      $* libfoo/1.1.0 >'build libfoo/1.1.0';
+      $* libfoo/1.1.0 --keep-unused >'build libfoo/1.1.0';
 
       $pkg_disfigure libbar 2>'disfigured libbar/1.1.0';
       $pkg_purge     libbar 2>'purged libbar/1.1.0';
@@ -643,6 +643,8 @@ rep_fetch     += -d cfg --auth all --trust-yes 2>!
   : dependent-reconfiguration
   :
   {
+    test.arguments += --keep-unused
+
     +$clone_root_cfg
 
     +$pkg_fetch -e $src/libfoo-1.0.0.tar.gz && $pkg_unpack libfoo
@@ -1008,12 +1010,12 @@ rep_fetch     += -d cfg --auth all --trust-yes 2>!
 
     $* libbar 2>>~%EOE%;
       disfigured libbar/1.0.0
+      disfigured libfoo/1.0.0
+      purged libfoo/1.0.0
       %.*
       %.*fetched libbar/1.2.0%
       unpacked libbar/1.2.0
       configured libbar/1.2.0
-      disfigured libfoo
-      purged libfoo
       %info: .+dir\{libbar-1.2.0.\} is up to date%
       updated libbar/1.2.0
       EOE
@@ -1134,14 +1136,15 @@ rep_fetch     += -d cfg --auth all --trust-yes 2>!
       EOE
 
     $* libbar/1.2.0 <'y' 2>>~%EOE%;
+        drop libfoo/1.0.0 (unused)
         upgrade libbar/1.2.0
       continue? [Y/n] disfigured libbar/1.0.0
+      disfigured libfoo/1.0.0
+      purged libfoo/1.0.0
       %.*
       %.*fetched libbar/1.2.0%
       unpacked libbar/1.2.0
       configured libbar/1.2.0
-      disfigured libfoo
-      purged libfoo
       %info: .+ is up to date%
       updated libbar/1.2.0
       EOE
diff --git a/tests/pkg-drop.test b/tests/pkg-drop.test
index e10bbe2..438e12d 100644
--- a/tests/pkg-drop.test
+++ b/tests/pkg-drop.test
@@ -405,7 +405,7 @@ $* libfoo/1.0.0 2>>~%EOE% != 0
   {
     $clone_cfg && $pkg_build libbar;
 
-    $* libbar 2>>EOE
+    $* libbar --yes 2>>EOE
       disfigured libbar
       disfigured libfoo
       purged libbar