Always reconfigure dependency if configuration is specified on command line for it (GH issue #354)

5 files changed, 360 insertions, 27 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 89239cc..b430c29 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -403,7 +403,8 @@ namespace bpkg
   using existing_dependencies = vector<package_key>;
 
   static evaluate_result
-  evaluate_dependency (database&,
+  evaluate_dependency (const common_options&,
+                       database&,
                        const shared_ptr<selected_package>&,
                        const optional<version_constraint>& desired,
                        bool desired_sys,
@@ -417,6 +418,7 @@ namespace bpkg
                        const dependent_constraints&,
                        const existing_dependencies&,
                        const deorphaned_dependencies&,
+                       const build_packages&,
                        bool ignore_unsatisfiable);
 
   // If there are no user expectations regarding this dependency, then we give
@@ -427,12 +429,14 @@ namespace bpkg
   // have dependents in the current configurations.
   //
   static optional<evaluate_result>
-  evaluate_dependency (database& db,
+  evaluate_dependency (const common_options& o,
+                       database& db,
                        const shared_ptr<selected_package>& sp,
                        const dependency_packages& deps,
                        bool no_move,
                        const existing_dependencies& existing_deps,
                        const deorphaned_dependencies& deorphaned_deps,
+                       const build_packages& pkgs,
                        bool ignore_unsatisfiable)
   {
     tracer trace ("evaluate_dependency");
@@ -648,7 +652,8 @@ namespace bpkg
       dpt_constrs.emplace_back (ddb, move (p), move (dep.constraint));
     }
 
-    return evaluate_dependency (db,
+    return evaluate_dependency (o,
+                                db,
                                 sp,
                                 dvc,
                                 dsys,
@@ -662,6 +667,7 @@ namespace bpkg
                                 dpt_constrs,
                                 existing_deps,
                                 deorphaned_deps,
+                                pkgs,
                                 ignore_unsatisfiable);
   }
 
@@ -689,7 +695,8 @@ namespace bpkg
   };
 
   static evaluate_result
-  evaluate_dependency (database& db,
+  evaluate_dependency (const common_options& o,
+                       database& db,
                        const shared_ptr<selected_package>& sp,
                        const optional<version_constraint>& dvc,
                        bool dsys,
@@ -703,6 +710,7 @@ namespace bpkg
                        const dependent_constraints& dpt_constrs,
                        const existing_dependencies& existing_deps,
                        const deorphaned_dependencies& deorphaned_deps,
+                       const build_packages& pkgs,
                        bool ignore_unsatisfiable)
   {
     tracer trace ("evaluate_dependency");
@@ -916,6 +924,39 @@ namespace bpkg
         *dov};
     };
 
+    auto build_result = [&ddb, dsys, existing, upgrade] (available&& a)
+    {
+      return evaluate_result {
+        ddb, move (a.first), move (a.second),
+        false /* unused */,
+        dsys,
+        existing,
+        upgrade,
+        nullopt /* orphan */};
+    };
+
+    // Note that if the selected dependency is the best that we can get, we
+    // normally issue the "no change" recommendation. However, if the
+    // configuration variables are specified for this dependency on the
+    // command line, then we issue the "reconfigure" recommendation instead.
+    //
+    // Return true, if the already selected dependency has been specified on
+    // the command line with the configuration variables, but has not yet been
+    // built on this pkg-build run.
+    //
+    auto reconfigure = [&ddb, &dsp, &nm, dsys, &pkgs] ()
+    {
+      assert (dsp != nullptr);
+
+      if (!dsys)
+      {
+        const build_package* p (pkgs.entered_build (ddb, nm));
+        return p != nullptr && !p->action && !p->config_vars.empty ();
+      }
+      else
+        return false;
+    };
+
     for (available& af: afs)
     {
       shared_ptr<available_package>& ap (af.first);
@@ -937,8 +978,16 @@ namespace bpkg
         //
         if (!dsp->system () && !deorphan)
         {
-          l5 ([&]{trace << *dsp << ddb << ": best";});
-          return no_change ();
+          if (reconfigure ())
+          {
+            l5 ([&]{trace << *dsp << ddb << ": reconfigure (best)";});
+            return build_result (find_available_fragment (o, ddb, dsp));
+          }
+          else
+          {
+            l5 ([&]{trace << *dsp << ddb << ": best";});
+            return no_change ();
+          }
         }
 
         // We can not upgrade the package to a stub version, so just skip it.
@@ -1081,20 +1130,24 @@ namespace bpkg
              find (existing_deps.begin (), existing_deps.end (),
                    package_key (ddb, nm)) != existing_deps.end ()))
         {
-          l5 ([&]{trace << *dsp << ddb << ": unchanged";});
-          return no_change ();
+          if (reconfigure ())
+          {
+            l5 ([&]{trace << *dsp << ddb << ": reconfigure";});
+            return build_result (move (af));
+          }
+          else
+          {
+            l5 ([&]{trace << *dsp << ddb << ": unchanged";});
+            return no_change ();
+          }
         }
+        else
+        {
+          l5 ([&]{trace << *sp << db << ": update to "
+                        << package_string (nm, av, dsys) << ddb;});
 
-        l5 ([&]{trace << *sp << db << ": update to "
-                      << package_string (nm, av, dsys) << ddb;});
-
-        return evaluate_result {
-          ddb, move (ap), move (af.second),
-          false /* unused */,
-          dsys,
-          existing,
-          upgrade,
-          nullopt /* orphan */};
+          return build_result (move (af));
+        }
       }
     }
 
@@ -1128,8 +1181,16 @@ namespace bpkg
     {
       assert (!dsys); // Version cannot be empty for the system package.
 
-      l5 ([&]{trace << *dsp << ddb << ": only";});
-      return no_change ();
+      if (reconfigure ())
+      {
+        l5 ([&]{trace << *dsp << ddb << ": reconfigure (only)";});
+        return build_result (find_available_fragment (o, ddb, dsp));
+      }
+      else
+      {
+        l5 ([&]{trace << *dsp << ddb << ": only";});
+        return no_change ();
+      }
     }
 
     // If the version satisfying the desired dependency version constraint is
@@ -1370,11 +1431,13 @@ namespace bpkg
   // evaluate_dependency() function description for details).
   //
   static optional<evaluate_result>
-  evaluate_recursive (database& db,
+  evaluate_recursive (const common_options& o,
+                      database& db,
                       const shared_ptr<selected_package>& sp,
                       const recursive_packages& recs,
                       const existing_dependencies& existing_deps,
                       const deorphaned_dependencies& deorphaned_deps,
+                      const build_packages& pkgs,
                       bool ignore_unsatisfiable,
                       upgrade_dependencies_cache& cache)
   {
@@ -1437,7 +1500,8 @@ namespace bpkg
            find_existing (db, sp->name, nullopt /* version_constraint */));
 
     optional<evaluate_result> r (
-      evaluate_dependency (db,
+      evaluate_dependency (o,
+                           db,
                            sp,
                            nullopt /* desired */,
                            false /* desired_sys */,
@@ -1451,6 +1515,7 @@ namespace bpkg
                            dpt_constrs,
                            existing_deps,
                            deorphaned_deps,
+                           pkgs,
                            ignore_unsatisfiable));
 
     // Translate the "no change" result into nullopt.
@@ -6219,6 +6284,7 @@ namespace bpkg
                          &o,
                          &existing_deps,
                          &deorphaned_deps,
+                         &pkgs,
                          cache = upgrade_dependencies_cache {}] (
                          database& db,
                          const shared_ptr<selected_package>& sp,
@@ -6230,12 +6296,14 @@ namespace bpkg
           // See if there is an optional dependency upgrade recommendation.
           //
           if (!sp->hold_package)
-            r = evaluate_dependency (db,
+            r = evaluate_dependency (o,
+                                     db,
                                      sp,
                                      dep_pkgs,
                                      o.no_move (),
                                      existing_deps,
                                      deorphaned_deps,
+                                     pkgs,
                                      ignore_unsatisfiable);
 
           // If none, then see for the recursive dependency upgrade
@@ -6245,11 +6313,13 @@ namespace bpkg
           // configured as such for a reason.
           //
           if (!r && !sp->system () && !rec_pkgs.empty ())
-            r = evaluate_recursive (db,
+            r = evaluate_recursive (o,
+                                    db,
                                     sp,
                                     rec_pkgs,
                                     existing_deps,
                                     deorphaned_deps,
+                                    pkgs,
                                     ignore_unsatisfiable,
                                     cache);
 
diff --git a/tests/common/dependency-alternatives/t8a/dox-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/dox-1.0.0.tar.gz
new file mode 100644
index 0000000..475f7d6
--- /dev/null
+++ b/tests/common/dependency-alternatives/t8a/dox-1.0.0.tar.gz
diff --git a/tests/common/dependency-alternatives/t8a/foz-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/foz-1.0.0.tar.gz
new file mode 100644
index 0000000..90506c6
--- /dev/null
+++ b/tests/common/dependency-alternatives/t8a/foz-1.0.0.tar.gz
diff --git a/tests/common/dependency-alternatives/t8a/fuz-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/fuz-1.0.0.tar.gz
new file mode 100644
index 0000000..03f8f1a
--- /dev/null
+++ b/tests/common/dependency-alternatives/t8a/fuz-1.0.0.tar.gz
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index 443ce5f..9e25f5f 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -217,6 +217,7 @@
 # |   |                            libbaz
 # |   |-- dix-0.1.0.tar.gz
 # |   |-- dix-1.0.0.tar.gz      -> dax require {config.dax.extras=true}
+# |   |-- dox-1.0.0.tar.gz      -> dax
 # |   |-- dux-1.0.0.tar.gz      -> dix
 # |   |-- fax-1.0.0.tar.gz      -> libbar ^1.0.0 ? ($cxx.target.class == 'windows') config.fax.backend=libbar |
 # |   |                            libbaz ^1.0.0 ? ($cxx.target.class != 'windows') config.fax.backend=libbaz,
@@ -226,6 +227,8 @@
 # |   |-- foo-1.0.0.tar.gz      -> {libbar libbaz} ^1.0.0
 # |   |-- fox-1.0.0.tar.gz      -> libbar ^1.0.0 | libbaz ^1.0.0
 # |   |-- fux-1.0.0.tar.gz      -> libbiz ? (!$config.fux.libbiz_old) | libbiz ^0.1.0 ? ($config.fux.libbiz_old)
+# |   |-- fuz-1.0.0.tar.gz      -> libfoo
+# |   |-- foz-1.0.0.tar.gz      -> fuz
 # |   |-- tax-1.0.0.tar.gz      -> libfoo == 1.0.0 | libfoo == 2.0.0
 # |   |-- tex-1.0.0.tar.gz      -> libfoo prefer{} accept(true) reflect {...}
 # |   |-- tix-1.0.0.tar.gz      -> libfoo >= 2.0.0 reflect {...} | libfoo >= 1.0.0 reflect {...}
@@ -3212,7 +3215,7 @@ test.arguments += --sys-no-query
       : satisfy-dependents
       :
       : Test resolving a conflict when libfix and libbiz have selected such
-      : versions of their dependency libbaz, that do not satisfy each other
+      : versions of their dependency libbaz, that don't satisfy each other
       : constraints. We resolve the conflict automatically as if by specifying
       : ?libbaz/0.0.3 on the command line, which satisfies both constraints.
       :
@@ -3470,6 +3473,266 @@ test.arguments += --sys-no-query
     }
   }
 
+  : reconfigure
+  :
+  {
+    test.arguments += --yes --configure-only
+
+    +$clone_root_cfg
+    +$rep_fetch $rep/t8a
+
+    : deps-with-buildfile-clause
+    :
+    {
+      $clone_cfg;
+
+      $* dox 2>!;
+
+      $pkg_status -r >>EOO;
+        !dox configured 1.0.0
+          dax configured 1.0.0
+            libbaz configured 1.1.0
+        EOO
+
+      $* ?dax; # Noop.
+
+      $* { config.dax.extras=true }+ ?dax 2>>EOE;
+        disfigured dox/1.0.0
+        disfigured dax/1.0.0
+        fetched libbar/1.0.0
+        unpacked libbar/1.0.0
+        configured libbar/1.0.0
+        configured dax/1.0.0
+        configured dox/1.0.0
+        EOE
+
+      cat cfg/dax-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.dax.extras = true
+        %.*
+        EOO
+
+      $* ?dax; # Noop.
+
+      cat cfg/dax-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.dax.extras = true
+        %.*
+        EOO
+
+      $pkg_status -r >>EOO;
+        !dox configured 1.0.0
+          dax configured 1.0.0
+            libbar configured 1.0.0
+            libbaz configured 1.1.0
+        EOO
+
+      $* { config.dax.extras=true }+ ?dax 2>>EOE;
+        disfigured dox/1.0.0
+        disfigured dax/1.0.0
+        configured dax/1.0.0
+        configured dox/1.0.0
+        EOE
+
+      $pkg_status -r >>EOO;
+        !dox configured 1.0.0
+          dax configured 1.0.0
+            libbar configured 1.0.0
+            libbaz configured 1.1.0
+        EOO
+
+      $* { config.dax.extras=false }+ ?dax 2>>EOE;
+        disfigured dox/1.0.0
+        disfigured dax/1.0.0
+        disfigured libbar/1.0.0
+        purged libbar/1.0.0
+        configured dax/1.0.0
+        configured dox/1.0.0
+        EOE
+
+      cat cfg/dax-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.dax.extras = false
+        %.*
+        EOO
+
+      $pkg_status -r >>EOO;
+        !dox configured 1.0.0
+          dax configured 1.0.0
+            libbaz configured 1.1.0
+        EOO
+
+      # While at it, test that an attempt to reconfigure an orphan dependency
+      # which has its own dependencies with buildfile clauses fails.
+      #
+      $rep_remove $rep/t8a;
+
+      $* { config.dax.extras=true }+ ?dax 2>>/EOE != 0;
+        error: unknown package dax
+          info: configuration cfg/ has no repositories
+          info: use 'bpkg rep-add' to add a repository
+        EOE
+
+      cp -rp cfg/dax-1.0.0/ dax;
+
+      $rep_add --type dir "$~/dax";
+      $rep_fetch;
+
+      $* { config.dax.extras=true }+ ?dax 2>>EOE != 0;
+        error: package dax/1.0.0 is orphaned
+          info: explicitly upgrade it to a new version
+        info: while satisfying dax/1.0.0
+        EOE
+
+      $pkg_drop dox
+    }
+
+    : deps-without-buildfile-clause
+    :
+    {
+      $clone_cfg;
+
+      $* foz 2>!;
+
+      $pkg_status -r >>EOO;
+        !foz configured 1.0.0
+          fuz configured 1.0.0
+            libfoo configured 2.0.0
+        EOO
+
+      $* ?fuz; # Noop.
+
+      cat cfg/fuz-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.fuz.extras = false
+        %.*
+        EOO
+
+      $* { config.fuz.extras=true }+ ?fuz 2>>EOE;
+        disfigured foz/1.0.0
+        disfigured fuz/1.0.0
+        configured fuz/1.0.0
+        configured foz/1.0.0
+        EOE
+
+      cat cfg/fuz-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.fuz.extras = true
+        %.*
+        EOO
+
+      $* ?fuz; # Noop.
+
+      cat cfg/fuz-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.fuz.extras = true
+        %.*
+        EOO
+
+      $* { config.fuz.extras=false }+ ?fuz 2>>EOE;
+        disfigured foz/1.0.0
+        disfigured fuz/1.0.0
+        configured fuz/1.0.0
+        configured foz/1.0.0
+        EOE
+
+      cat cfg/fuz-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.fuz.extras = false
+        %.*
+        EOO
+
+      # While at it, test that we can also reconfigure an orphan with its own
+      # dependencies but without buildfile clauses.
+      #
+      $rep_remove $rep/t8a;
+
+      cp -rp cfg/fuz-1.0.0/ fuz;
+      sed -i -e 's/(version:) 1.0.0/\1 2.0.0/' fuz/manifest;
+
+      $rep_add --type dir "$~/fuz";
+      $rep_fetch;
+
+      $* { config.fuz.extras=true }+ ?fuz 2>>EOE;
+        disfigured foz/1.0.0
+        disfigured fuz/1.0.0
+        configured fuz/1.0.0
+        configured foz/1.0.0
+        EOE
+
+      cat cfg/fuz-1.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.fuz.extras = true
+        %.*
+        EOO
+
+      $pkg_status -r >>EOO;
+        !foz configured 1.0.0
+          fuz configured 1.0.0 available 2.0.0
+            libfoo configured 2.0.0
+        EOO
+
+      $pkg_drop foz
+    }
+
+    : no-deps
+    :
+    {
+      $clone_cfg;
+
+      $* fuz 2>!;
+
+      $* ?libfoo; # Noop.
+
+      cat cfg/libfoo-2.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.libfoo.protocol = 2
+        %.*
+        EOO
+
+      $* { config.libfoo.protocol=1 }+ ?libfoo 2>>EOE;
+        disfigured fuz/1.0.0
+        disfigured libfoo/2.0.0
+        configured libfoo/2.0.0
+        configured fuz/1.0.0
+        EOE
+
+      cat cfg/libfoo-2.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.libfoo.protocol = 1
+        %.*
+        EOO
+
+      $* ?libfoo; # Noop.
+
+      # While at it, test that we can also reconfigure an orphan without
+      # dependencies.
+      #
+      $rep_remove $rep/t8a;
+
+      cp -rp cfg/libfoo-2.0.0/ libfoo;
+      sed -i -e 's/(version:) 2.0.0/\1 3.0.0/' libfoo/manifest;
+
+      $rep_add --type dir "$~/libfoo";
+      $rep_fetch;
+
+      $* { config.libfoo.protocol=3 }+ ?libfoo 2>>EOE;
+        disfigured fuz/1.0.0
+        disfigured libfoo/2.0.0
+        configured libfoo/2.0.0
+        configured fuz/1.0.0
+        EOE
+
+      cat cfg/libfoo-2.0.0/build/config.build >>~%EOO%;
+        %.*
+        config.libfoo.protocol = 3
+        %.*
+        EOO
+
+      $pkg_drop fuz
+    }
+  }
+
   : refine
   :
   {
@@ -24132,7 +24395,7 @@ test.arguments += --sys-no-query
               #
               cps =
               for p: $ps
-                timeout 60
+                timeout 120
                 if $* $p 2>&1 | $warn_to_info 2>!
                   cps += $p
                 end
@@ -24142,7 +24405,7 @@ test.arguments += --sys-no-query
               # them into dependencies.
               #
               for p: $cps
-                timeout 60
+                timeout 120
                 $* ?$p 2>!
               end
             end