Start work on loading configuration variables defaults

4 files changed, 300 insertions, 197 deletions

diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index a9fb31d..aca34aa 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -41,6 +41,12 @@ namespace bpkg
   void
   build2_init (const common_options&);
 
+  // Note: cannot be package_skeleton member function due to iterator return
+  // (build2 stuff is only forward-declared in the header).
+  //
+  static build2::scope_map::iterator
+  bootstrap (package_skeleton&, const strings&);
+
   package_skeleton::
   ~package_skeleton ()
   {
@@ -61,6 +67,7 @@ namespace bpkg
       db_ = v.db_;
       available_ = v.available_;
       config_vars_ = move (v.config_vars_);
+      disfigure_ = v.disfigure_;
       config_srcs_ = v.config_srcs_;
       src_root_ = move (v.src_root_);
       out_root_ = move (v.out_root_);
@@ -85,6 +92,7 @@ namespace bpkg
         db_ (v.db_),
         available_ (v.available_),
         config_vars_ (v.config_vars_),
+        disfigure_ (v.disfigure_),
         config_srcs_ (v.config_srcs_),
         src_root_ (v.src_root_),
         out_root_ (v.out_root_),
@@ -103,6 +111,7 @@ namespace bpkg
                     database& db,
                     const available_package& ap,
                     strings cvs,
+                    bool df,
                     const vector<config_variable>* css,
                     optional<dir_path> src_root,
                     optional<dir_path> out_root)
@@ -110,7 +119,8 @@ namespace bpkg
         db_ (&db),
         available_ (&ap),
         config_vars_ (move (cvs)),
-        config_srcs_ (css)
+        disfigure_ (df),
+        config_srcs_ (df ? nullptr : css)
   {
     // Should not be created for stubs.
     //
@@ -139,6 +149,85 @@ namespace bpkg
       assert (!out_root);
   }
 
+  void package_skeleton::
+  load_defaults ()
+  {
+    assert (ctx_ == nullptr); // Should only be called before evaluate_*().
+
+    try
+    {
+      using namespace build2;
+      using build2::fail;
+      using build2::endf;
+
+      // @@ TODO: c.c.{disfigure|unload}.
+
+      scope& rs (*bootstrap (*this, merge_cmd_vars ())->second.front ());
+
+      // Load project's root.build.
+      //
+      load_root (rs);
+
+      // Note that a configuration variable may not have a default value so we
+      // cannot just iterate over all the config.<name>** values set on root
+      // scope. Our options seem to be either iterating over the variable pool
+      // or forcing the config module with config.config.module=true and then
+      // using its saved variables map. Since the amout of stuff we load is
+      // quite limited, there shouldn't be too many variables in the pool. So
+      // let's go with the simpler approach for now.
+      //
+      // Though the saved variables map approach would have been more accurate
+      // since that's the variables that were introduced with the config
+      // directive. Potentially the user could just have a buildfile
+      // config.<name>** variable but it feels like that should be harmless
+      // (we will return it but nobody will presumably use that information).
+      //
+      string p ("config." + name ().variable ());
+      size_t n (p.size ());
+
+      for (const variable& var: rs.ctx.var_pool)
+      {
+        if (var.name.compare (0, n, p) != 0 || (var.name[n + 1] != '.' &&
+                                                var.name[n + 1] != '\0'))
+          continue;
+
+        using config::variable_origin;
+
+        pair<variable_origin, lookup> ol (config::origin (rs, var));
+
+        switch (ol.first)
+        {
+        case variable_origin::override_: break; // Value in config_vars.
+        case variable_origin::undefined: break; // No default value.
+        case variable_origin::default_:
+          {
+            // @@ TODO: save in some form?
+            //
+            // How will we enter them (along with value.extra=1) in the
+            // dependent's context. Probably programmatically. Perhaps we
+            // should just save them as untyped names? Then enter and
+            // typify. Seems reasonable. Not clear how/if we can convey
+            // overriden. Maybe we can allow the prefer/require to override
+            // them but then ignore their values?
+            //
+            break;
+          }
+        case variable_origin::buildfile:
+          {
+            // How can this happen if we disfigured all of them?
+            //
+            assert (false);
+            break;
+          }
+        }
+      }
+    }
+    catch (const build2::failed&)
+    {
+      throw failed (); // Assume the diagnostics has already been issued.
+    }
+  }
+
   // Print the location of a depends value in the specified manifest file.
   //
   // Note that currently we only use this function for the external packages.
@@ -748,8 +837,178 @@ namespace bpkg
     return make_pair (move (vars), move (srcs));
   }
 
-  // Note: cannot be package_skeleton member function due to iterator return
-  // (build2 stuff is only forward-declared in the header).
+  const strings& package_skeleton::
+  merge_cmd_vars ()
+  {
+    // Merge variable overrides (note that the order is important).
+    //
+    // We can reasonably assume reflect cannot have global or absolute scope
+    // variable overrides so we don't need to pass them to context.
+    //
+    const strings* r;
+
+    const strings& v1 (build2_cmd_vars);
+    const strings& v2 (config_vars_);
+
+    r = (v2.empty () ? &v1 : v1.empty () ? &v2 : nullptr);
+
+    if (r == nullptr)
+    {
+      if (cmd_vars_.empty ()) // Cached.
+      {
+        cmd_vars_.reserve (v1.size () + v2.size ());
+        cmd_vars_.assign (v1.begin (), v1.end ());
+        cmd_vars_.insert (cmd_vars_.end (), v2.begin (), v2.end ());
+      }
+
+      r = &cmd_vars_;
+    }
+
+    return *r;
+  }
+
+  build2::scope& package_skeleton::
+  load ()
+  {
+    if (ctx_ != nullptr)
+      return *rs_;
+
+    try
+    {
+      using namespace build2;
+
+      auto rsi (bootstrap (*this, merge_cmd_vars ()));
+      scope& rs (*rsi->second.front ());
+
+      // Load project's root.build as well as potentially accumulated reflect
+      // fragment.
+      //
+      // If we have the accumulated reflect fragment, wedge it just before
+      // loading root.build (but after initializing config which may load
+      // config.build and which we wish to override).
+      //
+      // Note that the plan for non-external packages is to extract the
+      // configuration and then load it with config.config.load and this
+      // approach should work for that case too.
+      //
+      function<void (parser&)> pre;
+
+      if (!reflect_frag_.empty ())
+      {
+        pre = [this, &rs] (parser& p)
+        {
+          istringstream is (reflect_frag_);
+          is.exceptions (istringstream::failbit | istringstream::badbit);
+
+          // Note that the fragment is just a bunch of variable assignments
+          // and thus unlikely to cause any errors.
+          //
+          path_name in ("<accumulated-reflect-fragment>");
+          p.parse_buildfile (is, in, &rs, rs);
+        };
+      }
+
+      load_root (rs, pre);
+
+      setup_base (rsi,
+                  out_root_.empty () ? src_root_ : out_root_,
+                  src_root_);
+
+      rs_ = &rs;
+      return rs;
+    }
+    catch (const build2::failed&)
+    {
+      throw failed (); // Assume the diagnostics has already been issued.
+    }
+  }
+
+  void package_skeleton::
+  merge_old_config_vars ()
+  {
+    if (config_srcs_ == nullptr)
+      return;
+
+    assert (reflect_frag_.empty ()); // Too late.
+
+    ctx_ = nullptr; // Reload.
+
+    try
+    {
+      using namespace build2;
+
+      scope& rs (*bootstrap (*this, merge_cmd_vars ())->second.front ());
+
+      // Load project's root.build.
+      //
+      load_root (rs);
+
+      // Extract and merge old user configuration variables from config.build
+      // (or equivalent) into config_vars. Then invalidate loaded state in
+      // order to make them overrides.
+      //
+      auto i (config_vars_.begin ()); // Insert position, see below.
+
+      names storage;
+      for (const config_variable& v: *config_srcs_)
+      {
+        if (v.source != config_source::user)
+          continue;
+
+        using config::variable_origin;
+
+        pair<variable_origin, lookup> ol (config::origin (rs, v.name));
+
+        switch (ol.first)
+        {
+        case variable_origin::override_:
+          {
+            // Already in config_vars.
+            //
+            // @@ TODO: theoretically, this could be an append/prepend
+            //    override(s) and to make this work correctly we would need
+            //    to replace them with an assign override with the final
+            //    value. Maybe one day.
+            //
+            break;
+          }
+        case variable_origin::buildfile:
+          {
+            // Doesn't really matter where we add them though conceptually
+            // feels like old should go before new (and in the original
+            // order).
+            //
+            i = config_vars_.insert (
+              i,
+              serialize_cmdline (v.name, *ol.second, storage)) + 1;
+
+            break;
+          }
+        case variable_origin::undefined:
+        case variable_origin::default_:
+          {
+            // Old user configuration no longer in config.build. We could
+            // complain but that feels overly drastic. Seeing that we will
+            // recalculate the new set of config variable sources, let's
+            // just ignore this (we could issue a warning, but who knows how
+            // many times it will be issued with all this backtracking).
+            //
+            break;
+          }
+        }
+      }
+
+      config_srcs_ = nullptr; // Merged.
+      cmd_vars_.clear ();     // Invalidated.
+      ctx_ = nullptr;         // Drop.
+    }
+    catch (const build2::failed&)
+    {
+      throw failed (); // Assume the diagnostics has already been issued.
+    }
+  }
+
+  // Bootstrap the package skeleton.
   //
   static build2::scope_map::iterator
   bootstrap (package_skeleton& skl, const strings& cmd_vars)
@@ -982,175 +1241,4 @@ namespace bpkg
       throw failed (); // Assume the diagnostics has already been issued.
     }
   }
-
-  const strings& package_skeleton::
-  merge_cmd_vars ()
-  {
-    // Merge variable overrides (note that the order is important).
-    //
-    // We can reasonably assume reflect cannot have global or absolute scope
-    // variable overrides so we don't need to pass them to context.
-    //
-    const strings* r;
-
-    const strings& v1 (build2_cmd_vars);
-    const strings& v2 (config_vars_);
-
-    r = (v2.empty () ? &v1 : v1.empty () ? &v2 : nullptr);
-
-    if (r == nullptr)
-    {
-      if (cmd_vars_.empty ()) // Cached.
-      {
-        cmd_vars_.reserve (v1.size () + v2.size ());
-        cmd_vars_.assign (v1.begin (), v1.end ());
-        cmd_vars_.insert (cmd_vars_.end (), v2.begin (), v2.end ());
-      }
-
-      r = &cmd_vars_;
-    }
-
-    return *r;
-  }
-
-  build2::scope& package_skeleton::
-  load ()
-  {
-    if (ctx_ != nullptr)
-      return *rs_;
-
-    try
-    {
-      using namespace build2;
-
-      auto rsi (bootstrap (*this, merge_cmd_vars ()));
-      scope& rs (*rsi->second.front ());
-
-      // Load project's root.build as well as potentially accumulated reflect
-      // fragment.
-      //
-      // If we have the accumulated reflect fragment, wedge it just before
-      // loading root.build (but after initializing config which may load
-      // config.build and which we wish to override).
-      //
-      // Note that the plan for non-external packages is to extract the
-      // configuration and then load it with config.config.load and this
-      // approach should work for that case too.
-      //
-      function<void (parser&)> pre;
-
-      if (!reflect_frag_.empty ())
-      {
-        pre = [this, &rs] (parser& p)
-        {
-          istringstream is (reflect_frag_);
-          is.exceptions (istringstream::failbit | istringstream::badbit);
-
-          // Note that the fragment is just a bunch of variable assignments
-          // and thus unlikely to cause any errors.
-          //
-          path_name in ("<accumulated-reflect-fragment>");
-          p.parse_buildfile (is, in, &rs, rs);
-        };
-      }
-
-      load_root (rs, pre);
-
-      setup_base (rsi,
-                  out_root_.empty () ? src_root_ : out_root_,
-                  src_root_);
-
-      rs_ = &rs;
-      return rs;
-    }
-    catch (const build2::failed&)
-    {
-      throw failed (); // Assume the diagnostics has already been issued.
-    }
-  }
-
-  void package_skeleton::
-  merge_old_config_vars ()
-  {
-    if (config_srcs_ == nullptr)
-      return;
-
-    assert (reflect_frag_.empty ()); // Too late.
-
-    ctx_ = nullptr; // Reload.
-
-    try
-    {
-      using namespace build2;
-
-      scope& rs (*bootstrap (*this, merge_cmd_vars ())->second.front ());
-
-      // Load project's root.build.
-      //
-      load_root (rs);
-
-      // Extract and merge old user configuration variables from config.build
-      // (or equivalent) into config_vars. Then invalidate loaded state in
-      // order to make them overrides.
-      //
-      auto i (config_vars_.begin ()); // Insert position, see below.
-
-      names storage;
-      for (const config_variable& v: *config_srcs_)
-      {
-        if (v.source != config_source::user)
-          continue;
-
-        using config::variable_origin;
-
-        pair<variable_origin, lookup> ol (config::origin (rs, v.name));
-
-        switch (ol.first)
-        {
-        case variable_origin::override_:
-          {
-            // Already in config_vars.
-            //
-            // @@ TODO: theoretically, this could be an append/prepend
-            //    override(s) and to make this work correctly we would need
-            //    to replace them with an assign override with the final
-            //    value. Maybe one day.
-            //
-            break;
-          }
-        case variable_origin::buildfile:
-          {
-            // Doesn't really matter where we add them though conceptually
-            // feels like old should go before new (and in the original
-            // order).
-            //
-            i = config_vars_.insert (
-              i,
-              serialize_cmdline (v.name, *ol.second, storage)) + 1;
-
-            break;
-          }
-        case variable_origin::undefined:
-        case variable_origin::default_:
-          {
-            // Old user configuration no longer in config.build. We could
-            // complain but that feels overly drastic. Seeing that we will
-            // recalculate the new set of config variable sources, let's
-            // just ignore this (we could issue a warning, but who knows how
-            // many times it will be issued with all this backtracking).
-            //
-            break;
-          }
-        }
-      }
-
-      config_srcs_ = nullptr; // Merged.
-      cmd_vars_.clear ();     // Invalidated.
-      ctx_ = nullptr;         // Drop.
-    }
-    catch (const build2::failed&)
-    {
-      throw failed (); // Assume the diagnostics has already been issued.
-    }
-  }
 }
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index bdf6684..dafb979 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -29,16 +29,16 @@ namespace bpkg
     // If the package is not external, then none of the root directories
     // should be specified.
     //
-    // Note that the options, database, available_package, and config_srcs are
-    // expected to outlive this object.
+    // The disfigure argument should indicate whether the package is being
+    // reconfigured from scratch (--disfigure).
     //
     // The config_vars argument contains configuration variables specified by
     // the user in this bpkg execution. Optional config_srcs is used to
     // extract (from config.build or equivalent) configuration variables
     // specified by the user in previous bpkg executions. It should be NULL if
-    // this is the first build of the package or if it is being disfigured.
-    // The extracted variables are merged with config_vars and the combined
-    // result is returned by collect_config() below.
+    // this is the first build of the package. The extracted variables are
+    // merged with config_vars and the combined result is returned by
+    // collect_config() below.
     //
     // @@ TODO: speaking of the "config.build or equivalent" part, the
     //    equivalent is likely to be extracted configuration (probably saved
@@ -47,6 +47,9 @@ namespace bpkg
     //    (because sometimes we may need to omit it) so most likely it will be
     //    passed as a separate arguments (likely a file path).
     //
+    // Note that the options, database, available_package, and config_srcs are
+    // expected to outlive this object.
+    //
     // Note also that this creates an "unloaded" skeleton and is therefore
     // relatively cheap.
     //
@@ -54,10 +57,19 @@ namespace bpkg
                       database&,
                       const available_package&,
                       strings config_vars,
+                      bool disfigure,
                       const vector<config_variable>* config_srcs,
                       optional<dir_path> src_root,
                       optional<dir_path> out_root);
 
+    // Load the default values and type information for configuration
+    // variables of the package.
+    //
+    // Note: must be called before any evaluate_*() functions.
+    //
+    void
+    load_defaults ();
+
     // For the following evaluate_*() functions assume that the clause belongs
     // to the specified (by index) depends value (used to print its location
     // on failure for an external package).
@@ -133,6 +145,7 @@ namespace bpkg
     const available_package* available_;
 
     strings config_vars_;
+    bool disfigure_;
     const vector<config_variable>* config_srcs_; // NULL if nothing to do or
                                                  // already done.
 
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 6db6845..e450985 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -2874,15 +2874,15 @@ namespace bpkg
                                      ? dir_path (pdb.config) /= nm.string ()
                                      : optional<dir_path> ());
 
-        pkg.skeleton = package_skeleton (options,
-                                         pdb,
-                                         *ap,
-                                         pkg.config_vars,
-                                         (sp != nullptr && !pkg.disfigure
-                                          ? &sp->config_variables
-                                          : nullptr),
-                                         move (src_root),
-                                         move (out_root));
+        pkg.skeleton = package_skeleton (
+          options,
+          pdb,
+          *ap,
+          pkg.config_vars,
+          pkg.disfigure,
+          (sp != nullptr ? &sp->config_variables : nullptr),
+          move (src_root),
+          move (out_root));
       }
       else
         l5 ([&]{trace << "resume " << pkg.available_name_version_db ();});
@@ -11616,9 +11616,8 @@ namespace bpkg
                                            pdb,
                                            *ap,
                                            move (p.config_vars),
-                                           (!p.disfigure
-                                            ? &sp->config_variables
-                                            : nullptr),
+                                           p.disfigure,
+                                           &sp->config_variables,
                                            move (src_root),
                                            move (out_root)),
                          prereqs (),
@@ -11660,9 +11659,8 @@ namespace bpkg
                                          pdb,
                                          *dap,
                                          move (p.config_vars),
-                                         (!p.disfigure
-                                          ? &sp->config_variables
-                                          : nullptr),
+                                         p.disfigure,
+                                         &sp->config_variables,
                                          move (src_root),
                                          move (out_root)),
                        prereqs (),
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index e2ecacb..23ec279 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -591,9 +591,12 @@ namespace bpkg
                                    ? dir_path (db.config) /= p->name.string ()
                                    : optional<dir_path> ());
 
-      // Note that the package could have been disfigured with --keep-config.
-      // Thus, we always pass config variables it may store to the package
-      // skeleton.
+      // Note on the disfigure logic: while we don't know whether the package
+      // has been disfigured with --keep-config or not, it has already been
+      // done physically and if without --keep-config, then config.build has
+      // been removed and config_variables cleaned. As a result, we can just
+      // proceed as disfigure=false and disfigure=true will be taken care
+      // automatically (because then things have been removed/cleaned).
       //
       pkg_configure (o,
                      db,
@@ -605,6 +608,7 @@ namespace bpkg
                                        db,
                                        *ap,
                                        move (vars),
+                                       false /* disfigure */,
                                        &p->config_variables,
                                        move (src_root),
                                        move (out_root)),