Improve diagnostics

4 files changed, 80 insertions, 24 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 54f5a37..da43ef6 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -5371,7 +5371,7 @@ namespace bpkg
     vector<configure_package> configure_packages;
     configure_packages.reserve (build_pkgs.size ());
 
-    // While at it also extract global configuration variable overrides from
+    // While at it also collect global configuration variable overrides from
     // each configure_prerequisites_result::config_variables and merge them
     // into configure_global_vars.
     //
@@ -5575,42 +5575,35 @@ namespace bpkg
 
         t.commit ();
 
-        /*
-        if (!simulate && !cpr.config_variables.empty ())
+        if (verb >= 5 && !simulate && !cpr.config_variables.empty ())
         {
-          diag_record dr (text);
+          diag_record dr (trace);
 
-          dr << sp->name << pdb << ':';
+          dr << sp->name << pdb << " configuration variables:";
 
           for (const string& cv: cpr.config_variables)
             dr << "\n  " << cv;
         }
-        */
 
         if (!simulate)
         {
-
 #ifndef BPKG_OUTPROC_CONFIGURE
           auto& gvs (configure_global_vars);
 
-          for (auto i (cpr.config_variables.begin ());
-               i != cpr.config_variables.end (); )
+          // Note that we keep global overrides in cpr.config_variables for
+          // diagnostics and skip them in var_override_function below.
+          //
+          for (const string& v: cpr.config_variables)
           {
             // Each package should have exactly the same set of global
             // overrides by construction since we don't allow package-
             // specific global overrides.
             //
-            string& v (*i);
-
             if (v[0] == '!')
             {
               if (find (gvs.begin (), gvs.end (), v) == gvs.end ())
-                gvs.push_back (move (v));
-
-              i = cpr.config_variables.erase (i);
+                gvs.push_back (v);
             }
-            else
-              ++i;
           }
 #endif
           // Add config.config.disfigure unless already disfigured (see the
@@ -5646,6 +5639,9 @@ namespace bpkg
           {
             for (const string& v: cp.res.config_variables)
             {
+              if (v[0] == '!') // Skip global overrides (see above).
+                continue;
+
               pair<char, variable_override> p (
                 ctx.parse_variable_override (v, i++, false /* buildspec */));
 
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index ccf2f54..83ec245 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -287,6 +287,8 @@ namespace bpkg
                     od = sp->effective_out_root (pdb.config);
 
 #ifndef BPKG_OUTPROC_CONFIGURE
+                  // @@
+                  //
                   // Use global overrides to recreate the original behavior of
                   // not warning about unused config.import.* variables
                   // (achived via the config.config.persist value in
@@ -294,12 +296,17 @@ namespace bpkg
                   // don't actually save the unused values anywhere, just
                   // don't warn about them).
                   //
-                  // @@ Can we somehow cause a clash, say if the same package
-                  //    comes from different configurations? Yeah, we probably
-                  //    can. Maybe detect a clash somehow and "fallforward" to
-                  //    the correct behavior?
+                  // Can we somehow cause a clash, say if the same package
+                  // comes from different configurations? Yeah, we probably
+                  // can. So could add it as undermined (?), detect a clash,
+                  // and "fallforward" to the correct behavior.
+                  //
+                  // But we can clash with an absent value -- that is, we
+                  // force importing from a wrong configuration where without
+                  // any import things would have been found in the same
+                  // amalgamation.
                   //
-                  vars.push_back ("!config.import." + sp->name.variable () +
+                  vars.push_back ("config.import." + sp->name.variable () +
                                   "='" + od.representation () + '\'');
 #else
                   vars.push_back ("config.import." + sp->name.variable () +
@@ -533,8 +540,24 @@ namespace bpkg
         throw;
       }
 #else
-      // @@ Would be good to print the equivalent command line in -v.
+      // Print the out-process command line in the verbose mode.
       //
+      if (verb >= 2)
+      {
+        string bspec;
+
+        // Use path representation to get canonical trailing slash.
+        //
+        if (src_root == out_root)
+          bspec = "configure('" + out_root.representation () + "')";
+        else
+          bspec = "configure('" +
+            src_root.representation () + "'@'" +
+            out_root.representation () + "')";
+
+        print_b (o, verb_b::quiet, cpr.config_variables, bspec);
+      }
+
       try
       {
         // Note: no bpkg::failed should be thrown from this block.
@@ -585,9 +608,9 @@ namespace bpkg
 
           if (src_root != p)
           {
-            // @@ fuzzy if need this or can do as package skeleton (seeing
+            // @@ Fuzzy if need this or can do as package skeleton (seeing
             //    that we know we are re-configuring).
-
+            //
             ctx.new_src_root = src_root;
             ctx.old_src_root = move (p);
             p = src_root;
diff --git a/bpkg/utility.hxx b/bpkg/utility.hxx
index bb264ba..f36185a 100644
--- a/bpkg/utility.hxx
+++ b/bpkg/utility.hxx
@@ -66,6 +66,7 @@ namespace bpkg
   // <libbutl/process.hxx>
   //
   using butl::process_start_callback;
+  using butl::process_print_callback;
 
   // <libbutl/filesystem.hxx>
   //
@@ -254,6 +255,10 @@ namespace bpkg
   process_path
   search_b (const common_options&);
 
+  template <typename... A>
+  void
+  print_b (const common_options&, verb_b, A&&... args);
+
   template <typename O, typename E, typename... A>
   process
   start_b (const common_options&, O&& out, E&& err, verb_b, A&&... args);
diff --git a/bpkg/utility.txx b/bpkg/utility.txx
index 8bdd7ec..33bb711 100644
--- a/bpkg/utility.txx
+++ b/bpkg/utility.txx
@@ -59,6 +59,36 @@ namespace bpkg
       ops.push_back ("--no-progress");
   }
 
+  template <typename... A>
+  void
+  print_b (const common_options& co, verb_b v, A&&... args)
+  {
+    process_path pp (search_b (co));
+
+    small_vector<const char*, 1> ops;
+
+    // As in start_b() below.
+    //
+    string verb_arg;
+    map_verb_b (co, v, ops, verb_arg);
+
+    if (co.diag_color ())
+      ops.push_back ("--diag-color");
+
+    if (co.no_diag_color ())
+      ops.push_back ("--no-diag-color");
+
+    process_print_callback (
+      [] (const char* const args[], size_t n)
+      {
+        print_process (args, n);
+      },
+      pp,
+      ops,
+      co.build_option (),
+      forward<A> (args)...);
+  }
+
   template <typename O, typename E, typename... A>
   process
   start_b (const common_options& co,
@@ -73,6 +103,8 @@ namespace bpkg
     {
       small_vector<const char*, 1> ops;
 
+      // NOTE: see print_b() above if changing anything here.
+      //
       // NOTE: see custom versions in system_package_manager* if adding
       //       anything new here (search for search_b()).