Fix pkg-build by denying dropping package if it has dependents

8 files changed, 227 insertions, 69 deletions

diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index 1eae206..ff6547f 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -257,6 +257,10 @@ namespace bpkg
     //
     assert ((flags & build_repoint) == 0 || (p.flags & build_repoint) == 0);
 
+    // If true, then add the user-selection tag.
+    //
+    bool add_user_selection (false);
+
     // Copy the user-specified options/variables.
     //
     if (p.user_selection ())
@@ -304,13 +308,31 @@ namespace bpkg
 
       // Propagate the user-selection tag.
       //
-      required_by.emplace (db.get ().main_database (), "command line");
+      add_user_selection = true;
     }
 
-    // Copy the required-by package names only if semantics matches.
+    // Merge in the required-by package names only if semantics matches.
+    // Otherwise, prefer the "required by dependents" semantics since we, in
+    // particular, should never replace such package builds in the map with
+    // package drops (see collect_drop() for details).
     //
     if (p.required_by_dependents == required_by_dependents)
+    {
       required_by.insert (p.required_by.begin (), p.required_by.end ());
+    }
+    else if (p.required_by_dependents)
+    {
+      // Restore the user-selection tag.
+      //
+      if (user_selection ())
+        add_user_selection = true;
+
+      required_by_dependents = true;
+      required_by = move (p.required_by);
+    }
+
+    if (add_user_selection)
+      required_by.emplace (db.get ().main_database (), "command line");
 
     // Copy constraints, suppressing duplicates.
     //
@@ -1455,7 +1477,9 @@ namespace bpkg
 
     // Apply the version replacement, if requested, and indicate that it was
     // applied. Ignore the replacement if its version doesn't satisfy the
-    // dependency constraints specified by the caller.
+    // dependency constraints specified by the caller. Also ignore if this is
+    // a drop and the required-by package names of the specified build package
+    // object have the "required by dependents" semantics
     //
     auto vi (replaced_vers.find (pk));
 
@@ -1499,17 +1523,35 @@ namespace bpkg
       }
       else
       {
-        v.replaced = true;
+        if (!pkg.required_by_dependents)
+        {
+          v.replaced = true;
 
-        l5 ([&]{trace << "replacement: drop";});
+          l5 ([&]{trace << "replacement: drop";});
 
-        // We shouldn't be replacing a package build with the drop if someone
-        // depends on this package.
-        //
-        assert (pkg.selected != nullptr && !pkg.required_by_dependents);
+          // We shouldn't be replacing a package build with the drop if someone
+          // depends on this package.
+          //
+          assert (pkg.selected != nullptr);
+
+          collect_drop (options, pkg.db, pkg.selected, replaced_vers);
+          return nullptr;
+        }
+        else
+        {
+          assert (!pkg.required_by.empty ());
 
-        collect_drop (options, pkg.db, pkg.selected, replaced_vers);
-        return nullptr;
+          l5 ([&]
+              {
+                diag_record dr (trace);
+                dr << "replacement to drop is denied since " << pk
+                   << " is required by ";
+                for (auto b (pkg.required_by.begin ()), i (b);
+                     i != pkg.required_by.end ();
+                     ++i)
+                  dr << (i != b ? ", " : "") << *i;
+              });
+        }
       }
     }
 
@@ -3590,7 +3632,14 @@ namespace bpkg
             if (p == nullptr)
             {
               p = entered_build (dpk);
-              assert (p != nullptr);
+
+              // We don't expect the collected build to be replaced with the
+              // drop since its required-by package names have the "required
+              // by dependents" semantics.
+              //
+              assert (p != nullptr &&
+                      p->action    &&
+                      *p->action == build_package::build);
             }
 
             bool collect_prereqs (!p->recursive_collection);
@@ -5361,58 +5410,78 @@ namespace bpkg
     {
       build_package& bp (i->second.package);
 
-      if (bp.available != nullptr)
+      // Don't overwrite the build object if its required-by package names
+      // have the "required by dependents" semantics.
+      //
+      if (!bp.required_by_dependents)
       {
-        // Similar to the version replacement in collect_build(), see if
-        // in-place drop is possible (no dependencies, etc) and set scratch to
-        // false if that's the case.
-        //
-        bool scratch (true);
+        if (bp.available != nullptr)
+        {
+          // Similar to the version replacement in collect_build(), see if
+          // in-place drop is possible (no dependencies, etc) and set scratch
+          // to false if that's the case.
+          //
+          bool scratch (true);
 
-        // While checking if the package has any dependencies skip the
-        // toolchain build-time dependencies since they should be quite
-        // common.
-        //
-        // An update: it turned out that just absence of dependencies is not
-        // the only condition that causes a package to be dropped in place.
-        // The following conditions must also be met:
-        //
-        // - The package must also not participate in any configuration
-        //   negotiation on the dependency side (otherwise it could have been
-        //   added to a cluster as a dependency).
-        //
-        // - The package must not be added to unsatisfied_depts on the
-        //   dependency side.
-        //
-        // This feels quite hairy at the moment, so we won't be dropping in
-        // place for now.
-        //
+          // While checking if the package has any dependencies skip the
+          // toolchain build-time dependencies since they should be quite
+          // common.
+          //
+          // An update: it turned out that just absence of dependencies is not
+          // the only condition that causes a package to be dropped in place.
+          // The following conditions must also be met:
+          //
+          // - The package must also not participate in any configuration
+          //   negotiation on the dependency side (otherwise it could have
+          //   been added to a cluster as a dependency).
+          //
+          // - The package must not be added to unsatisfied_depts on the
+          //   dependency side.
+          //
+          // This feels quite hairy at the moment, so we won't be dropping in
+          // place for now.
+          //
 #if 0
-        if (!has_dependencies (options, bp.available->dependencies))
-          scratch = false;
+          if (!has_dependencies (options, bp.available->dependencies))
+            scratch = false;
 #endif
 
-        l5 ([&]{trace << bp.available_name_version_db ()
-                      << " package version needs to be replaced "
-                      << (!scratch ? "in-place " : "") << "with drop";});
+          l5 ([&]{trace << bp.available_name_version_db ()
+                        << " package version needs to be replaced "
+                        << (!scratch ? "in-place " : "") << "with drop";});
 
-        if (scratch)
-        {
-          if (vi != replaced_vers.end ())
-            vi->second = replaced_version ();
-          else
-            replaced_vers.emplace (move (pk), replaced_version ());
+          if (scratch)
+          {
+            if (vi != replaced_vers.end ())
+              vi->second = replaced_version ();
+            else
+              replaced_vers.emplace (move (pk), replaced_version ());
 
-          throw replace_version ();
+            throw replace_version ();
+          }
         }
-      }
 
-      // Overwrite the existing (possibly pre-entered, adjustment, or repoint)
-      // entry.
-      //
-      l4 ([&]{trace << "overwrite " << pk;});
+        // Overwrite the existing (possibly pre-entered, adjustment, or
+        // repoint) entry.
+        //
+        l4 ([&]{trace << "overwrite " << pk;});
 
-      bp = move (p);
+        bp = move (p);
+      }
+      else
+      {
+        assert (!bp.required_by.empty ());
+
+        l5 ([&]
+            {
+              diag_record dr (trace);
+              dr << pk << " cannot be dropped since it is required by ";
+              for (auto b (bp.required_by.begin ()), i (b);
+                   i != bp.required_by.end ();
+                   ++i)
+                dr << (i != b ? ", " : "") << *i;
+            });
+      }
     }
     else
     {
diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx
index ece3d90..122ed22 100644
--- a/bpkg/pkg-build-collect.hxx
+++ b/bpkg/pkg-build-collect.hxx
@@ -1237,12 +1237,13 @@ namespace bpkg
     //
     // Consult replaced_vers for an existing version replacement entry and
     // follow it, if present, potentially collecting the package drop instead.
-    // Ignore the entry if its version doesn't satisfy the dependency
-    // constraints specified by the caller. In this case it's likely that this
-    // replacement will be applied for some later collect_build() call but can
-    // potentially turn out bogus. Note that a version replacement for a
-    // specific package may only be applied once during the collection
-    // iteration.
+    // Ignore the entry if its version doesn't satisfy the specified
+    // dependency constraints or the entry is a package drop and the specified
+    // required-by package names have the "required by dependents" semantics.
+    // In this case it's likely that this replacement will be applied for some
+    // later collect_build() call but can potentially turn out bogus. Note
+    // that a version replacement for a specific package may only be applied
+    // once during the collection iteration.
     //
     // Add entry to replaced_vers and throw replace_version if the
     // existing version needs to be replaced but the new version cannot be
@@ -1544,7 +1545,9 @@ namespace bpkg
                                   const function<find_database_function>&,
                                   const function<add_priv_cfg_function>&);
 
-    // Collect the package being dropped.
+    // Collect the package being dropped. Noop if the specified package is
+    // already being built and its required-by package names have the
+    // "required by dependents" semantics.
     //
     // Add entry to replaced_vers and throw replace_version if the existing
     // version needs to be dropped but this can't be done in-place (see
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index c1dd312..51e1fb1 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -4858,6 +4858,10 @@ namespace bpkg
             ++i;
         }
 
+        if (scratch_exe)
+          l5 ([&]{trace << "one of dependency evaluation decisions has "
+                        << "changed, re-collecting from scratch";});
+
         // If the execute_plan() call was noop, there are no user expectations
         // regarding any dependency, and no upgrade is requested, then the
         // only possible refinement outcome can be recommendations to drop
diff --git a/tests/common/satisfy/libfox-2.1.0.tar.gz b/tests/common/satisfy/libfox-2.1.0.tar.gz
new file mode 100644
index 0000000..60a4cce
--- /dev/null
+++ b/tests/common/satisfy/libfox-2.1.0.tar.gz
diff --git a/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz b/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz
new file mode 120000
index 0000000..aad4c49
--- /dev/null
+++ b/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz
@@ -0,0 +1 @@
+../libfix-1.0.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz b/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz
new file mode 120000
index 0000000..674ac04
--- /dev/null
+++ b/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz
@@ -0,0 +1 @@
+../libfox-0.0.1.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz b/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz
new file mode 120000
index 0000000..157a046
--- /dev/null
+++ b/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz
@@ -0,0 +1 @@
+../libfox-2.1.0.tar.gz
\ No newline at end of file
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index f4c79dc..bc435f2 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -131,6 +131,8 @@
 # |   |-- libbar-0.1.0.tar.gz
 # |   |-- libbar-1.2.0.tar.gz
 # |   |-- libfoo-3.0.0.tar.gz   -> libbar
+# |   |-- libfox-0.0.1.tar.gz
+# |   |-- libfox-2.1.0.tar.gz   -> libbar, libbaz == 1.2.0
 # |   |-- libfox-3.0.0.tar.gz   -> libbar == 0.1.0, libbaz == 1.2.0
 # |   |-- libbaz-1.2.0.tar.gz   -> libbar == 1.2.0
 # |   |-- libbaz-2.1.0.tar.gz
@@ -4026,13 +4028,13 @@ test.arguments += --sys-no-query
       }
     }
 
-    : version-replacement
+    : denied-version-replacements
     :
     {
       +$clone_root_cfg
       +$rep_add $rep/t4j && $rep_fetch
 
-      : denied
+      : unsatisfactory-version
       :
       {
         $clone_cfg;
@@ -4113,6 +4115,82 @@ test.arguments += --sys-no-query
           %.*
           EOE
       }
+
+      : collect-drop
+      :
+      {
+        $clone_cfg;
+
+        $* libfix ?libfox/0.0.1 libbaz 2>!;
+
+        $pkg_status -ar >>EOO;
+          libfox configured !0.0.1 available 3.0.0 2.1.0
+          !libfix configured 1.0.0
+            libfox configured !0.0.1 available 3.0.0 2.1.0
+          !libbaz configured 2.1.0
+          EOO
+
+        $* ?libbaz ?libfox/2.1.0 --verbose 5 2>>~%EOE%;
+          %.*
+          trace: pkg_build: refine package collection/plan execution from scratch
+          trace: execute_plan: simulate: yes
+          trace: evaluate_dependency: libfox/0.0.1: update to libfox/2.1.0
+          trace: evaluate_dependency: libbaz/2.1.0: unused
+          trace: pkg_build: refine package collection/plan execution
+          trace: collect_build_prerequisites: pre-reeval libfix/1.0.0
+          trace: collect_build_prerequisites: pre-reevaluated libfix/1.0.0: end reached
+          trace: collect_build_prerequisites: begin libfox/2.1.0
+          trace: collect_build: add libbar/1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/2.1.0
+          trace: collect_build_prerequisites: begin libbar/1.2.0
+          trace: collect_build_prerequisites: end libbar/1.2.0
+          warning: package libfox dependency on (libbaz == 1.2.0) is forcing downgrade of libbaz/2.1.0 to 1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/2.1.0
+          trace: collect_build_prerequisites: begin libbaz/1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libbaz/1.2.0
+          trace: collect_build_prerequisites: end libbaz/1.2.0
+          trace: collect_build_prerequisites: end libfox/2.1.0
+          trace: collect_drop: libbaz cannot be dropped since it is required by command line, libfox/2.1.0
+          trace: execute_plan: simulate: yes
+          %.*
+          trace: pkg_build: one of dependency evaluation decisions has changed, re-collecting from scratch
+          trace: pkg_build: refine package collection/plan execution from scratch
+          trace: collect_build_prerequisites: pre-reeval libfix/1.0.0
+          trace: collect_build_prerequisites: pre-reevaluated libfix/1.0.0: end reached
+          trace: collect_build_prerequisites: begin libfox/2.1.0
+          trace: collect_build: add libbar/1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/2.1.0
+          trace: collect_build_prerequisites: begin libbar/1.2.0
+          trace: collect_build_prerequisites: end libbar/1.2.0
+          warning: package libfox dependency on (libbaz == 1.2.0) is forcing downgrade of libbaz/2.1.0 to 1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/2.1.0
+          trace: collect_build_prerequisites: begin libbaz/1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libbaz/1.2.0
+          trace: collect_build_prerequisites: end libbaz/1.2.0
+          trace: collect_build_prerequisites: end libfox/2.1.0
+          trace: execute_plan: simulate: yes
+          %.*
+          trace: execute_plan: simulate: no
+          %.*
+          EOE
+
+        $pkg_status -ar >>EOO;
+          libfox configured !2.1.0 available 3.0.0
+            libbar configured 1.2.0
+            libbaz configured 1.2.0 available 2.1.0
+              libbar configured 1.2.0
+          !libfix configured 1.0.0
+            libfox configured !2.1.0 available 3.0.0
+              libbar configured 1.2.0
+              libbaz configured 1.2.0 available 2.1.0
+                libbar configured 1.2.0
+          libbaz configured 1.2.0 available 2.1.0
+            libbar configured 1.2.0
+          libbar configured 1.2.0
+          EOO
+
+        $pkg_drop libfix
+      }
     }
 
     : constraint-resolution
@@ -5632,7 +5710,7 @@ test.arguments += --sys-no-query
           %  new libbaz/1.1.0 \[cfg2.\]%
             drop libbaz/1.1.0 (unused)
             new libbar/1.0.0 (required by dax)
-          %  reconfigure/update dax/1.0.0 \(dependent of libbaz \[cfg2.\]\)%
+            reconfigure/update dax/1.0.0 (required by dix)
               config.dax.extras=true (set by dix)
             upgrade dix/1.0.0
             reconfigure dux (dependent of dix)
@@ -7507,6 +7585,7 @@ test.arguments += --sys-no-query
             trace: execute_plan: simulate: yes
             %.*
             trace: evaluate_dependency: fux/1.0.0: unused
+            trace: pkg_build: one of dependency evaluation decisions has changed, re-collecting from scratch
             trace: pkg_build: refine package collection/plan execution from scratch
             trace: collect_build: add libfoo/0.1.0
             trace: collect_build_prerequisites: skip configured libfoo/0.1.0
@@ -14831,7 +14910,7 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libfoo/1.0.0
             config.libfoo.extras=true (set by tex)
-          reconfigure/update tex/1.0.0 (dependent of libbar, libfoo)
+          reconfigure/update tex/1.0.0 (required by tix)
             config.tex.extras=true (set by tix)
             config.tex.libfoo_extras=true (set by tex)
           reconfigure/update tix/1.0.0
@@ -15005,7 +15084,7 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libfoo/1.0.0
             config.libfoo.extras=true (set by tex)
-          reconfigure/update tex/1.0.0 (dependent of libbar, libfoo)
+          reconfigure/update tex/1.0.0 (required by tiz)
             config.tex.extras=true (set by tiz)
             config.tex.libfoo_extras=true (set by tex)
           reconfigure/update tiz/1.0.0
@@ -15833,7 +15912,7 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libbar/1.0.0
             config.libbar.extras=true (set by diz)
-          reconfigure/update bar/1.0.0 (dependent of libbar)
+          reconfigure/update bar/1.0.0 (required by dex)
             config.bar.extras=true (set by dex)
           reconfigure/update dex/1.0.0 (required by dox)
             config.dex.extras=true (set by dox)
@@ -16198,7 +16277,7 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libbar/1.0.0
             config.libbar.extras=true (set by diz)
-          reconfigure/update bar/1.0.0 (dependent of libbar)
+          reconfigure/update bar/1.0.0 (required by dex)
             config.bar.extras=true (set by dex)
           reconfigure/update dex/1.0.0
             config.dex.extras=true (set by dox)
@@ -16359,7 +16438,7 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libbar/1.0.0
             config.libbar.extras=true (set by tex)
-          reconfigure/update tex/1.0.0 (dependent of libbar)
+          reconfigure/update tex/1.0.0 (required by tiz)
             config.tex.extras=true (set by tiz)
             config.tex.libfoo_extras=true (set by tex)
           reconfigure/update tiz/1.0.0