Add configuration variable sources to selected packages

11 files changed, 330 insertions, 40 deletions

diff --git a/bpkg/cfg-create.cxx b/bpkg/cfg-create.cxx
index 29de01c..c31b197 100644
--- a/bpkg/cfg-create.cxx
+++ b/bpkg/cfg-create.cxx
@@ -11,6 +11,7 @@
 #include <bpkg/cfg-link.hxx>
 
 using namespace std;
+using namespace butl;
 
 namespace bpkg
 {
@@ -249,7 +250,7 @@ namespace bpkg
       string a (args.next ());
 
       if (a.find ('=') != string::npos)
-        vars.push_back (move (a));
+        vars.push_back (move (trim (a)));
       else if (!a.empty ())
         mods.push_back (move (a));
       else
diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index 9bef69a..972d064 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -384,7 +384,10 @@ namespace bpkg
       //
       reflect_names_.clear ();
       reflect_frag_.clear ();
+
 #if 0
+      // NOTE: see also collect_config() if enabling this.
+      //
       reflect_vars_.clear ();
 #else
       reflect_vars_ = config_vars_;
@@ -547,7 +550,7 @@ namespace bpkg
       // TODO: copy over config_vars_ that are not in the map (case #1).
 #endif
 
-      // Drop the build system state since it needd reloading (some computed
+      // Drop the build system state since it needs reloading (some computed
       // values in root.build may depend on the new configuration values).
       //
       ctx_ = nullptr;
@@ -558,9 +561,53 @@ namespace bpkg
     }
   }
 
-  strings package_skeleton::
-  collect_reflect () &&
+  pair<strings, vector<optional<config_source>>> package_skeleton::
+  collect_config () &&
   {
+    // Note that if the reflect variables list is not empty, then it also
+    // contains the user-specified configuration variables, which all come
+    // first (see above).
+    //
+    size_t nc (config_vars_.size ());
+
+    strings vars (reflect_vars_.empty ()
+                  ? move (config_vars_)
+                  : move (reflect_vars_));
+
+    vector<optional<config_source>> sources;
+
+    if (!vars.empty ())
+    {
+      sources.reserve (vars.size ());
+
+      // Assign the user source to only those user-specified configuration
+      // variables which are project variables (i.e., names start with
+      // config.<project>). Assign the reflect source to all variables that
+      // follow the user-specified configuration variables (which can only be
+      // project variables).
+      //
+      // Note that some user-specified variables may have qualifications
+      // (global, scope, etc) but there is no reason to expect any project
+      // configuration variables to use such qualifications (since they can
+      // only apply to one project). So we skip all qualified variables.
+      //
+      string p ("config." + name ().variable ());
+      size_t n (p.size ());
+
+      for (const string& v: vars)
+      {
+        if (sources.size () < nc)
+        {
+          if (v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr)
+            sources.push_back (config_source::user);
+          else
+            sources.push_back (nullopt);
+        }
+        else
+          sources.push_back (config_source::reflect);
+      }
+    }
+
     assert (db_ != nullptr);
 
     ctx_ = nullptr; // In case we only had conditions.
@@ -568,9 +615,7 @@ namespace bpkg
     db_ = nullptr;
     available_ = nullptr;
 
-    return reflect_vars_.empty ()
-      ? move (config_vars_)
-      : move (reflect_vars_);
+    return make_pair (move (vars), move (sources));
   }
 
   build2::scope& package_skeleton::
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index d2a848c..aaa9b8c 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -56,15 +56,17 @@ namespace bpkg
     void
     evaluate_reflect (const string&, size_t depends_index);
 
-    // Return the accumulated reflect values.
+    // Return the accumulated reflect values together with their sources (the
+    // resulting vectors are parallel).
     //
-    // Note that the result is merged with config_vars and you should be used
-    // instead rather than in addition to config_vars.
+    // Note that the result is merged with config_vars and should be used
+    // instead rather than in addition to config_vars. The source of
+    // configuration variables which are not the project variables is absent.
     //
     // Note also that this should be the final call on this object.
     //
-    strings
-    collect_reflect () &&;
+    pair<strings, vector<optional<config_source>>>
+    collect_config () &&;
 
     const package_name&
     name () const {return available_->id.name;}
diff --git a/bpkg/package.cxx b/bpkg/package.cxx
index 4b9cdc3..3a356f8 100644
--- a/bpkg/package.cxx
+++ b/bpkg/package.cxx
@@ -529,6 +529,28 @@ namespace bpkg
     return lazy_shared_ptr<selected_package> (ddb, move (prerequisite));
   }
 
+  string
+  to_string (config_source s)
+  {
+    switch (s)
+    {
+    case config_source::user:      return "user";
+    case config_source::dependent: return "dependent";
+    case config_source::reflect:   return "reflect";
+    }
+
+    return string (); // Should never reach.
+  }
+
+  config_source
+  to_config_source (const string& s)
+  {
+         if (s == "user")      return config_source::user;
+    else if (s == "dependent") return config_source::dependent;
+    else if (s == "reflect")   return config_source::reflect;
+    else throw invalid_argument ("invalid config source '" + s + "'");
+  }
+
   shared_ptr<available_package>
   make_available (const common_options& options,
                   database& db,
diff --git a/bpkg/package.hxx b/bpkg/package.hxx
index fbf8643..7dc8e11 100644
--- a/bpkg/package.hxx
+++ b/bpkg/package.hxx
@@ -27,7 +27,7 @@
 //
 #define DB_SCHEMA_VERSION_BASE 7
 
-#pragma db model version(DB_SCHEMA_VERSION_BASE, 17, closed)
+#pragma db model version(DB_SCHEMA_VERSION_BASE, 18, closed)
 
 namespace bpkg
 {
@@ -1066,6 +1066,30 @@ namespace bpkg
     to(bpkg::_selected_package_ref (?))           \
     from(std::move (?).to_ptr (*db))
 
+  enum class config_source
+  {
+    user,      // User configuration specified on command line.
+    dependent, // Dependent-imposed configuration from prefer/require clauses.
+    reflect    // Package-reflected configuration from reflect clause.
+  };
+
+  string
+  to_string (config_source);
+
+  config_source
+  to_config_source (const string&); // May throw std::invalid_argument.
+
+  #pragma db map type(config_source) as(string) \
+    to(to_string (?))                           \
+    from(bpkg::to_config_source (?))
+
+  #pragma db value
+  struct config_variable
+  {
+    string        name;
+    config_source source;
+  };
+
   #pragma db object pointer(shared_ptr) session
   class selected_package
   {
@@ -1154,6 +1178,8 @@ namespace bpkg
 
     package_prerequisites prerequisites;
 
+    vector<config_variable> config_variables;
+
   public:
     bool
     system () const
@@ -1229,6 +1255,8 @@ namespace bpkg
     #pragma db member(prerequisites) id_column("package") \
       key_column("") value_column("")
 
+    #pragma db member(config_variables) id_column("package") value_column("")
+
     // 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 d2006f9..904d0f6 100644
--- a/bpkg/package.xml
+++ b/bpkg/package.xml
@@ -1,4 +1,25 @@
 <changelog xmlns="http://www.codesynthesis.com/xmlns/odb/changelog" database="sqlite" version="1">
+  <changeset version="18">
+    <add-table name="main.selected_package_config_variables" kind="container">
+      <column name="package" type="TEXT" null="true" options="COLLATE NOCASE"/>
+      <column name="index" type="INTEGER" null="true"/>
+      <column name="name" type="TEXT" null="true"/>
+      <column name="source" type="TEXT" null="true"/>
+      <foreign-key name="package_fk" on-delete="CASCADE">
+        <column name="package"/>
+        <references table="main.selected_package">
+          <column name="name"/>
+        </references>
+      </foreign-key>
+      <index name="selected_package_config_variables_package_i">
+        <column name="package"/>
+      </index>
+      <index name="selected_package_config_variables_index_i">
+        <column name="index"/>
+      </index>
+    </add-table>
+  </changeset>
+
   <changeset version="17">
     <alter-table name="main.selected_package">
       <add-column name="buildfiles_checksum" type="TEXT" null="true"/>
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 8f56fc8..60674b6 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -4578,7 +4578,7 @@ namespace bpkg
           fail << "unexpected options group for configuration variable '"
                << v << "'";
 
-        cvars.push_back (move (v));
+        cvars.push_back (move (trim (v)));
       }
 
       if (!cvars.empty () && !sep)
@@ -4619,7 +4619,7 @@ namespace bpkg
               if (a.find ('=') == string::npos)
                 fail << "unexpected group argument '" << a << "'";
 
-              cvs.push_back (move (a));
+              cvs.push_back (move (trim (a)));
             }
           }
 
@@ -8177,12 +8177,21 @@ namespace bpkg
         // resulting package skeleton and dependency list for optimization
         // (not to re-evaluate enable conditions, etc).
         //
+        // Note that we may not collect the package prerequisites builds if
+        // the package is already configured but we still need to reconfigure
+        // it due, for example, to an upgrade of its dependency. In this case
+        // we pass to pkg_configure() the newly created package skeleton which
+        // contains the package configuration variables specified on the
+        // command line but (naturally) no reflection configuration variables.
+        // Note, however, that in this case pkg_configure() call will evaluate
+        // the reflect clauses itself and so the proper reflection variables
+        // will still end up in the package configuration.
+        //
         // @@ Note that if we ever allow the user to override the alternative
         //    selection, this will break (and also if the user re-configures
         //    the package manually). Maybe that a good reason not to allow
         //    this? Or we could store this information in the database.
         //
-
         if (p.skeleton)
         {
           assert (p.dependencies);
@@ -8198,6 +8207,8 @@ namespace bpkg
         }
         else
         {
+          assert (sp != nullptr); // See above.
+
           optional<dir_path> src_root (p.external_dir ());
 
           optional<dir_path> out_root (
diff --git a/bpkg/pkg-command.cxx b/bpkg/pkg-command.cxx
index 0ff6a0d..e07d801 100644
--- a/bpkg/pkg-command.cxx
+++ b/bpkg/pkg-command.cxx
@@ -213,7 +213,7 @@ namespace bpkg
         if (args.group ().more ())
           fail << "unexpected options group for variable '" << a << "'";
 
-        cvars.push_back (move (a));
+        cvars.push_back (move (trim (a)));
       }
       else
       {
@@ -228,7 +228,7 @@ namespace bpkg
           if (a.find ('=') == string::npos)
             fail << "unexpected group argument '" << a << "'";
 
-          vars.push_back (move (a));
+          vars.push_back (move (trim (a)));
         }
 
         pkg_args.push_back (pkg_arg {move (n), move (vars)});
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index 772cc67..0519f7c 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -18,16 +18,32 @@ using namespace butl;
 
 namespace bpkg
 {
-  // Given dependencies of a package, return its prerequisite packages and
+  // Given dependencies of a package, return its prerequisite packages,
   // configuration variables that resulted from selection of these
-  // prerequisites (import, reflection, etc). See pkg_configure() for the
-  // semantics of the dependency list. Fail if for some of the dependency
-  // alternative lists there is no satisfactory alternative (all its
-  // dependencies are configured, satisfy the respective constraints, etc).
+  // prerequisites (import, reflection, etc), and sources of the configuration
+  // variables resulted from evaluating the reflect clauses. See
+  // pkg_configure() for the semantics of the dependency list. Fail if for
+  // some of the dependency alternative lists there is no satisfactory
+  // alternative (all its dependencies are configured, satisfy the respective
+  // constraints, etc).
   //
+  struct configure_prerequisites_result
+  {
+    package_prerequisites   prerequisites;
+    strings                 config_variables; // Note: name and value.
+
+    // Only contains sources of configuration variables collected using the
+    // package skeleton, excluding those user-specified variables which are
+    // not the project variables for the specified package (module
+    // configuration variables, etc). Thus, it is not parallel to the
+    // variables member.
+    //
+    vector<config_variable> config_sources; // Note: name and source.
+  };
+
   // Note: loads selected packages.
   //
-  static pair<package_prerequisites, vector<string>>
+  static configure_prerequisites_result
   pkg_configure_prerequisites (const common_options& o,
                                database& db,
                                transaction&,
@@ -36,8 +52,9 @@ namespace bpkg
                                bool simulate,
                                const function<find_database_function>& fdb)
   {
-    package_prerequisites pps;
-    vector<string> cvs;
+    package_prerequisites   prereqs;
+    strings                 vars;
+    vector<config_variable> srcs;
 
     for (size_t di (0); di != deps.size (); ++di)
     {
@@ -114,7 +131,7 @@ namespace bpkg
           const package_name& pn (pr.first.object_id ());
           const optional<version_constraint>& pc (pr.second);
 
-          auto p (pps.emplace (pr.first, pc));
+          auto p (prereqs.emplace (pr.first, pc));
 
           // Currently we can only capture a single constraint, so if we
           // already have a dependency on this package and one constraint is
@@ -167,8 +184,8 @@ namespace bpkg
                 //    bootstrap` in their manifest.
                 //
                 dir_path od (sp->effective_out_root (pdb.config));
-                cvs.push_back ("config.import." + sp->name.variable () +
-                               "='" + od.representation () + "'");
+                vars.push_back ("config.import." + sp->name.variable () +
+                                "='" + od.representation () + "'");
               }
             }
           }
@@ -192,18 +209,36 @@ namespace bpkg
     //
     if (!simulate)
     {
-      strings rvs (move (ps).collect_reflect ());
+      auto rvs (move (ps).collect_config ());
 
-      if (cvs.empty ())
-        cvs = move (rvs);
-      else
+      strings&                         vs (rvs.first);
+      vector<optional<config_source>>& ss (rvs.second);
+
+      if (!vs.empty ())
       {
-        for (string& cv: rvs)
-          cvs.push_back (move (cv));
+        vars.reserve (vars.size () + vs.size ());
+
+        for (size_t i (0); i != vs.size (); ++i)
+        {
+          string&                        v (vs[i]);
+          const optional<config_source>& s (ss[i]);
+
+          if (s)
+          {
+            size_t p (v.find_first_of ("=+ \t"));
+            assert (p != string::npos);
+
+            srcs.push_back (config_variable {string (v, 0, p), *s});
+          }
+
+          vars.push_back (move (v));
+        }
       }
     }
 
-    return make_pair (move (pps), move (cvs));
+    return configure_prerequisites_result {move (prereqs),
+                                           move (vars),
+                                           move (srcs)};
   }
 
   void
@@ -241,7 +276,7 @@ namespace bpkg
     //
     assert (p->prerequisites.empty ());
 
-    pair<package_prerequisites, vector<string>> cpr (
+    configure_prerequisites_result cpr (
       pkg_configure_prerequisites (o,
                                    db,
                                    t,
@@ -250,7 +285,7 @@ namespace bpkg
                                    simulate,
                                    fdb));
 
-    p->prerequisites = move (cpr.first);
+    p->prerequisites = move (cpr.prerequisites);
 
     if (!simulate)
     {
@@ -269,11 +304,42 @@ namespace bpkg
 
       l4 ([&]{trace << "buildspec: " << bspec;});
 
+      // Deduce the configuration variables which are not reflected anymore
+      // and disfigure them.
+      //
+      string dvar;
+      for (const config_variable& cv: p->config_variables)
+      {
+        if (cv.source == config_source::reflect)
+        {
+          const vector<config_variable>& ss (cpr.config_sources);
+          auto i (find_if (ss.begin (), ss.end (),
+                           [&cv] (const config_variable& v)
+                           {
+                             return v.name == cv.name;
+                           }));
+
+          if (i == ss.end ())
+          {
+            if (dvar.empty ())
+              dvar = "config.config.disfigure=";
+            else
+              dvar += ' ';
+
+            dvar += cv.name;
+          }
+        }
+      }
+
       // Configure.
       //
       try
       {
-        run_b (o, verb_b::quiet, cpr.second, bspec);
+        run_b (o,
+               verb_b::quiet,
+               cpr.config_variables,
+               (!dvar.empty () ? dvar.c_str () : nullptr),
+               bspec);
       }
       catch (const failed&)
       {
@@ -298,6 +364,8 @@ namespace bpkg
                        false /* simulate */);
         throw;
       }
+
+      p->config_variables = move (cpr.config_sources);
     }
 
     p->out_root = out_root.leaf ();
@@ -368,7 +436,7 @@ namespace bpkg
       }
 
       if (!sep && a.find ('=') != string::npos)
-        vars.push_back (move (a));
+        vars.push_back (move (trim (a)));
       else if (n.empty ())
         n = move (a);
       else
diff --git a/bpkg/pkg-disfigure.cxx b/bpkg/pkg-disfigure.cxx
index bae2a4e..690c3e1 100644
--- a/bpkg/pkg-disfigure.cxx
+++ b/bpkg/pkg-disfigure.cxx
@@ -198,6 +198,9 @@ namespace bpkg
              << "use 'pkg-purge' to remove";
         throw;
       }
+
+      if (disfigure)
+        p->config_variables.clear ();
     }
 
     p->out_root = nullopt;
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index 36ffb9d..8ee0ee1 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -4051,6 +4051,95 @@ test.options += --no-progress
             $pkg_drop fax
           }
         }
+
+        : reconfigure-reflect-vars
+        :
+        {
+          $clone_cfg;
+          cp -rp ../fax/ ./;
+
+          $* config.fax.libbiz=true -- fax/ 2>>~"%EOE%";
+            fetched $backend_dep
+            unpacked $backend_dep
+            fetched libbiz/1.0.0
+            unpacked libbiz/1.0.0
+            using fax/1.0.0 \(external\)
+            configured $backend_dep
+            configured libbiz/1.0.0
+            configured fax/1.0.0
+            %info: .+fax.+ is up to date%
+            updated fax/1.0.0
+            EOE
+
+          $pkg_status -r >>"EOO";
+            !fax configured !1.0.0
+              $backend_configured
+              libbiz configured 1.0.0
+            EOO
+
+          cat cfg/fax/build/config.build >>~"%EOO%";
+            %.*
+            config.fax.backend = $backend
+            config.fax.libbiz = true
+            %config.fax.extras = '.+'%
+            config.fax.libbox = false
+            EOO
+
+          # While at it, make sure none of the reflect variables are
+          # unexpectedly wiped out on reconfiguration due to the dependency
+          # upgrade.
+          #
+          $* fax/ "?sys:$backend/*" 2>>~"%EOE%";
+            disfigured fax/1.0.0
+            %disfigured $backend/.+%
+            %purged $backend/.+%
+            configured sys:$backend/*
+            configured fax/1.0.0
+            %info: .+fax.+ is up to date%
+            updated fax/1.0.0
+            EOE
+
+          $pkg_status -r >>~"%EOO%";
+            !fax configured !1.0.0
+            %  $backend configured,system .+%
+              libbiz configured 1.0.0
+            EOO
+
+          cat cfg/fax/build/config.build >>~"%EOO%";
+            %.*
+            config.fax.backend = $backend
+            config.fax.libbiz = true
+            %config.fax.extras = '.+'%
+            config.fax.libbox = false
+            EOO
+
+          # Now make sure that dependency clauses re-evaluation is properly
+          # reflected in the configuration.
+          #
+          $* config.fax.libbiz=false -- fax/ 2>>~"%EOE%";
+            disfigured fax/1.0.0
+            disfigured libbiz/1.0.0
+            purged libbiz/1.0.0
+            configured fax/1.0.0
+            %info: .+fax.+ is up to date%
+            updated fax/1.0.0
+            EOE
+
+          $pkg_status -r >>~"%EOO%";
+            !fax configured !1.0.0
+            %  $backend configured,system .+%
+            EOO
+
+          cat cfg/fax/build/config.build >>~"%EOO%";
+            %.*
+            config.fax.backend = $backend
+            config.fax.libbiz = false
+            config.fax.extras = [null]
+            config.fax.libbox = false
+            EOO
+
+          $pkg_drop fax
+        }
       }
 
       : evaluate-reflect-vars