Don't reconfigure dependency if negotiated configuration doesn't change

8 files changed, 164 insertions, 55 deletions

diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index a3cdcdc..567f2e7 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -1943,6 +1943,8 @@ namespace bpkg
   pair<strings, vector<config_variable>> package_skeleton::
   collect_config () &&
   {
+    // NOTE: remember to update config_checksum() if changing anything here.
+
     assert (db_ != nullptr); // Must be called only once.
 
     using build2::config::variable_origin;
@@ -1985,11 +1987,19 @@ namespace bpkg
         // variables which are project variables (i.e., names start with
         // config.<project>).
         //
+        size_t pn (var_prefix_.size ());
         for (const string& v: config_vars_)
         {
-          if (project_override (v, var_prefix_))
+          size_t vn;
+          if (project_override (v, var_prefix_, &vn))
           {
-            string n (var_name (v));
+            // Skip config.<project>.develop (can potentially be passed by
+            // bdep-init) if the package doesn't use it.
+            //
+            if (!develop_ && v.compare (pn, vn - pn, ".develop") == 0)
+              continue;
+
+            string n (v, 0, vn);
 
             // Check for a duplicate.
             //
@@ -2055,6 +2065,54 @@ namespace bpkg
     return make_pair (move (vars), move (srcs));
   }
 
+  string package_skeleton::
+  config_checksum ()
+  {
+    // Note: this is parallel to collect_config() logic but is not destructive.
+
+    assert (db_ != nullptr); // Must be called before collect_config().
+
+    if (!loaded_old_config_)
+      load_old_config ();
+
+    sha256 cs;
+
+    if (!config_vars_.empty ())
+    {
+      cstrings vs;
+      size_t pn (var_prefix_.size ());
+      for (const string& v: config_vars_)
+      {
+        size_t vn;
+        if (project_override (v, var_prefix_, &vn))
+        {
+          // Skip config.<project>.develop (can potentially be passed by
+          // bdep-init) if the package doesn't use it.
+          //
+          if (develop_ || v.compare (pn, vn - pn, ".develop") != 0)
+            cs.append (v);
+        }
+      }
+    }
+
+    if (!dependent_vars_.empty ())
+    {
+      for (const string& v: dependent_vars_)
+        cs.append (v);
+    }
+
+    if (!reflect_.empty ())
+    {
+      for (const reflect_variable_value& v: reflect_)
+      {
+        if (v.origin != build2::config::variable_origin::override_)
+          cs.append (serialize_cmdline (v.name, v.value));
+      }
+    }
+
+    return !cs.empty () ? cs.string () : string ();
+  }
+
   const strings& package_skeleton::
   merge_cmd_vars (const strings& dependent_vars,
                   const strings& dependency_vars,
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index b2ed752..bc5d25c 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -82,6 +82,7 @@ namespace bpkg
     // ? dependent_config()
     // * evaluate_*()
     // * empty() | print_config()
+    // * config_checksum()
     //   collect_config()
     //
     // Note that a copy of the skeleton is expected to continue with the
@@ -181,6 +182,12 @@ namespace bpkg
     pair<strings, vector<config_variable>>
     collect_config () &&;
 
+    // Return the checksum of the project configuration variables that will be
+    // returned by the collect_config() function call.
+    //
+    string
+    config_checksum ();
+
     // Implementation details.
     //
   public:
diff --git a/bpkg/package.hxx b/bpkg/package.hxx
index 060f13a..02d2b07 100644
--- a/bpkg/package.hxx
+++ b/bpkg/package.hxx
@@ -26,9 +26,13 @@
 
 // Used by the data migration entries.
 //
+// NOTE: drop all the `#pragma db member(...) default(...)` pragmas when
+//       migration is no longer supported (i.e., the current and base schema
+//       versions are the same).
+//
 #define DB_SCHEMA_VERSION_BASE 12
 
-#pragma db model version(DB_SCHEMA_VERSION_BASE, 23, closed)
+#pragma db model version(DB_SCHEMA_VERSION_BASE, 24, closed)
 
 namespace bpkg
 {
@@ -618,9 +622,6 @@ namespace bpkg
   //
   #pragma db value(test_dependency) definition
 
-  // @@ TMP Drop when database migration to the schema version 11 is no longer
-  //    supported.
-  //
   #pragma db member(test_dependency::buildtime) default(false)
 
   using optional_test_dependency_type = optional<test_dependency_type>;
@@ -887,29 +888,21 @@ namespace bpkg
 
     // alt_naming
     //
-    // @@ TMP Drop when database migration to the schema version 20 is no
-    //    longer supported.
-    //
-    //    Note that since no real packages with alternative buildfile naming
-    //    use conditional dependencies yet, we can just set alt_naming to
-    //    false during migration to the database schema version 20. Also we
-    //    never rely on alt_naming to be nullopt for the stub packages, so
-    //    let's not complicate things and set alt_naming to false for them
-    //    either.
+    // Note that since no real packages with alternative buildfile naming use
+    // conditional dependencies yet, we can just set alt_naming to false
+    // during migration to the database schema version 20. Also we never rely
+    // on alt_naming to be nullopt for the stub packages, so let's not
+    // complicate things and set alt_naming to false for them either.
     //
     #pragma db member(alt_naming) default(false)
 
     // *_build
     //
-    // @@ TMP Drop when database migration to the schema version 15 is no
-    //    longer supported.
-    //
-    //    Note that since no real packages use conditional dependencies yet,
-    //    we can just set bootstrap_build to the empty string during migration
-    //    to the database schema version 15. Also we never rely on
-    //    bootstrap_build to be nullopt for the stub packages, so let's not
-    //    complicate things and set bootstrap_build to the empty string for
-    //    them either.
+    // Note that since no real packages use conditional dependencies yet, we
+    // can just set bootstrap_build to the empty string during migration to
+    // the database schema version 15. Also we never rely on bootstrap_build
+    // to be nullopt for the stub packages, so let's not complicate things and
+    // set bootstrap_build to the empty string for them either.
     //
     #pragma db member(bootstrap_build) default("")
 
@@ -1272,8 +1265,15 @@ namespace bpkg
 
     package_prerequisites prerequisites;
 
+    // Project configuration variable names and their sources.
+    //
     vector<config_variable> config_variables;
 
+    // SHA256 checksum of variables (names and values) referred to by the
+    // config_variables member.
+    //
+    std::string config_checksum;
+
   public:
     bool
     system () const
@@ -1353,6 +1353,14 @@ namespace bpkg
 
     #pragma db member(config_variables) id_column("package") value_column("")
 
+    // For the sake of simplicity let's not calculate the checksum during
+    // migration. It seems that the only drawback of this approach is a
+    // (single) spurious reconfiguration of a dependency of a dependent with
+    // configuration clause previously configured by bpkg with the database
+    // schema version prior to 24.
+    //
+    #pragma db member(config_checksum) default("")
+
     // Explicit aggregate initialization for C++20 (private default ctor).
     //
     selected_package (package_name n,
diff --git a/bpkg/package.xml b/bpkg/package.xml
index fd332bc..343f27a 100644
--- a/bpkg/package.xml
+++ b/bpkg/package.xml
@@ -1,4 +1,10 @@
 <changelog xmlns="http://www.codesynthesis.com/xmlns/odb/changelog" database="sqlite" version="1">
+  <changeset version="24">
+    <alter-table name="main.selected_package">
+      <add-column name="config_checksum" type="TEXT" null="true" default="''"/>
+    </alter-table>
+  </changeset>
+
   <changeset version="23">
     <alter-table name="main.available_package">
       <add-column name="type" type="TEXT" null="true"/>
diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index 2e7cc42..c8173b2 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -3456,13 +3456,6 @@ namespace bpkg
                 build_package* b (entered_build (p));
                 assert (b != nullptr);
 
-                // Reconfigure the configured dependencies (see
-                // collect_build_postponed() for details).
-                //
-                if (b->selected != nullptr &&
-                    b->selected->state == package_state::configured)
-                  b->flags |= build_package::adjust_reconfigure;
-
                 if (!b->recursive_collection)
                 {
                   l5 ([&]{trace << "collecting cfg-postponed dependency "
@@ -3470,17 +3463,19 @@ namespace bpkg
                                 << " of dependent "
                                 << pkg.available_name_version_db ();});
 
+                  assert (b->skeleton); // Should have been init'ed above.
+
+                  package_skeleton& ps (*b->skeleton);
+
                   // Similar to the inital negotiation case, verify and set
                   // the dependent configuration for this dependency.
                   //
                   {
-                    assert (b->skeleton); // Should have been init'ed above.
-
                     const package_configuration& pc (
                       cfg.dependency_configurations[p]);
 
-                    pair<bool, string> pr (b->skeleton->available != nullptr
-                                           ? b->skeleton->verify_sensible (pc)
+                    pair<bool, string> pr (ps.available != nullptr
+                                           ? ps.verify_sensible (pc)
                                            : make_pair (true, string ()));
 
                     if (!pr.first)
@@ -3494,7 +3489,7 @@ namespace bpkg
                       pc.print (dr, "    ");
                     }
 
-                    b->skeleton->dependent_config (pc);
+                    ps.dependent_config (pc);
                   }
 
                   collect_build_prerequisites (options,
@@ -3512,6 +3507,21 @@ namespace bpkg
                                                postponed_cfgs,
                                                postponed_poss,
                                                unacceptable_alts);
+
+                  // Unless the dependency is already being reconfigured,
+                  // reconfigure it if its configuration changes.
+                  //
+                  if (!b->reconfigure ())
+                  {
+                    const shared_ptr<selected_package>& sp (b->selected);
+
+                    if (sp != nullptr                          &&
+                        sp->state == package_state::configured &&
+                        sp->config_checksum != ps.config_checksum ())
+                    {
+                      b->flags |= build_package::adjust_reconfigure;
+                    }
+                  }
                 }
                 else
                   l5 ([&]{trace << "dependency "
@@ -4933,38 +4943,27 @@ namespace bpkg
         build_package* b (entered_build (p));
         assert (b != nullptr);
 
-        // Reconfigure the configured dependencies.
-        //
-        // Note that potentially this can be an overkill if the dependency
-        // configuration doesn't really change. Later we can implement some
-        // precise detection for that using configuration checksum or similar.
-        //
-        // Also note that for configured dependents which belong to the
-        // configuration cluster this flag is already set (see above).
-        //
-        if (b->selected != nullptr &&
-            b->selected->state == package_state::configured)
-          b->flags |= build_package::adjust_reconfigure;
-
         // Skip the dependencies which are already collected recursively.
         //
         if (!b->recursive_collection)
         {
+          assert (b->skeleton); // Should have been init'ed above.
+
+          package_skeleton& ps (*b->skeleton);
+
           // Verify and set the dependent configuration for this dependency.
           //
           // Note: see similar code for the up-negotiation case.
           //
           {
-            assert (b->skeleton); // Should have been init'ed above.
-
             const package_configuration& pc (
               pcfg->dependency_configurations[p]);
 
             // Skip the verification if this is a system package without
             // skeleton info.
             //
-            pair<bool, string> pr (b->skeleton->available != nullptr
-                                   ? b->skeleton->verify_sensible (pc)
+            pair<bool, string> pr (ps.available != nullptr
+                                   ? ps.verify_sensible (pc)
                                    : make_pair (true, string ()));
 
             if (!pr.first)
@@ -4983,7 +4982,7 @@ namespace bpkg
               pc.print (dr, "    "); // Note 4 spaces since in nested info.
             }
 
-            b->skeleton->dependent_config (pc);
+            ps.dependent_config (pc);
           }
 
           build_package_refs dep_chain;
@@ -5002,6 +5001,24 @@ namespace bpkg
                                        postponed_cfgs,
                                        postponed_poss,
                                        unacceptable_alts);
+
+          // Unless the dependency is already being reconfigured, reconfigure
+          // it if its configuration changes.
+          //
+          // Note that for configured dependents which belong to the
+          // configuration cluster this flag is already set (see above).
+          //
+          if (!b->reconfigure ())
+          {
+            const shared_ptr<selected_package>& sp (b->selected);
+
+            if (sp != nullptr                          &&
+                sp->state == package_state::configured &&
+                sp->config_checksum != ps.config_checksum ())
+            {
+              b->flags |= build_package::adjust_reconfigure;
+            }
+          }
         }
         else
           l5 ([&]{trace << "dependency " << b->available_name_version_db ()
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index 2801539..291929b 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -371,9 +371,12 @@ namespace bpkg
     // etc) as well as their sources.
     //
     vector<config_variable> srcs;
+    string checksum;
 
     if (!simulate)
     {
+      checksum = ps.config_checksum ();
+
       pair<strings, vector<config_variable>> rvs (move (ps).collect_config ());
 
       strings& vs (rvs.first);
@@ -395,7 +398,8 @@ namespace bpkg
 
     return configure_prerequisites_result {move (prereqs),
                                            move (vars),
-                                           move (srcs)};
+                                           move (srcs),
+                                           move (checksum)};
   }
 
 
@@ -767,6 +771,7 @@ namespace bpkg
 #endif
 
       p->config_variables = move (cpr.config_sources);
+      p->config_checksum  = move (cpr.config_checksum);
     }
 
     p->out_root = out_root.leaf ();
diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx
index 099e1e8..24e06a3 100644
--- a/bpkg/pkg-configure.hxx
+++ b/bpkg/pkg-configure.hxx
@@ -83,6 +83,11 @@ namespace bpkg
     // config_variables member.
     //
     vector<config_variable> config_sources; // Note: name and source.
+
+    // SHA256 checksum of variables (names and values) referred to by the
+    // config_sources member.
+    //
+    string config_checksum;
   };
 
   // Return the "would be" state for packages that would be configured
diff --git a/bpkg/pkg-disfigure.cxx b/bpkg/pkg-disfigure.cxx
index 690c3e1..af2c4f1 100644
--- a/bpkg/pkg-disfigure.cxx
+++ b/bpkg/pkg-disfigure.cxx
@@ -200,7 +200,10 @@ namespace bpkg
       }
 
       if (disfigure)
+      {
         p->config_variables.clear ();
+        p->config_checksum.clear ();
+      }
     }
 
     p->out_root = nullopt;