Fix pkg-build by ignoring version replacement if it doesn't satisfy dependency constraints

15 files changed, 167 insertions, 59 deletions

diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index c0a592f..1eae206 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -1426,6 +1426,8 @@ namespace bpkg
   {
     using std::swap; // ...and not list::swap().
 
+    using constraint_type = build_package::constraint_type;
+
     tracer trace ("collect_build");
 
     assert (pkg.repository_fragment == nullptr ||
@@ -1452,7 +1454,8 @@ namespace bpkg
     package_key pk (pkg.db, pkg.available->id.name);
 
     // Apply the version replacement, if requested, and indicate that it was
-    // applied.
+    // applied. Ignore the replacement if its version doesn't satisfy the
+    // dependency constraints specified by the caller.
     //
     auto vi (replaced_vers.find (pk));
 
@@ -1463,22 +1466,47 @@ namespace bpkg
 
       replaced_version& v (vi->second);
 
-      v.replaced = true;
-
       if (v.available != nullptr)
       {
-        pkg.available = v.available;
-        pkg.repository_fragment = v.repository_fragment;
-        pkg.system = v.system;
+        const version& rv (v.system
+                           ? *v.available->system_version (pk.db)
+                           : v.available->version);
 
-        l5 ([&]{trace << "replacement: "
-                      << pkg.available_name_version_db ();});
+        bool replace (true);
+        for (const constraint_type& c: pkg.constraints)
+        {
+          if (!satisfies (rv, c.value))
+          {
+            replace = false;
+
+            l5 ([&]{trace << "replacement to " << rv << " is denied since "
+                          << c.dependent << " depends on (" << pk.name << ' '
+                          << c.value << ')';});
+          }
+        }
+
+        if (replace)
+        {
+          v.replaced = true;
+
+          pkg.available = v.available;
+          pkg.repository_fragment = v.repository_fragment;
+          pkg.system = v.system;
+
+          l5 ([&]{trace << "replacement: "
+                        << pkg.available_name_version_db ();});
+        }
       }
       else
       {
+        v.replaced = true;
+
         l5 ([&]{trace << "replacement: drop";});
 
-        assert (pkg.selected != nullptr);
+        // 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);
 
         collect_drop (options, pkg.db, pkg.selected, replaced_vers);
         return nullptr;
@@ -1486,51 +1514,18 @@ namespace bpkg
     }
 
     // Add the version replacement entry, call the verification function if
-    // specified, and throw replace_version, unless the package is in the
-    // unsatisfied dependents list on the dependency side and the version
-    // replacement doesn't satisfy the ignored constraint. In the later case,
-    // report the first encountered ignored (unsatisfied) dependency
-    // constraint and fail.
+    // specified, and throw replace_version.
     //
-    // The thinking for the above failure is as follows: if we add the
-    // replacement entry and throw, then on the next iteration there won't be
-    // any information preserved that this version is actually unsatisfactory
-    // for some dependent and we need to ignore the respective constraint
-    // during simulating the configuration of this dependent and to fail if
-    // the problem doesn't resolve naturally (see unsatisfied_depts for
-    // details). This approach may potentially end up with some failures which
-    // we could potentially avoid if postpone them for some longer. Let's,
-    // however, keep it simple for now and reconsider if it turn out to be an
-    // issue.
+    // Note that this package can potentially be present in the unsatisfied
+    // dependents list on the dependency side with the replacement version
+    // being unsatisfactory for the ignored constraint. In this case, during
+    // the from-scratch re-collection this replacement will be ignored if/when
+    // this package is collected with this constraint specified. But it can
+    // still be applied for some later collect_build() call or potentially
+    // turn out bogus.
     //
-    auto replace_ver = [&pk,
-                        &vpb,
-                        &vi,
-                        &replaced_vers,
-                        &unsatisfied_depts,
-                        &trace,
-                        this] (const build_package& p)
+    auto replace_ver = [&pk, &vpb, &vi, &replaced_vers] (const build_package& p)
     {
-
-      const version& v (p.available_version ());
-
-      for (const unsatisfied_dependent& ud: unsatisfied_depts)
-      {
-        for (const auto& c: ud.ignored_constraints)
-        {
-          if (c.dependency == pk && !satisfies (v, c.constraint))
-          {
-            l5 ([&]{trace << "dependency " << p.available_name_version_db ()
-                          << " is unsatisfactory for dependent "
-                          << ud.dependent << " (" << p.name () << ' '
-                          << c.constraint << "), so the failure can't be "
-                          << "postponed anymore";});
-
-            unsatisfied_depts.diag (*this);
-          }
-        }
-      }
-
       replaced_version rv (p.available, p.repository_fragment, p.system);
 
       if (vi != replaced_vers.end ())
@@ -1601,13 +1596,13 @@ namespace bpkg
         //
         // If neither of the versions is satisfactory, then ignore those
         // unsatisfied constraints which prevent us from picking the package
-        // version which is currently in the map (this way we try to avoid the
-        // postponed failure; see replace_ver() lambda for details).
+        // version which is currently in the map. It feels that the version in
+        // the map is no worse than the other one and we choose it
+        // specifically for the sake of optimization, trying to avoid throwing
+        // the replace_version exception.
         //
         if (p1->available_version () != p2->available_version ())
         {
-          using constraint_type = build_package::constraint_type;
-
           // See if pv's version satisfies pc's constraints, skipping those
           // which are meant to be ignored (ics). Return the pointer to the
           // unsatisfied constraint or NULL if all are satisfied.
@@ -1653,8 +1648,7 @@ namespace bpkg
                 // command line and so p may not be NULL. If that (suddenly)
                 // is not the case, then we will have to ignore the constraint
                 // imposed by the dependent which is not in the map, replace
-                // the version, and as a result fail immediately (see
-                // replace_ver() lambda for details).
+                // the version, and call replace_ver().
                 //
                 if (p == nullptr)
                 {
diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx
index 86878f0..ece3d90 100644
--- a/bpkg/pkg-build-collect.hxx
+++ b/bpkg/pkg-build-collect.hxx
@@ -1236,8 +1236,15 @@ namespace bpkg
     // replaced with the drop. So can be used as bool.
     //
     // Consult replaced_vers for an existing version replacement entry and
-    // follow it, if present, potentially collecting the package drop
-    // instead. Add entry to replaced_vers and throw replace_version if the
+    // 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.
+    //
+    // Add entry to replaced_vers and throw replace_version if the
     // existing version needs to be replaced but the new version cannot be
     // re-collected recursively in-place (see replaced_versions for details).
     //
diff --git a/tests/common/satisfy/libbaz-1.2.0.tar.gz b/tests/common/satisfy/libbaz-1.2.0.tar.gz
new file mode 100644
index 0000000..86b6e25
--- /dev/null
+++ b/tests/common/satisfy/libbaz-1.2.0.tar.gz
diff --git a/tests/common/satisfy/libbaz-2.1.0.tar.gz b/tests/common/satisfy/libbaz-2.1.0.tar.gz
new file mode 100644
index 0000000..7567b65
--- /dev/null
+++ b/tests/common/satisfy/libbaz-2.1.0.tar.gz
diff --git a/tests/common/satisfy/libfoo-3.0.0.tar.gz b/tests/common/satisfy/libfoo-3.0.0.tar.gz
new file mode 100644
index 0000000..55dc602
--- /dev/null
+++ b/tests/common/satisfy/libfoo-3.0.0.tar.gz
diff --git a/tests/common/satisfy/libfox-3.0.0.tar.gz b/tests/common/satisfy/libfox-3.0.0.tar.gz
new file mode 100644
index 0000000..0bc246e
--- /dev/null
+++ b/tests/common/satisfy/libfox-3.0.0.tar.gz
diff --git a/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz b/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz
new file mode 120000
index 0000000..f622e36
--- /dev/null
+++ b/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz
@@ -0,0 +1 @@
+../libbar-0.1.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz b/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz
new file mode 120000
index 0000000..b4a7773
--- /dev/null
+++ b/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz
@@ -0,0 +1 @@
+../libbar-1.2.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz b/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz
new file mode 120000
index 0000000..d43cdcd
--- /dev/null
+++ b/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz
@@ -0,0 +1 @@
+../libbaz-1.2.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz b/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz
new file mode 120000
index 0000000..11cd8c8
--- /dev/null
+++ b/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz
@@ -0,0 +1 @@
+../libbaz-2.1.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz b/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz
new file mode 120000
index 0000000..7678898
--- /dev/null
+++ b/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz
@@ -0,0 +1 @@
+../libfoo-3.0.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz b/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz
new file mode 120000
index 0000000..2aef930
--- /dev/null
+++ b/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz
@@ -0,0 +1 @@
+../libfox-3.0.0.tar.gz
\ No newline at end of file
diff --git a/tests/common/satisfy/t4j/repositories.manifest b/tests/common/satisfy/t4j/repositories.manifest
new file mode 100644
index 0000000..5b70556
--- /dev/null
+++ b/tests/common/satisfy/t4j/repositories.manifest
@@ -0,0 +1 @@
+: 1
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index fe01161..f4c79dc 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -127,6 +127,15 @@
 # |   |-- libbar-0.1.0.tar.gz
 # |   `-- repositories.manifest
 # |
+# |-- t4j
+# |   |-- libbar-0.1.0.tar.gz
+# |   |-- libbar-1.2.0.tar.gz
+# |   |-- libfoo-3.0.0.tar.gz   -> libbar
+# |   |-- 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
+# |   `-- repositories.manifest
+# |
 # |-- t5
 # |   |-- libbar-1.2.0.tar.gz
 # |   |-- libbox-1.2.0.tar.gz
@@ -509,6 +518,7 @@ posix = ($cxx.target.class != 'windows')
   cp -r $src/t4e  $out/t4e  && $rep_create $out/t4e  &$out/t4e/packages.manifest
   cp -r $src/t4f  $out/t4f  && $rep_create $out/t4f  &$out/t4f/packages.manifest
   cp -r $src/t4i  $out/t4i  && $rep_create $out/t4i  &$out/t4i/packages.manifest
+  cp -r $src/t4j  $out/t4j  && $rep_create $out/t4j  &$out/t4j/packages.manifest
   cp -r $src/t5   $out/t5   && $rep_create $out/t5   &$out/t5/packages.manifest
   cp -r $src/t6   $out/t6   && $rep_create $out/t6   &$out/t6/packages.manifest
   cp -r $src/t7a  $out/t7a  && $rep_create $out/t7a  &$out/t7a/packages.manifest
@@ -4016,6 +4026,95 @@ test.arguments += --sys-no-query
       }
     }
 
+    : version-replacement
+    :
+    {
+      +$clone_root_cfg
+      +$rep_add $rep/t4j && $rep_fetch
+
+      : denied
+      :
+      {
+        $clone_cfg;
+
+        $* libbaz libfoo libfox --verbose 5 2>>~%EOE% != 0
+          %.*
+          trace: pkg_build: refine package collection/plan execution from scratch
+          trace: collect_build: add libbaz/2.1.0
+          trace: collect_build: add libfoo/3.0.0
+          trace: collect_build: add libfox/3.0.0
+          trace: collect_build_prerequisites: begin libbaz/2.1.0
+          trace: collect_build_prerequisites: end libbaz/2.1.0
+          trace: collect_build_prerequisites: begin libfoo/3.0.0
+          trace: collect_build: add libbar/1.2.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfoo/3.0.0
+          trace: collect_build_prerequisites: begin libbar/1.2.0
+          trace: collect_build_prerequisites: end libbar/1.2.0
+          trace: collect_build_prerequisites: end libfoo/3.0.0
+          trace: collect_build_prerequisites: begin libfox/3.0.0
+          trace: collect_build: pick libbar/0.1.0 over libbar/1.2.0
+          trace: collect_build: libbar/1.2.0 package version needs to be replaced with libbar/0.1.0
+          trace: pkg_build: collection failed due to package version replacement, retry from scratch
+          trace: pkg_build: refine package collection/plan execution from scratch
+          trace: collect_build: add libbaz/2.1.0
+          trace: collect_build: add libfoo/3.0.0
+          trace: collect_build: add libfox/3.0.0
+          trace: collect_build_prerequisites: begin libbaz/2.1.0
+          trace: collect_build_prerequisites: end libbaz/2.1.0
+          trace: collect_build_prerequisites: begin libfoo/3.0.0
+          trace: collect_build: apply version replacement for libbar/1.2.0
+          trace: collect_build: replacement: libbar/0.1.0
+          trace: collect_build: add libbar/0.1.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/0.1.0 of dependent libfoo/3.0.0
+          trace: collect_build_prerequisites: begin libbar/0.1.0
+          trace: collect_build_prerequisites: end libbar/0.1.0
+          trace: collect_build_prerequisites: end libfoo/3.0.0
+          trace: collect_build_prerequisites: begin libfox/3.0.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/0.1.0 of dependent libfox/3.0.0
+          trace: collect_build: pick libbaz/1.2.0 over libbaz/2.1.0
+          trace: collect_build: libbaz/2.1.0 package version needs to be replaced with libbaz/1.2.0
+          trace: pkg_build: collection failed due to package version replacement, retry from scratch
+          trace: pkg_build: refine package collection/plan execution from scratch
+          trace: collect_build: apply version replacement for libbaz/2.1.0
+          trace: collect_build: replacement: libbaz/1.2.0
+          trace: collect_build: add libbaz/1.2.0
+          trace: collect_build: add libfoo/3.0.0
+          trace: collect_build: add libfox/3.0.0
+          trace: collect_build_prerequisites: begin libbaz/1.2.0
+          trace: collect_build: apply version replacement for libbar/1.2.0
+          trace: collect_build: replacement to 0.1.0 is denied since libbaz/1.2.0 depends on (libbar == 1.2.0)
+          trace: collect_build: add libbar/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: begin libbar/1.2.0
+          trace: collect_build_prerequisites: end libbar/1.2.0
+          trace: collect_build_prerequisites: end libbaz/1.2.0
+          trace: collect_build_prerequisites: begin libfoo/3.0.0
+          trace: collect_build: apply version replacement for libbar/1.2.0
+          trace: collect_build: replacement: libbar/0.1.0
+          trace: collect_build: pick libbar/1.2.0 over libbar/0.1.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfoo/3.0.0
+          trace: collect_build_prerequisites: end libfoo/3.0.0
+          trace: collect_build_prerequisites: begin libfox/3.0.0
+          trace: collect_build: postpone failure for dependent libfox unsatisfied with dependency libbar/1.2.0 (== 0.1.0)
+          trace: collect_build: pick libbar/1.2.0 over libbar/0.1.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/3.0.0
+          trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/3.0.0
+          trace: collect_build_prerequisites: end libfox/3.0.0
+          trace: execute_plan: simulate: yes
+          %.*
+          error: unable to satisfy constraints on package libbar
+            info: libbaz/1.2.0 depends on (libbar == 1.2.0)
+                    libfox/3.0.0 requires (libbaz == 1.2.0)
+            info: libfox/3.0.0 depends on (libbar == 0.1.0)
+            info: available libbar/1.2.0
+            info: available libbar/0.1.0
+            info: while satisfying libfox/3.0.0
+            info: explicitly specify libbar version to manually satisfy both constraints
+          %.*
+          EOE
+      }
+    }
+
     : constraint-resolution
     :
     {
diff --git a/tests/pkg-build/t4j b/tests/pkg-build/t4j
new file mode 120000
index 0000000..3e18229
--- /dev/null
+++ b/tests/pkg-build/t4j
@@ -0,0 +1 @@
+../common/satisfy/t4j
\ No newline at end of file