Use original variable name in config report

5 files changed, 130 insertions, 42 deletions

diff --git a/doc/manual.cli b/doc/manual.cli
index 93053e4..847691d 100644
--- a/doc/manual.cli
+++ b/doc/manual.cli
@@ -4966,6 +4966,32 @@ if! $defined(config.libhello.database)
   fail 'config.libhello.database must be specified'
 \
 
+\N|A configuration variable without a default value is omitted from
+\c{config.build} unless the value is specified by the user. This semantics is
+useful for values that are normally derived from other configuration values
+but could also be specified by the user. If the value is derived, then we
+don't want it saved in \c{config.build} since that would prevent it from
+being re-derived if the configuration values it is based on are changed.
+For example:
+
+\
+config [strings] config.hello.database
+
+assert ($size($config.hello.database) > 0) \
+  'database must be specified with config.hello.database'
+
+config [bool, config.report.variable=multi] config.hello.multi_database
+
+multi = ($defined(config.hello.multi_database) \
+         ? $config.hello.multi_database        \
+         : $size(config.hello.database) > 1)
+
+assert ($multi || $size(config.hello.database) == 1) \
+  'one database can be specified if config.hello.multi_database=false'
+\
+
+|
+
 If computing the default value is expensive or requires elaborate logic, then
 the handling of a configuration variable can be broken down into two steps
 along these lines:
diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index 0122889..ee1b6b1 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -4,6 +4,7 @@
 #include <libbuild2/file.hxx>
 
 #include <cerrno>
+#include <cstring> // strlen()
 #include <iomanip> // left, setw()
 #include <sstream>
 
@@ -1639,8 +1640,10 @@ namespace build2
     // Print the project configuration report(s), similar to how we do it in
     // build system modules.
     //
+    using config_report = parser::config_report;
+
     const project_name* proj (nullptr); // Resolve lazily.
-    for (const parser::config_report& cr: p.config_reports)
+    for (const config_report& cr: p.config_reports)
     {
       if (verb < (cr.new_value ? 2 : 3))
         continue;
@@ -1673,26 +1676,45 @@ namespace build2
                       ? '.' + proj->variable () + '.'
                       : string ()));
 
-      // Calculate max name length.
+      // Return the variable name for printing.
       //
-      size_t pad (10);
-      for (const pair<lookup, string>& lf: cr.values)
+      auto name = [&stem] (const config_report::value& cv) -> const char*
       {
-        lookup l (lf.first);
+        lookup l (cv.val);
 
-        size_t n;
         if (l.value == nullptr)
         {
-          n = l.var->name.size ();
+          if (cv.org.empty ())
+            return l.var->name.c_str ();
+
+          // This case may or may not have the prefix.
+          //
+          size_t p, n (
+            !stem.empty ()
+            ? (p = cv.org.find (stem)) != string::npos ? p + stem.size () : 0
+            : cv.org.compare (0, 7, "config.") == 0 ? 7 : 0);
+
+          return cv.org.c_str () + n;
         }
         else
         {
+          assert (cv.org.empty ()); // Sanity check.
+
           size_t p (!stem.empty ()
                     ? l.var->name.find (stem) + stem.size ()
                     : 7); // "config."
-          n = l.var->name.size () - p;
+
+          return l.var->name.c_str () + p;
         }
+      };
+
 
+      // Calculate max name length.
+      //
+      size_t pad (10);
+      for (const config_report::value& cv: cr.values)
+      {
+        size_t n (strlen (name (cv)));
         if (n > pad)
           pad = n;
       }
@@ -1705,13 +1727,14 @@ namespace build2
          << ' ' << *proj << '@' << root;
 
       names storage;
-      for (const pair<lookup, string>& lf: cr.values)
+      for (const config_report::value& cv: cr.values)
       {
-        lookup l (lf.first);
-        const string& f (lf.second);
+        lookup l (cv.val);
+        const string& f (cv.fmt);
 
         // If the report variable has been overriden, now is the time to
-        // lookup its value.
+        // lookup its value. Note: see also the name() lambda above if
+        // changing anything here.
         //
         string n;
         if (l.value == nullptr)
@@ -1727,6 +1750,8 @@ namespace build2
           n = string (l.var->name, p);
         }
 
+        const char* pn (name (cv)); // Print name.
+
         dr << "\n  ";
 
         if (l)
@@ -1736,15 +1761,15 @@ namespace build2
 
           if (f == "multiline")
           {
-            dr << n;
+            dr << pn;
             for (auto& n: ns)
               dr << "\n    " << n;
           }
           else
-            dr << left << setw (static_cast<int> (pad)) << n << ' ' << ns;
+            dr << left << setw (static_cast<int> (pad)) << pn << ' ' << ns;
         }
         else
-          dr << left << setw (static_cast<int> (pad)) << n << " [null]";
+          dr << left << setw (static_cast<int> (pad)) << pn << " [null]";
       }
     }
 
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 2623bf3..4cbb30d 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -3852,6 +3852,9 @@ namespace build2
         try
         {
           report_var = convert<string> (move (i->value));
+
+          if (!report)
+            report = string ("true");
         }
         catch (const invalid_argument& e)
         {
@@ -3889,9 +3892,13 @@ namespace build2
 
     // As a way to print custom (discovered, computed, etc) configuration
     // information we allow specifying a non config.* variable provided it is
-    // explicitly marked with the config.report attribute.
+    // explicitly marked with the config.report attribute (or another
+    // attribute that implies it).
     //
     bool new_val (false);
+    string org_var; // Original variable if config.report.variable specified.
+
+    const variable* var (nullptr); // config.* variable.
     lookup l;
 
     if (report && *report != "false" && !config)
@@ -3910,7 +3917,14 @@ namespace build2
       // philosophical question. In either case it doesn't seem useful for it
       // to unconditionally force reporting at level 2.
       //
-      report_var = move (name);
+      if (!report_var.empty ())
+      {
+        // For example, config [config.report.variable=multi] multi_database
+        //
+        org_var = move (name);
+      }
+      else
+        report_var = move (name);
 
       next (t, tt); // We shouldn't have the default value part.
     }
@@ -4040,17 +4054,16 @@ namespace build2
              << (proj.empty () ? "<project>" : proj.c_str ()) << ".**' form";
       }
 
-      const variable& var (
-        parse_variable_name (move (name), get_location (t)));
-      apply_variable_attributes (var);
+      var = &parse_variable_name (move (name), get_location (t));
+      apply_variable_attributes (*var);
 
       // Note that even though we are relying on the config.** variable
       // pattern to set global visibility, let's make sure as a sanity check.
       //
-      if (var.visibility != variable_visibility::global)
+      if (var->visibility != variable_visibility::global)
       {
-        fail (t) << "configuration variable " << var << " has "
-                 << var.visibility << " visibility";
+        fail (t) << "configuration variable " << *var << " has "
+                 << var->visibility << " visibility";
       }
 
       // See if we have the default value part.
@@ -4072,15 +4085,15 @@ namespace build2
       //
       bool dev;
       {
-        size_t p (var.name.rfind ('.'));
-        dev = p != 6 && var.name.compare (p + 1, string::npos, "develop") == 0;
+        size_t p (var->name.rfind ('.'));
+        dev = p != 6 && var->name.compare (p + 1, string::npos, "develop") == 0;
       }
 
       uint64_t sflags (0);
       if (dev)
       {
-        if (var.type != &value_traits<bool>::value_type)
-          fail (loc) << var << " variable must be of type bool";
+        if (var->type != &value_traits<bool>::value_type)
+          fail (loc) << *var << " variable must be of type bool";
 
         // This is quite messy: below we don't always parse the value (plus it
         // may be computed) so here we just peek at the next token. But we
@@ -4089,10 +4102,10 @@ namespace build2
         if (!def_val                                    ||
             peek (lexer_mode::value, '@') != type::word ||
             peeked ().value != "false")
-          fail (loc) << var << " variable default value must be literal false";
+          fail (loc) << *var << " variable default value must be literal false";
 
         if (nullable)
-          fail (loc) << var << " variable must not be nullable";
+          fail (loc) << *var << " variable must not be nullable";
 
         sflags |= config::save_false_omitted;
       }
@@ -4101,7 +4114,7 @@ namespace build2
       // in order to mark it as saved. We also have to do this to get the new
       // value status.
       //
-      l = config::lookup_config (new_val, *root_, var, sflags);
+      l = config::lookup_config (new_val, *root_, *var, sflags);
 
       // Handle the default value.
       //
@@ -4137,12 +4150,12 @@ namespace build2
         else
         {
           value lhs, rhs (parse_variable_value (t, tt, !dev /* mode */));
-          apply_value_attributes (&var, lhs, move (rhs), type::assign);
+          apply_value_attributes (var, lhs, move (rhs), type::assign);
 
           if (!nullable)
             nullable = lhs.null;
 
-          l = config::lookup_config (new_val, *root_, var, move (lhs), sflags);
+          l = config::lookup_config (new_val, *root_, *var, move (lhs), sflags);
         }
       }
 
@@ -4153,7 +4166,7 @@ namespace build2
       // then the user is expected to handle the undefined case).
       //
       if (!nullable && l.defined () && l->null)
-        fail (loc) << "null value in non-nullable variable " << var;
+        fail (loc) << "null value in non-nullable variable " << *var;
     }
 
     // We will be printing the report at either level 2 (-v) or 3 (-V)
@@ -4188,6 +4201,9 @@ namespace build2
       //
       if (!report_var.empty ())
       {
+        if (org_var.empty () && var != nullptr)
+          org_var = var->name;
+
         // In a somewhat hackish way we pass the variable in an undefined
         // lookup.
         //
@@ -4201,23 +4217,33 @@ namespace build2
 
       if (l.var != nullptr)
       {
-        auto r (make_pair (l, move (*report)));
-
         // If we have a duplicate, update it (it could be useful to have
         // multiple config directives to "probe" the value before calculating
         // the default; see lookup_config() for details).
         //
+        // Since the original variable is what the user will see in the
+        // report, we prefer that as a key.
+        //
         auto i (find_if (report_values.begin (),
                          report_values.end (),
-                         [&l] (const pair<lookup, string>& p)
+                         [&org_var, &l] (const config_report::value& v)
                          {
-                           return p.first.var == l.var;
+                           return (v.org.empty () && org_var.empty ()
+                                   ? v.val.var == l.var
+                                   : (v.org.empty ()
+                                      ? v.val.var->name == org_var
+                                      : v.org == l.var->name));
                          }));
 
         if (i == report_values.end ())
-          report_values.push_back (move (r));
+          report_values.push_back (
+            config_report::value {l, move (*report), move (org_var)});
         else
-          *i = move (r);
+        {
+          i->val = l;
+          i->fmt = move (*report);
+          if (i->org.empty ()) i->org = move (org_var);
+        }
 
         report_new_value = report_new_value || new_val;
       }
diff --git a/libbuild2/parser.hxx b/libbuild2/parser.hxx
index 0645b5a..3e1d0a0 100644
--- a/libbuild2/parser.hxx
+++ b/libbuild2/parser.hxx
@@ -136,9 +136,16 @@ namespace build2
     //
     struct config_report
     {
-      project_name                 module;    // Reporting module name.
-      vector<pair<lookup, string>> values;    // Config value and format.
-      bool                         new_value; // One of values is new.
+      struct value
+      {
+        lookup val; // Value.
+        string fmt; // Format.
+        string org; // Original variable if config.report.variable.
+      };
+
+      project_name  module;    // Reporting module name.
+      vector<value> values;
+      bool          new_value; // One of values is new.
     };
     small_vector<config_report, 1> config_reports;
 
diff --git a/tests/directive/config.testscript b/tests/directive/config.testscript
index fba858f..ebdd6ac 100644
--- a/tests/directive/config.testscript
+++ b/tests/directive/config.testscript
@@ -212,12 +212,14 @@ test.arguments =
     config [strings, config.report=multiline]  config.test.d ?= 1 2 3
     config [string,  config.report.variable=e] config.test.e ?= abc
     config [         config.report]            f
+    config [         config.report.variable=g] gg
     config [bool]                              config.test.n ?= [null]
     config [bool]                              config.test.p
     config [bool]                              config.test.p ?= true
 
     e = "'$config.test.e'"
     f = ($config.test.b || $config.test.c)
+    g = abc
 
     EOI
 
@@ -240,6 +242,7 @@ test.arguments =
         3
       e          'abc'
       f          true
+      gg         abc
       n          [null]
       p          true
     EOO
@@ -262,6 +265,7 @@ test.arguments =
         3
       e          'xyz'
       f          true
+      gg         abc
       n          true
       p          false
     EOO