Implementation of evaluate_require() plus other tweaks

5 files changed, 358 insertions, 54 deletions

diff --git a/bpkg/package-configuration.cxx b/bpkg/package-configuration.cxx
index fb742e8..e74be64 100644
--- a/bpkg/package-configuration.cxx
+++ b/bpkg/package-configuration.cxx
@@ -16,7 +16,12 @@ namespace bpkg
     pair<size_t, size_t> pos,
     const small_vector<reference_wrapper<package_skeleton>, 1>& depcs)
   {
-    dependent_config_variable_values old_cfgs;
+    pos.first--; pos.second--; // Convert to 0-base.
+
+    const dependency_alternative& da (
+      dept.available.get ().dependencies[pos.first][pos.second]);
+
+    assert (da.require || da.prefer);
 
     // Step 1: save a snapshot of the old configuration while unsetting values
     // that have this dependent as the originator and reloading the defaults.
@@ -24,6 +29,22 @@ namespace bpkg
     // While at it, also collect the configurations to pass to dependent's
     // evaluate_*() calls.
     //
+    // Our assumptions regarding require:
+    //
+    // - Can only set bool configuration variables and only to true.
+    //
+    // - Should not have any conditions on the state of other configuration
+    //   variables, including their origin (but can have other conditions,
+    //   for example on the target platform).
+    //
+    // This means that we don't need to set the default values, but will need
+    // the type information as well as overrides. So what we will do is only
+    // call reload_defaults() for the first time to load types/override. Note
+    // that this assumes the set of configuration variables cannot change
+    // based on the values of other configuration variables (we have a note
+    // in the manual instructing the user not to do this).
+    //
+    dependent_config_variable_values old_cfgs;
     package_skeleton::dependency_configurations depc_cfgs;
     depc_cfgs.reserve (depcs.size ());
 
@@ -41,7 +62,10 @@ namespace bpkg
               dependent_config_variable_value {
                 v.name, move (v.value), move (*v.dependent)});
 
+            // Note that we will not reload it to default in case of require.
+            //
             v.origin = variable_origin::undefined;
+            v.value = nullopt;
             v.dependent = nullopt;
           }
           else
@@ -50,18 +74,14 @@ namespace bpkg
         }
       }
 
-      depc.reload_defaults (cfg);
+      if (da.prefer || cfg.empty ())
+        depc.reload_defaults (cfg);
 
       depc_cfgs.push_back (cfg);
     }
 
     // Step 2: execute the prefer/accept or requires clauses.
     //
-    pos.first--; pos.second--; // Convert to 0-base.
-
-    const dependency_alternative& da (
-      dept.available.get ().dependencies[pos.first][pos.second]);
-
     if (!(da.require
           ? dept.evaluate_require (depc_cfgs, *da.require, pos.first)
           : dept.evaluate_prefer_accept (depc_cfgs,
diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx
index 55a43a5..ba7240e 100644
--- a/bpkg/package-configuration.hxx
+++ b/bpkg/package-configuration.hxx
@@ -55,6 +55,11 @@ namespace bpkg
   class package_configuration: public vector<config_variable_value>
   {
   public:
+    package_key package;
+
+    explicit
+    package_configuration (package_key p): package (move (p)) {}
+
     config_variable_value*
     find (const string& name)
     {
@@ -78,10 +83,24 @@ namespace bpkg
     }
   };
 
+  class package_configurations: public small_vector<package_configuration, 1>
+  {
+  public:
+    package_configuration&
+    operator[] (const package_key& p)
+    {
+      auto i (find_if (begin (), end (),
+                       [&p] (const package_configuration& pc)
+                       {
+                         return pc.package == p;
+                       }));
+      if (i != end ())
+        return *i;
 
-  // @@ Maybe redo as small_vector?
-  //
-  using package_configurations = map<package_key, package_configuration>;
+      push_back (package_configuration (p));
+      return back ();
+    }
+  };
 
 
   // A subset of config_variable_value for variable values set by the
diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index 0d07981..24f155c 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -328,7 +328,7 @@ namespace bpkg
       // construction (in evaluate_{prefer_accept,require}()): we do not add
       // as dependent variables that have the override origin.
       //
-      package_configuration old (move (cfg));
+      package_configuration old (move (cfg)); cfg.package = move (old.package);
 
       scope& rs (
         *bootstrap (
@@ -356,6 +356,11 @@ namespace bpkg
       // variable), then the saved variables map seem like the natural place
       // to keep this information.
       //
+      // @@ One potentially-bogus config variable could be config.*.develop.
+      //    Would have been nice not to drag it around if not used by the
+      //    package. And, could be helpful to warn that configuration variable
+      //    does not exist.
+      //
       string p ("config." + name ().variable ());
       size_t n (p.size ());
 
@@ -507,7 +512,7 @@ namespace bpkg
   // the temp directory is not cleaned in case of an error. Maybe one day.
   //
   static void
-  depends_location (const diag_record& dr,
+  depends_location (const build2::diag_record& dr,
                     const path& mf,
                     size_t depends_index)
   {
@@ -527,10 +532,10 @@ namespace bpkg
         {
           if (nv.name == "depends" && i++ == depends_index)
           {
-            dr << info (location (p.name (),
-                                  nv.value_line,
-                                  nv.value_column))
-               << "depends value defined here";
+            dr << build2::info (build2::location (mf,
+                                                  nv.value_line,
+                                                  nv.value_column))
+               << "in depends value defined here";
             break;
           }
         }
@@ -547,6 +552,7 @@ namespace bpkg
     {
       using namespace build2;
       using build2::fail;
+      using build2::info;
       using build2::endf;
 
       scope& rs (load ());
@@ -564,7 +570,7 @@ namespace bpkg
       uint64_t il (1);
 
       auto df = build2::make_diag_frame (
-        [&cond, &rs, depends_index] (const diag_record& dr)
+        [&cond, &rs, depends_index] (const build2::diag_record& dr)
         {
           dr << info << "enable condition: (" << cond << ")";
 
@@ -655,6 +661,7 @@ namespace bpkg
       //
       using namespace build2;
       using build2::fail;
+      using build2::info;
       using build2::endf;
 
       scope& rs (load ());
@@ -666,16 +673,16 @@ namespace bpkg
       uint64_t il (1);
 
       auto df = build2::make_diag_frame (
-        [&refl, &rs, depends_index] (const diag_record& dr)
+        [&refl, &rs, depends_index] (const build2::diag_record& dr)
         {
           // Probably safe to assume a one-line fragment contains a variable
           // assignment.
           //
           if (refl.find ('\n') == string::npos)
-            dr << info << "reflect variable: " << refl;
+            dr << info << "reflect variable: " << trim (string (refl));
           else
-            dr << info << "reflect fragment:\n"
-               << refl;
+            dr << info << "reflect clause:\n"
+               << trim_right (string (refl));
 
           // For external packages we have the manifest so print the location
           // of the depends value in questions.
@@ -693,7 +700,7 @@ namespace bpkg
       // filter out unchanged on the second.
       //
       auto& vp (rs.var_pool ());
-      const variable& ns (vp.insert ("config." + name ().variable ()));
+      string ns ("config." + name ().variable ());
 
       struct value_data
       {
@@ -823,6 +830,9 @@ namespace bpkg
         //    config.hello.backend = foo  # reflect
         //    config.hello.backend += bar # user
         //
+
+        // @@ Can't we redo it via origin() call like in other places?
+        //
         pair<lookup, size_t> org {lookup {val, var, rs.vars}, 1 /* depth */};
         pair<lookup, size_t> ovr;
 
@@ -927,6 +937,7 @@ namespace bpkg
       using namespace build2;
       using config::variable_origin;
       using build2::fail;
+      using build2::info;
       using build2::endf;
 
       // This is what needs to happen to the variables of different origins in
@@ -953,10 +964,10 @@ namespace bpkg
         uint64_t il (1);
 
         auto df = build2::make_diag_frame (
-          [&prefer, &rs, depends_index] (const diag_record& dr)
+          [&prefer, &rs, depends_index] (const build2::diag_record& dr)
           {
-            dr << info << "prefer fragment:\n"
-               << prefer;
+            dr << info << "prefer clause:\n"
+               << trim_right (string (prefer));
 
             // For external packages we have the manifest so print the
             // location of the depends value in questions.
@@ -970,6 +981,34 @@ namespace bpkg
         lexer l (is, in, il /* start line */);
         parser p (rs.ctx);
         p.parse_buildfile (l, &rs, rs);
+
+        // Check if the dependent set any stray configuration variables.
+        //
+        for (package_configuration& cfg: cfgs)
+        {
+          string ns ("config." + cfg.package.name.variable ());
+
+          for (auto p (rs.vars.lookup_namespace (ns));
+               p.first != p.second;
+               ++p.first)
+          {
+            const variable& var (p.first->first);
+
+            // This can be one of the overrides (__override, __prefix, etc),
+            // which we skip.
+            //
+            if (var.override ())
+              continue;
+
+            if (cfg.find (var.name) == nullptr)
+            {
+              fail << "package " << cfg.package.name << " has no "
+                   << "configuration variable " << var.name <<
+                info << var.name << " set in require clause of dependent "
+                   << key.string ();
+            }
+          }
+        }
       }
 
       // Evaluate the accept clause.
@@ -983,7 +1022,7 @@ namespace bpkg
         uint64_t il (1);
 
         auto df = build2::make_diag_frame (
-          [&accept, &rs, depends_index] (const diag_record& dr)
+          [&accept, &rs, depends_index] (const build2::diag_record& dr)
           {
             dr << info << "accept condition: (" << accept << ")";
 
@@ -1022,11 +1061,28 @@ namespace bpkg
       {
         for (package_configuration& cfg: cfgs)
         {
-          for (config_variable_value& v: cfg)
+          string ns ("config." + cfg.package.name.variable ());
+
+          for (auto p (rs.vars.lookup_namespace (ns));
+               p.first != p.second;
+               ++p.first)
           {
-            pair<variable_origin, lookup> ol (config::origin (rs, v.name));
+            const variable& var (p.first->first);
 
-            // An override cannot become non-override. And a non-override
+            if (var.override ())
+              continue;
+
+            const variable_map::value_data& val (p.first->second);
+
+            pair<variable_origin, lookup> ol (
+              config::origin (rs,
+                              var,
+                              pair<lookup, size_t> {
+                                lookup {val, var, rs.vars}, 1 /* depth */}));
+
+            config_variable_value& v (*cfg.find (var.name));
+
+            // An override cannot become a non-override. And a non-override
             // cannot become an override.
             //
             assert ((v.origin == variable_origin::override_) ==
@@ -1041,7 +1097,7 @@ namespace bpkg
                 // default/undefine -> buildfile -- override dependency default
                 // buildfile        -> buildfile -- override other dependent
                 //
-                optional<names> val (reverse_value (*ol.second));
+                optional<names> ns (reverse_value (val));
 
                 if (v.origin == variable_origin::buildfile)
                 {
@@ -1049,13 +1105,13 @@ namespace bpkg
                   // (even if the value was technically "overwritten" by this
                   // dependent).
                   //
-                  if (v.value == val)
+                  if (v.value == ns)
                     break;
                 }
                 else
                   v.origin = variable_origin::buildfile;
 
-                v.value = move (val);
+                v.value = move (ns);
                 v.dependent = key; // We are the originating dependent.
                 break;
               }
@@ -1073,6 +1129,10 @@ namespace bpkg
                 // @@ We may still need to see if there is original set
                 //    by this dependent and if so, reflect the overridden
                 //    value.
+                //
+                //    Maybe we don't need the version: the presence of the
+                //    value in rs is enough.
+                //
                 break;
               }
             }
@@ -1095,14 +1155,205 @@ namespace bpkg
   }
 
   bool package_skeleton::
-  evaluate_require (const dependency_configurations&,
-                    const string&, size_t /*depends_index*/)
+  evaluate_require (const dependency_configurations& cfgs,
+                    const string& require, size_t depends_index)
   {
-    // How will we square the implied accept logic with user overrides?
-    // Feels like the only way is to not enter them as overrides and then
-    // see of any of them override manually? Nasty.
+    try
+    {
+      using namespace build2;
+      using config::variable_origin;
+      using build2::fail;
+      using build2::info;
+      using build2::endf;
+
+      // A require clause can only set bool configuration variables and only
+      // to true and may not have any conditions on other configuration
+      // variables (including their origin). As a result, we don't need to set
+      // the default (or other dependent) values, but will need the type
+      // information as well as overrides (see up_negotiate_configuration()
+      // for details).
+      //
+      scope& rs (load (cfgs, false /* defaults */));
+
+      // Evaluate the require clause.
+      //
+      istringstream is (require);
+      is.exceptions (istringstream::failbit | istringstream::badbit);
+
+      path_name in ("<depends-require-clause>");
+      uint64_t il (1);
+
+      // Note: keep the diag frame beyond parse_buildfile().
+      //
+      auto df = build2::make_diag_frame (
+        [&require, &rs, depends_index] (const build2::diag_record& dr)
+        {
+          dr << info << "require clause:\n"
+             << trim_right (string (require));
+
+          // For external packages we have the manifest so print the
+          // location of the depends value in questions.
+          //
+          if (!rs.out_eq_src ())
+            depends_location (dr,
+                              rs.src_path () / manifest_file,
+                              depends_index);
+        });
+
+      lexer l (is, in, il /* start line */);
+      parser p (rs.ctx);
+      p.parse_buildfile (l, &rs, rs);
+
+      // First determine if acceptable.
+      //
+      // While at it, also check for stray variables and enforce all the
+      // require restrictions (bool, set to true, etc).
+      //
+      bool r (true);
+      for (package_configuration& cfg: cfgs)
+      {
+        string ns ("config." + cfg.package.name.variable ());
+
+        // Note that because we didn't set any default (or other dependent)
+        // values, all the values we see are set by this dependent.
+        //
+        for (auto p (rs.vars.lookup_namespace (ns));
+             p.first != p.second;
+             ++p.first)
+        {
+          const variable& var (p.first->first);
+
+          // This can be one of the overrides (__override, __prefix, etc),
+          // which we skip.
+          //
+          if (var.override ())
+            continue;
+
+          const config_variable_value* v (cfg.find (var.name));
+
+          if (v == nullptr)
+          {
+            fail << "package " << cfg.package.name << " has no configuration "
+                 << "variable " << var.name <<
+              info << var.name << " set in require clause of dependent "
+                 << key.string ();
+          }
+
+          if (!v->type || *v->type != "bool")
+          {
+            fail << "configuration variable " << var.name << " is not of "
+                 << "bool type" <<
+              info << var.name << " set in require clause of dependent "
+                 << key.string ();
+          }
+
+          const value& val (p.first->second);
+
+          if (!cast_false<bool> (val))
+          {
+            fail << "configuration variable " << var.name << " is not set "
+                 << "to true" <<
+              info << var.name << " set in require clause of dependent "
+                 << key.string ();
+          }
+
+          // The only situation where the result would not be acceptable is if
+          // one of the values were overridden to false.
+          //
+          pair<variable_origin, lookup> ol (
+            config::origin (rs,
+                            var,
+                            pair<lookup, size_t> {
+                              lookup {val, var, rs.vars}, 1 /* depth */}));
+
+          // An override cannot become a non-override. And a non-override
+          // cannot become an override.
+          //
+          assert ((v->origin == variable_origin::override_) ==
+                  (ol.first == variable_origin::override_));
+
+          if (ol.first == variable_origin::override_)
+          {
+            if (!cast_false<bool> (*ol.second))
+              r = false;
+          }
+        }
+      }
+
+      // If acceptable, update the configuration with the new values, if any.
+      //
+      // Note that we cannot easily combine this loop with the above because
+      // we should not modify configurations if the result is not acceptable.
+      //
+      // @@ TODO: we also need to save the subset of values that were "set"
+      //    (for some definition of "set") by this dependent to be reflected
+      //    to further clauses.
+      //
+      if (r)
+      {
+        for (package_configuration& cfg: cfgs)
+        {
+          string ns ("config." + cfg.package.name.variable ());
+
+          for (auto p (rs.vars.lookup_namespace (ns));
+               p.first != p.second;
+               ++p.first)
+          {
+            const variable& var (p.first->first);
+
+            if (var.override ())
+              continue;
+
+            config_variable_value& v (*cfg.find (var.name));
 
-    return false; // @@ TODO
+            if (v.origin != variable_origin::override_)
+            {
+              // Possible transitions:
+              //
+              // default/undefine -> buildfile -- override dependency default
+              // buildfile        -> buildfile -- override other dependent
+              //
+              optional<names> ns (names {build2::name ("true")});
+
+              // @@ TODO: reflect (before continue)
+
+              if (v.origin == variable_origin::buildfile)
+              {
+                // If unchanged, then we keep the old originating dependent
+                // (even if the value was technically "overwritten" by this
+                // dependent).
+                //
+                if (v.value == ns)
+                  continue;
+              }
+              else
+                v.origin = variable_origin::buildfile;
+
+              v.value = move (ns);
+              v.dependent = key; // We are the originating dependent.
+            }
+            else
+            {
+              // @@ TODO: reflect (true). Could probably handle with the
+              //    same code as !=override case.
+            }
+          }
+        }
+      }
+
+      // Drop the build system state since it needs reloading (while it may
+      // seem safe for us to keep the state since we didn't set any defaults,
+      // we may have overrides that the clause did not set, so let's drop it
+      // for good measure and also to keep things simple).
+      //
+      ctx_ = nullptr;
+
+      return r;
+    }
+    catch (const build2::failed&)
+    {
+      throw failed (); // Assume the diagnostics has already been issued.
+    }
   }
 
   pair<strings, vector<config_variable>> package_skeleton::
@@ -1401,7 +1652,7 @@ namespace bpkg
   }
 
   build2::scope& package_skeleton::
-  load (const dependency_configurations& cfgs)
+  load (const dependency_configurations& cfgs, bool defaults)
   {
     if (ctx_ != nullptr)
     {
@@ -1471,7 +1722,8 @@ namespace bpkg
       {
         scope& rs;
         const dependency_configurations& cfgs;
-      } d {rs, cfgs};
+        bool defaults;
+      } d {rs, cfgs, defaults};
 
       if (!reflect_frag_.empty () || !cfgs.empty ())
       {
@@ -1509,19 +1761,25 @@ namespace bpkg
               case variable_origin::default_:
               case variable_origin::buildfile:
                 {
-                  auto& val (
-                    static_cast<variable_map::value_data&> (
-                      rs.assign (var)));
-
-                  if (v.value)
-                    val.assign (names (*v.value), &var);
+                  if (d.defaults)
+                  {
+                    auto& val (
+                      static_cast<variable_map::value_data&> (
+                        rs.assign (var)));
+
+                    if (v.value)
+                      val.assign (names (*v.value), &var);
+                    else
+                      val = nullptr;
+
+                    if (v.origin == variable_origin::default_)
+                      val.extra = 1;
+
+                    v.version = val.version;
+                  }
                   else
-                    val = nullptr;
-
-                  if (v.origin == variable_origin::default_)
-                    val.extra = 1;
+                    v.version = 0;
 
-                  v.version = val.version;
                   break;
                 }
               case variable_origin::undefined:
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index 1c4ae32..e949784 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -178,11 +178,14 @@ namespace bpkg
     //
     // Call this function before evaluating every clause.
     //
-    // If dependency configurations are specified, then set their values and
-    // save the resulting versions in config_variable_value::version.
+    // If dependency configurations are specified, then typify the variables
+    // and set their values saving the resulting value versions in
+    // config_variable_value::version. If defaults is false, then only typify
+    // the variables and set overrides without setting the default/buildfile
+    // values.
     //
     build2::scope&
-    load (const dependency_configurations& = {});
+    load (const dependency_configurations& = {}, bool defaults = true);
 
     // Merge command line variable overrides into one list (normally to be
     // passed to bootstrap()).
diff --git a/bpkg/utility.hxx b/bpkg/utility.hxx
index 2dcf46d..597800c 100644
--- a/bpkg/utility.hxx
+++ b/bpkg/utility.hxx
@@ -48,6 +48,10 @@ namespace bpkg
   using butl::digit;
   using butl::xdigit;
 
+  using butl::trim;
+  using butl::trim_left;
+  using butl::trim_right;
+
   using butl::make_guard;
   using butl::make_exception_guard;