Change default for unused config.import.* variables from drop to keep

4 files changed, 94 insertions, 34 deletions

diff --git a/libbuild2/config/init.cxx b/libbuild2/config/init.cxx
index bcaea83..e1c1dd0 100644
--- a/libbuild2/config/init.cxx
+++ b/libbuild2/config/init.cxx
@@ -40,14 +40,15 @@ namespace build2
 
       auto& vp (rs.ctx.var_pool.rw (rs));
 
-      // While config.import (see below) could theoretically be specified in a
-      // buildfile, config.export is expected to always be specified as a
-      // command line override.
+      // While config.import could theoretically be specified in a buildfile,
+      // config.export is expected to always be specified as a command line
+      // override.
       //
-      // Note: must be entered during bootstrap since we need it in
+      // Note: must be entered during bootstrap since we need them in
       // configure_execute().
       //
-      vp.insert<path> ("config.export", true /* ovr */);
+      vp.insert<path>  ("config.export", true /* ovr */);
+      vp.insert<paths> ("config.import", true /* ovr */);
 
       // Only create the module if we are configuring or creating or if it was
       // forced with config.module (useful if we need to call $config.export()
@@ -124,7 +125,6 @@ namespace build2
       auto& vp (rs.ctx.var_pool.rw (rs));
 
       auto& c_v (vp.insert<uint64_t> ("config.version", false /*ovr*/));
-      auto& c_i (vp.insert<paths> ("config.import", true /* ovr */));
 
       // Load config.build if one exists followed by extra files specified in
       // config.import (we don't need to worry about disfigure since we will
@@ -181,7 +181,7 @@ namespace build2
           load_config_file (f, l);
       }
 
-      if (lookup l = rs[c_i])
+      if (lookup l = rs["config.import"])
       {
         // Only load files that were specified on our root scope as well as
         // global overrides. This way we can use our override "positioning"
diff --git a/libbuild2/config/module.cxx b/libbuild2/config/module.cxx
index b43f1d9..10ec833 100644
--- a/libbuild2/config/module.cxx
+++ b/libbuild2/config/module.cxx
@@ -10,7 +10,7 @@ namespace build2
 {
   namespace config
   {
-    void module::
+    bool module::
     save_variable (const variable& var, uint64_t flags)
     {
       const string& n (var.name);
@@ -27,7 +27,7 @@ namespace build2
       {
         // @@ For now with 'config.' prefix.
         //
-        i = sm.insert (string (n, 0, n.find ('.', 7)));
+        i = sm.insert (string (n, 0, n.find ('.', 7))).first;
       }
 
       // Don't insert duplicates. The config.import.* vars are particularly
@@ -36,16 +36,20 @@ namespace build2
       saved_variables& sv (i->second);
       auto j (sv.find (var));
 
-      if (j == sv.end ())
-        sv.push_back (saved_variable {var, flags});
-      else
+      if (j != sv.end ())
+      {
         assert (j->flags == flags);
+        return false;
+      }
+
+      sv.push_back (saved_variable {var, flags});
+      return true;
     }
 
-    void module::
+    bool module::
     save_module (const char* name, int prio)
     {
-      saved_modules.insert (string ("config.") += name, prio);
+      return saved_modules.insert (string ("config.") += name, prio).second;
     }
 
     const string module::name ("config");
diff --git a/libbuild2/config/module.hxx b/libbuild2/config/module.hxx
index 4b9d4f2..296dec5 100644
--- a/libbuild2/config/module.hxx
+++ b/libbuild2/config/module.hxx
@@ -62,7 +62,7 @@ namespace build2
       //
       std::multimap<std::int32_t, const_iterator> order;
 
-      iterator
+      pair<iterator, bool>
       insert (string name, int prio = 0)
       {
         auto p (emplace (move (name), saved_variables ()));
@@ -70,7 +70,7 @@ namespace build2
         if (p.second)
           order.emplace (prio, p.first);
 
-        return p.first;
+        return p;
       }
     };
 
@@ -78,10 +78,12 @@ namespace build2
     {
       config::saved_modules saved_modules;
 
-      void
+      // Return true if variable/module were newly inserted.
+      //
+      bool
       save_variable (const variable&, uint64_t flags = 0);
 
-      void
+      bool
       save_module (const char* name, int prio = 0);
 
       static const string name;
diff --git a/libbuild2/config/operation.cxx b/libbuild2/config/operation.cxx
index 14576a0..ea6ff15 100644
--- a/libbuild2/config/operation.cxx
+++ b/libbuild2/config/operation.cxx
@@ -85,6 +85,36 @@ namespace build2
 
     using project_set = set<const scope*>; // Use pointers to get comparison.
 
+    // Mark all the config.import.* variables defined in this variable map as
+    // saved optionally issuing a warning if the variable would otherwise be
+    // unsaved.
+    //
+    static void
+    save_config_import (context& ctx,
+                        module& m,
+                        const variable_map& vars,
+                        bool warning = false,
+                        const location& loc = location ())
+    {
+      auto& vp (ctx.var_pool);
+
+      for (auto p (vars.find_namespace (*vp.find ("config.import")));
+           p.first != p.second;
+           ++p.first)
+      {
+        const variable* var (&p.first->first.get ());
+
+        // Annoyingly, this can be one of the overrides (__override, __prefix,
+        // etc).
+        //
+        if (size_t n = var->override ())
+          var = vp.find (string (var->name, 0, n));
+
+        if (m.save_variable (*var) && warning)
+          warn (loc) << "keeping potentially no longer used variable " << *var;
+      }
+    }
+
     // If inherit is false, then don't rely on inheritance from outer scopes
     // (used for config.export).
     //
@@ -96,7 +126,11 @@ namespace build2
     {
       context& ctx (rs.ctx);
 
-      const module* mod (rs.lookup_module<const module> (module::name));
+      // @@ We are modifying the module (marking additional variables as
+      // saved) and this function can be called from a buildfile (probably
+      // only during serial execution but still).
+      //
+      module* mod (rs.lookup_module<module> (module::name));
 
       if (mod == nullptr)
         fail (on) << "no configuration information available during this "
@@ -122,6 +156,21 @@ namespace build2
           }
         }
 
+        // Mark all the config.import.* variables defined on our root scope as
+        // saved.
+        //
+        // This is in contrast to module-specific config variables (and later
+        // probably to project-specific) where by default we drop no longer
+        // used values (see below). Note that this also used to be the case
+        // for the config.import.* variables but that turned out to be a poor
+        // default since there is no way to "use" such variables except to
+        // import something. So now the default for config.import.* is to keep
+        // with a warning.
+        //
+        // Note that in the future we may make these defaults configurable.
+        //
+        save_config_import (ctx, *mod, rs.vars, true /* warn */, location (on));
+
         // Save config variables.
         //
         names storage;
@@ -165,7 +214,9 @@ namespace build2
               // definitely want to move them to our config.build since they
               // will be dropped from the amalgamation's config.build on the
               // next reconfigure. Let's also warn the user just in case,
-              // unless there is no module and thus we couldn't really check.
+              // unless there is no module and thus we couldn't really check
+              // (the latter could happen when calling $config.export() during
+              // other meta-operations).
               //
               // There is also another case that falls under this now that
               // overrides are by default amalgamation-wide rather than just
@@ -194,6 +245,21 @@ namespace build2
                       const saved_variables& sv (i->second);
                       found = sv.find (var) != sv.end ();
 
+                      // By default treat config.import.* values as "found"
+                      // (see above for details).
+                      //
+                      // Note that if/when we support overriding this default,
+                      // we would need to get the "effective" value for an
+                      // outer project (and the user would need to specify
+                      // such an override even though that project is not
+                      // being configured -- i.e., they would need to
+                      // communicate to us what will have happened once the
+                      // outer project is reconfigured). Feels like it will be
+                      // quite obscure and error-prone.
+                      //
+                      if (!found)
+                        found = var.name.compare (0, 14, "config.import.") == 0;
+
                       // Handle that other case: if this is an override but
                       // the outer project itself is not being configured,
                       // then we need to save this override.
@@ -949,22 +1015,10 @@ namespace build2
 
       // Save all the global config.import.* variables.
       //
-      variable_pool& vp (ctx.var_pool.rw (rs));
-      for (auto p (gs.vars.find_namespace (vp.insert ("config.import")));
-           p.first != p.second;
-           ++p.first)
-      {
-        const variable& var (p.first->first);
-
-        // Annoyingly, this can be (always is?) one of the overrides
-        // (__override, __prefix, etc).
-        //
-        size_t n (var.override ());
-        m.save_variable (n != 0 ? *vp.find (string (var.name, 0, n)) : var);
-      }
+      save_config_import (ctx, m, gs.vars);
 
       // Now project-specific. For now we just save all of them and let
-      // save_config() above weed out the ones that don't apply.
+      // save_config() above weed out the ones that do not apply.
       //
       for (const variable_override& vo: ctx.var_overrides)
       {