Handle bogus configuration variables

4 files changed, 244 insertions, 41 deletions

diff --git a/bpkg/package-configuration.cxx b/bpkg/package-configuration.cxx
index 624de18..560fb3b 100644
--- a/bpkg/package-configuration.cxx
+++ b/bpkg/package-configuration.cxx
@@ -3,12 +3,62 @@
 
 #include <bpkg/package-configuration.hxx>
 
+#include <sstream>
+
 #include <bpkg/package-skeleton.hxx>
 
 namespace bpkg
 {
   using build2::config::variable_origin;
 
+  string config_variable_value::
+  serialize_cmdline () const
+  {
+    using namespace build2;
+
+    string r (name + '=');
+
+    if (!value)
+      r += "[null]";
+    else
+    {
+      if (!value->empty ())
+      {
+        // Note: we need to use command-line (effective) quoting.
+        //
+        ostringstream os;
+        to_stream (os, *value, quote_mode::effective, '@');
+        r += os.str ();
+      }
+    }
+
+    return r;
+  }
+
+  void
+  to_checksum (sha256& cs, const config_variable_value& v)
+  {
+    using namespace build2;
+
+    cs.append (v.name);
+    cs.append (static_cast<uint8_t> (v.origin));
+    if (v.type)
+      cs.append (*v.type);
+
+    if (v.origin != variable_origin::undefined)
+    {
+      if (v.value)
+        for (const name& n: *v.value)
+          to_checksum (cs, n);
+
+      if (v.origin == variable_origin::buildfile)
+      {
+        cs.append (v.dependent->string ());
+        cs.append (v.confirmed);
+      }
+    }
+  }
+
   bool
   up_negotiate_configuration (
     package_configurations& cfgs,
@@ -118,9 +168,7 @@ namespace bpkg
 
             // Note that we will not reload it to default in case of require.
             //
-            v.origin = variable_origin::undefined;
-            v.value = nullopt;
-            v.dependent = nullopt;
+            v.undefine ();
           }
           else
             old_cfgs.push_back (
diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx
index d6fc989..655b414 100644
--- a/bpkg/package-configuration.hxx
+++ b/bpkg/package-configuration.hxx
@@ -46,8 +46,32 @@ namespace bpkg
     // first set this variable to this value.
     //
     optional<package_key> dependent;
+
+    // If origin is buildfile, then this flag indicates whether the
+    // originating dependent has been encountered during the negotiation
+    // retry.
+    //
+    bool confirmed;
+
+  public:
+    void
+    undefine ()
+    {
+      origin = build2::config::variable_origin::undefined;
+      value = nullopt;
+      dependent = nullopt;
+      confirmed = false;
+    }
+
+    // Serialize the variable value as a command line override.
+    //
+    string
+    serialize_cmdline () const;
   };
 
+  void
+  to_checksum (sha256&, const config_variable_value&);
+
   // A subset of config_variable_value for variable values set by the
   // dependents (origin is buildfile). Used to track change history.
   //
diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index 4813651..eaebf26 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -262,30 +262,6 @@ namespace bpkg
     return r;
   }
 
-  static string
-  serialize_cmdline (const string& var, const optional<build2::names>& val)
-  {
-    using namespace build2;
-
-    string r (var + '=');
-
-    if (!val)
-      r += "[null]";
-    else
-    {
-      if (!val->empty ())
-      {
-        // Note: we need to use command-line (effective) quoting.
-        //
-        ostringstream os;
-        to_stream (os, *val, quote_mode::effective, '@');
-        r += os.str ();
-      }
-    }
-
-    return r;
-  }
-
   // Reverse value to names.
   //
   static optional<build2::names>
@@ -317,7 +293,7 @@ namespace bpkg
     for (const config_variable_value& v: cfg)
     {
       if (v.origin == variable_origin::buildfile)
-        r.push_back (serialize_cmdline (v.name, v.value));
+        r.push_back (v.serialize_cmdline ());
     }
 
     return r;
@@ -411,7 +387,7 @@ namespace bpkg
         case variable_origin::override_:
         case variable_origin::undefined:
           {
-            config_variable_value v {var.name, ol.first, {}, {}, {}};
+            config_variable_value v {var.name, ol.first, {}, {}, {}, false};
 
             // Override could mean user override from config_vars_ or the
             // dependent override that we have merged above.
@@ -424,6 +400,7 @@ namespace bpkg
 
                 v.origin = variable_origin::buildfile;
                 v.dependent = move (ov->dependent);
+                v.confirmed = true;
               }
             }
 
@@ -1171,6 +1148,7 @@ namespace bpkg
 
                 v.value = move (ns);
                 v.dependent = key; // We are the originating dependent.
+                v.confirmed = true;
                 break;
               }
             case variable_origin::default_:
@@ -1448,6 +1426,7 @@ namespace bpkg
 
               v.value = move (ns);
               v.dependent = key; // We are the originating dependent.
+              v.confirmed = true;
             }
           }
         }
@@ -1792,7 +1771,7 @@ namespace bpkg
         for (const config_variable_value& v: cfg)
         {
           if (v.origin == variable_origin::override_)
-            dependency_vars.push_back (serialize_cmdline (v.name, v.value));
+            dependency_vars.push_back (v.serialize_cmdline ());
         }
       }
 
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 63bbc36..296a41c 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -2073,7 +2073,9 @@ namespace bpkg
   {
     size_t               depth;
     package_key          dependent;
+#if 0 // @@ TMP
     pair<size_t, size_t> position;
+#endif
   };
 
   struct merge_configuration
@@ -4624,7 +4626,7 @@ namespace bpkg
                 //
                 dep_chain.clear ();
 
-                throw retry_configuration {cfg.depth, move (pk), dp};
+                throw retry_configuration {cfg.depth, move (pk)/*, dp*/}; // @@ TMP
               }
 
               l5 ([&]{trace << "configuration for cfg-postponed "
@@ -5570,6 +5572,7 @@ namespace bpkg
                              postponed_packages& postponed_alts,
                              postponed_dependencies& postponed_deps,
                              postponed_configurations& postponed_cfgs,
+                             strings& postponed_cfgs_history,
                              postponed_positions& postponed_poss,
                              const function<find_database_function>& fdb,
                              const repointed_dependents& rpt_depts,
@@ -5578,6 +5581,8 @@ namespace bpkg
     {
       // Snapshot of the package builds collection state.
       //
+      // Note: should not include postponed_cfgs_history.
+      //
       class snapshot
       {
       public:
@@ -6440,6 +6445,7 @@ namespace bpkg
                                        postponed_alts,
                                        postponed_deps,
                                        postponed_cfgs,
+                                       postponed_cfgs_history,
                                        postponed_poss,
                                        fdb,
                                        rpt_depts,
@@ -6507,7 +6513,7 @@ namespace bpkg
 
               break;
             }
-            catch (retry_configuration& e)
+            catch (const retry_configuration& e)
             {
               // If this is not "our problem", then keep looking.
               //
@@ -6529,21 +6535,32 @@ namespace bpkg
 
               pc = &postponed_cfgs[ci];
 
+              // @@ TMP: "shadow" seems no longer correct.
+              //
               l5 ([&]{trace << "cfg-negotiation of " << *pc << " failed due "
                             << "to dependent " << e.dependent << ", adding "
                             << "shadow dependent and re-negotiating";});
 
-              // @@ TODO
-              //
-              // Note that there is also a possibility of having a "bogus"
-              // dependent/dependencies (i.e., we add them to the cluster but
-              // they never get re-visited). While seems harmless (or not),
-              // maybe we need to prune the part of the configuration that was
-              // added by them? Maybe we should have the confirmed flag which
-              // is set when the originating dependent confirms the value?!
-
               // Copy over the configuration for further refinement.
               //
+              // Note that there is also a possibility of ending up with
+              // "bogus" configuration variables that were set by a dependent
+              // during up-negotiation but, due to changes to the overall
+              // configuration, such a dependent were never re-visited.
+              //
+              // The way we are going to deal with this is by detecting such
+              // bogus variables based on the confirmed flag, cleaning them
+              // out, and doing another retry. Here we clear the confirmed
+              // flag and the detection happens in collect_build_postponed()
+              // after we have processed everything postponed (since that's
+              // the only time we can be certain there could no longer be a
+              // re-visit).
+              //
+              for (package_configuration& cfg: cfgs)
+                for (config_variable_value& v: cfg)
+                  if (v.dependent)
+                    v.confirmed = false;
+
               pc->dependency_configurations = move (cfgs);
 
 #if 0
@@ -6810,6 +6827,139 @@ namespace bpkg
         postponed_deps.cancel_bogus (trace, false /* initial_collection */);
       }
 
+      // Check if any negotiatiated configurations ended up with any bogus
+      // variables (see retry_configuration catch block for background).
+      //
+      // Note that we could potentially end up yo-yo'ing: we remove a bogus
+      // and that causes the original dependent to get re-visited which in
+      // turn re-introduces the bogus. In other words, one of the bogus
+      // variables which we have removed are actually the cause of no longer
+      // needing the dependent that introduced it. Feels like the correct
+      // outcome of this should be keeping the bogus variable that triggered
+      // yo-yo'ing. Of course, there could be some that we should keep and
+      // some that we should drop and figuring this out would require retrying
+      // all possible combinations. An alternative solution would be to detect
+      // yo-yo'ing, print the bogus variables involved, and ask the user to
+      // choose (with an override) which ones to keep. Let's go with this for
+      // now.
+      //
+      {
+        // On the first pass see if we have anything bogus.
+        //
+        bool bogus (false);
+        for (postponed_configuration& pcfg: postponed_cfgs)
+        {
+          if (pcfg.negotiated && *pcfg.negotiated) // Negotiated.
+          {
+            for (package_configuration& cfg: pcfg.dependency_configurations)
+            {
+              for (config_variable_value& v: cfg)
+              {
+                if (v.dependent && !v.confirmed)
+                {
+                  bogus = true;
+                  break;
+                }
+              }
+              if (bogus) break;
+            }
+            if (bogus) break;
+          }
+        }
+
+        if (bogus)
+        {
+          // On the second pass calculate the checksum of all the negotiated
+          // clusters.
+          //
+          sha256 cs;
+          for (postponed_configuration& pcfg: postponed_cfgs)
+          {
+            if (pcfg.negotiated && *pcfg.negotiated)
+            {
+              for (package_configuration& cfg: pcfg.dependency_configurations)
+              {
+                for (config_variable_value& v: cfg)
+                {
+                  if (v.dependent)
+                    to_checksum (cs, v);
+                }
+              }
+            }
+          }
+
+          bool cycle;
+          {
+            string s (cs.string ());
+            if (find (postponed_cfgs_history.begin (),
+                      postponed_cfgs_history.end (),
+                      s) == postponed_cfgs_history.end ())
+            {
+              postponed_cfgs_history.push_back (move (s));
+              cycle = false;
+            }
+            else
+              cycle = true;
+          }
+
+          // On the third pass we either retry or diagnose.
+          //
+          diag_record dr;
+          if (cycle)
+          {
+            dr <<
+              fail << "unable to remove bogus configuration values without "
+                   << "causing configuration refinement cycle" <<
+              info << "consider manually specifying one or more of the "
+                   << "following variables as user configuration";
+          }
+
+          for (postponed_configuration& pcfg: postponed_cfgs)
+          {
+            optional<package_key> dept; // Bogus dependent.
+
+            if (pcfg.negotiated && *pcfg.negotiated)
+            {
+              for (package_configuration& cfg: pcfg.dependency_configurations)
+              {
+                // Note that the entire dependency configuration may end up
+                // being "bogus" (i.e., it does not contain any configuration
+                // variables with a confirmed dependent). But that will be
+                // handled naturally: we will either no longer have this
+                // dependency in the cluster and thus never call its
+                // skeleton's dependent_config() or this call will be no-op
+                // since it won't find any dependent variables.
+                //
+                for (config_variable_value& v: cfg)
+                {
+                  if (v.dependent && !v.confirmed)
+                  {
+                    if (!dept)
+                      dept = move (v.dependent);
+
+                    if (cycle)
+                      dr << info << v.serialize_cmdline ();
+                    else
+                      v.undefine ();
+                  }
+                }
+              }
+
+              if (dept)
+              {
+                if (cycle)
+                  break;
+                else
+                  throw retry_configuration {pcfg.depth, move (*dept)};
+              }
+            }
+
+            if (dept)
+              break;
+          }
+        }
+      }
+
       // If any postponed_{repo,alts} builds remained, then perform the
       // diagnostics run. Naturally we shouldn't have any postponed_cfgs
       // without one of the former.
@@ -10826,6 +10976,7 @@ namespace bpkg
         postponed_packages       postponed_repo;
         postponed_packages       postponed_alts;
         postponed_configurations postponed_cfgs;
+        strings                  postponed_cfgs_history;
 
         try
         {
@@ -11099,6 +11250,7 @@ namespace bpkg
                                           postponed_alts,
                                           postponed_deps,
                                           postponed_cfgs,
+                                          postponed_cfgs_history,
                                           postponed_poss,
                                           find_prereq_database,
                                           rpt_depts,