Fix several issues in build system module importation logicmodule-import

7 files changed, 275 insertions, 100 deletions

diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx
index 1bc294d..afbb59a 100644
--- a/libbuild2/context.cxx
+++ b/libbuild2/context.cxx
@@ -298,7 +298,7 @@ namespace build2
            const strings& cmd_vars,
            reserves res,
            optional<context*> mc,
-           const loaded_modules_lock* ml,
+           const module_libraries_lock* ml,
            const function<var_override_function>& var_ovr_func)
       : data_ (new data (*this)),
         sched (&s),
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 5c426ad..b0fac02 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -21,7 +21,7 @@
 namespace build2
 {
   class file_cache;
-  class loaded_modules_lock;
+  class module_libraries_lock;
 
   class LIBBUILD2_SYMEXPORT run_phase_mutex
   {
@@ -142,9 +142,9 @@ namespace build2
   // instead go the multiple communicating schedulers route, a la the job
   // server).
   //
-  // The loaded_modules state (module.hxx) is shared among all the contexts
+  // The module_libraries state (module.hxx) is shared among all the contexts
   // (there is no way to have multiple shared library loading "contexts") and
-  // is protected by loaded_modules_lock. A nested context should normally
+  // is protected by module_libraries_lock. A nested context should normally
   // inherit this lock value from its outer context.
   //
   // Note also that any given thread should not participate in multiple
@@ -648,9 +648,9 @@ namespace build2
     dir_path old_src_root;
     dir_path new_src_root;
 
-    // NULL if this context hasn't already locked the loaded_modules state.
+    // NULL if this context hasn't already locked the module_libraries state.
     //
-    const loaded_modules_lock* modules_lock;
+    const module_libraries_lock* modules_lock;
 
     // Nested context for updating build system modules and ad hoc recipes.
     //
@@ -697,7 +697,7 @@ namespace build2
              const strings& cmd_vars = {},
              reserves = {0, 160},
              optional<context*> module_context = nullptr,
-             const loaded_modules_lock* inherited_mudules_lock = nullptr,
+             const module_libraries_lock* inherited_modules_lock = nullptr,
              const function<var_override_function>& = nullptr);
 
     // Special context with bare minimum of initializations. It is only
diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index acfb5d7..99eab96 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -1337,9 +1337,9 @@ namespace build2
 
     // Call module's post-boot functions.
     //
-    for (size_t i (0); i != root.root_extra->modules.size (); ++i)
+    for (size_t i (0); i != root.root_extra->loaded_modules.size (); ++i)
     {
-      module_state& s (root.root_extra->modules[i]);
+      module_state& s (root.root_extra->loaded_modules[i]);
 
       if (s.boot_post != nullptr)
         boot_post_module (root, s);
@@ -1556,11 +1556,11 @@ namespace build2
     // Note that init() can load additional modules invalidating iterators.
     //
     auto init_modules =
-      [&root, n = root.root_extra->modules.size ()] (module_boot_init v)
+      [&root, n = root.root_extra->loaded_modules.size ()] (module_boot_init v)
     {
       for (size_t i (0); i != n; ++i)
       {
-        module_state& s (root.root_extra->modules[i]);
+        module_state& s (root.root_extra->loaded_modules[i]);
 
         if (s.boot_init && *s.boot_init == v)
           init_module (root, root, s.name, s.loc);
@@ -2085,6 +2085,9 @@ namespace build2
   // Return empty name if an ad hoc import resulted in a NULL target (only
   // allowed if optional is true).
   //
+  // Note that this function has a side effect of potentially marking some
+  // config.import.* variables as used.
+  //
   pair<name, optional<dir_path>>
   import_search (bool& new_value,
                  scope& ibase,
diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx
index 4c1271c..dd83225 100644
--- a/libbuild2/module.cxx
+++ b/libbuild2/module.cxx
@@ -30,21 +30,20 @@ using namespace butl;
 
 namespace build2
 {
-  mutex loaded_modules_lock::mutex_;
+  mutex module_libraries_lock::mutex_;
 
-  loaded_module_map loaded_modules;
+  module_libraries_map module_libraries;
 
   void
   load_builtin_module (module_load_function* lf)
   {
     for (const module_functions* i (lf ()); i->name != nullptr; ++i)
-      loaded_modules[i->name] = i;
+      module_libraries.emplace (i->name, module_library {*i, dir_path ()});
   }
 
   // Sorted array of bundled modules (excluding core modules bundled with
   // libbuild2; see below).
   //
-#if !defined(BUILD2_BOOTSTRAP) && !defined(LIBBUILD2_STATIC_BUILD)
   static const char* bundled_modules[] = {
     "bash",
     "bin",
@@ -64,7 +63,6 @@ namespace build2
       bundled_modules + sizeof (bundled_modules) / sizeof (*bundled_modules),
       mod);
   }
-#endif
 
   // Note: also used by ad hoc recipes thus not static.
   //
@@ -244,11 +242,20 @@ namespace build2
   }
 #endif
 
-  static module_load_function*
+  // Return the module functions as well as the module project directory or
+  // empty if not imported from project. Return {nullptr, nullopt} if not
+  // found.
+  //
+  // The dry-run mode only calls import_search() and always returns NULL for
+  // module functions (see below for background).
+  //
+  static pair<module_load_function*, optional<dir_path>>
   import_module (
 #if defined(BUILD2_BOOTSTRAP) || defined(LIBBUILD2_STATIC_BUILD)
+    bool,
     scope&,
 #else
+    bool dry_run,
     scope& bs,
 #endif
     const string& mod,
@@ -262,15 +269,21 @@ namespace build2
   {
     tracer trace ("import_module");
 
+    pair<module_load_function*, optional<dir_path>> r (nullptr, nullopt);
+
     // Take care of core modules that are bundled with libbuild2 in case they
     // are not pre-loaded by the driver.
     //
-    if      (mod == "config")  return &config::build2_config_load;
-    else if (mod == "dist")    return &dist::build2_dist_load;
-    else if (mod == "install") return &install::build2_install_load;
-    else if (mod == "test")    return &test::build2_test_load;
+    if      (mod == "config")  r.first = &config::build2_config_load;
+    else if (mod == "dist")    r.first = &dist::build2_dist_load;
+    else if (mod == "install") r.first = &install::build2_install_load;
+    else if (mod == "test")    r.first = &test::build2_test_load;
 
-    module_load_function* r (nullptr);
+    if (r.first != nullptr)
+    {
+      r.second = dir_path ();
+      return r;
+    }
 
     // No dynamic loading of build system modules during bootstrap or if
     // statically-linked..
@@ -339,7 +352,7 @@ namespace build2
     // and undefined if the module was not mentioned.
     //
     if (boot && !bundled && ctx.no_external_modules)
-      return nullptr;
+      return r; // NULL
 
     // See if we can import a target for this module.
     //
@@ -394,7 +407,7 @@ namespace build2
     if (ir.first.empty ())
     {
       assert (opt);
-      return nullptr;
+      return r; // NULL
     }
 
     if (ir.second)
@@ -402,6 +415,8 @@ namespace build2
       // What if a module is specified with config.import.<mod>.<lib>.libs?
       // Note that this could still be a project-qualified target.
       //
+      // Note: we now return an empty directory to mean something else.
+      //
       if (ir.second->empty ())
         fail (loc) << "direct module target importation not yet supported";
 
@@ -409,6 +424,17 @@ namespace build2
       // the target (which will also give us the shared library path).
       //
       l5 ([&]{trace << "found " << ir.first << " in " << *ir.second;});
+    }
+
+    if (dry_run)
+    {
+      r.second = ir.second ? move (*ir.second) : dir_path ();
+      return r;
+    }
+
+    if (ir.second)
+    {
+      r.second = *ir.second;
 
       // Create the build context if necessary.
       //
@@ -421,7 +447,7 @@ namespace build2
         create_module_context (ctx, loc);
       }
 
-      // Inherit loaded_modules lock from the outer context.
+      // Inherit module_libraries lock from the outer context.
       //
       ctx.module_context->modules_lock = ctx.modules_lock;
 
@@ -481,6 +507,8 @@ namespace build2
     }
     else
     {
+      r.second = dir_path ();
+
       // No module project found. Form the shared library name (incorporating
       // build system core version) and try using system-default search
       // (installed, rpath, etc).
@@ -523,7 +551,7 @@ namespace build2
         fail (loc) << "unable to lookup " << sym << " in build system module "
                    << mod << " (" << lib << "): " << err;
 
-      r = function_cast<module_load_function*> (hs.second);
+      r.first = function_cast<module_load_function*> (hs.second);
     }
     else if (!opt)
     {
@@ -535,7 +563,10 @@ namespace build2
                  << "line variable to specify its project out_root";
     }
     else
+    {
+      r.second = nullopt;
       l5 ([&]{trace << "unable to load " << lib << ": " << err;});
+    }
 
 #endif // BUILD2_BOOTSTRAP || LIBBUILD2_STATIC_BUILD
 
@@ -551,89 +582,200 @@ namespace build2
   {
     tracer trace ("find_module");
 
-    // Note that we hold the lock for the entire time it takes to build a
-    // module.
+    // If this is a submodule, get the main module name.
     //
-    loaded_modules_lock lock (bs.ctx);
+    string mmod (smod, 0, smod.find ('.'));
 
-    // Optional modules and submodules sure make this logic convoluted. So we
-    // divide it into two parts: (1) find or insert an entry (for submodule
-    // or, failed that, for the main module, the latter potentially NULL) and
-    // (2) analyze the entry and issue diagnostics.
+    // We have a somewhat strange two-level caching in imported_modules
+    // and module_libraries in order to achieve the following:
+    //
+    // 1. Correctly handle cases where a module can be imported from one
+    //    project but not the other.
+    //
+    // 2. Make sure that for each project that imports the module we actually
+    //    call import_search() in order to mark any config.import.* as used.
     //
-    auto i (loaded_modules.find (smod)), e (loaded_modules.end ());
+    // 3. Make sure that all the projects import the same module.
+    //
+    scope& rs (*bs.root_scope ());
+
+    const string* mod;
+    const module_functions* fun;
 
-    if (i == e)
+    // First check the project's imported_modules in case this (main) module
+    // is known to be not found.
+    //
+    auto j (rs.root_extra->imported_modules.find (mmod));
+    auto je (rs.root_extra->imported_modules.end ());
+
+    if (j != je && !j->found)
+    {
+      mod = &mmod;
+      fun = nullptr;
+    }
+    else
     {
-      // If this is a submodule, get the main module name.
+      // Note that we hold the lock for the entire time it takes to build a
+      // module.
       //
-      string mmod (smod, 0, smod.find ('.'));
+      module_libraries_lock lock (bs.ctx);
 
-      if (mmod != smod)
-        i = loaded_modules.find (mmod);
+      // Optional modules and submodules sure make this logic convoluted. So
+      // we divide it into two parts: (1) find or insert an entry (for
+      // submodule or, failed that, for the main module) and (2) analyze the
+      // entry and issue diagnostics.
+      //
+      auto i (module_libraries.find (smod));
+      auto ie (module_libraries.end ());
 
-      if (i == e)
+      bool imported (false);
+      if (i == ie)
       {
-        module_load_function* f (import_module (bs, mmod, loc, boot, opt));
+        if (mmod != smod)
+          i = module_libraries.find (mmod);
 
-        if (f != nullptr)
+        if (i == ie)
         {
-          // Enter all the entries noticing which one is our submodule. If
-          // none are, then we notice the main module.
-          //
-          for (const module_functions* j (f ()); j->name != nullptr; ++j)
+          pair<module_load_function*, optional<dir_path>> ir (
+            import_module (false /* dry_run */, bs, mmod, loc, boot, opt));
+
+          if (module_load_function* f = ir.first)
           {
-            const string& n (j->name);
+            // Enter all the entries noticing which one is our submodule. If
+            // none are, then we notice the main module.
+            //
+            for (const module_functions* j (f ()); j->name != nullptr; ++j)
+            {
+              const string& n (j->name);
 
-            l5 ([&]{trace << "registering " << n;});
+              l5 ([&]{trace << "registering " << n;});
 
-            auto p (loaded_modules.emplace (n, j));
+              bool main (n == mmod);
 
-            if (!p.second)
-              fail (loc) << "build system submodule name " << n << " of main "
-                         << "module " << mmod << " is already in use";
+              auto p (module_libraries.emplace (
+                        n,
+                        module_library {
+                          *j,
+                          main ? move (*ir.second) : dir_path ()}));
 
-            if (n == smod || (i == e && n == mmod))
-              i = p.first;
+              if (!p.second)
+                fail (loc) << "build system submodule name " << n << " of main "
+                           << "module " << mmod << " is already in use";
+
+              // Note: this assumes the main module is last.
+              //
+              if (n == smod || (main && i == ie))
+                i = p.first;
+            }
+
+            // We should at least have the main module.
+            //
+            if (i == ie)
+              fail (loc) << "invalid function list in build system module "
+                         << mmod;
           }
 
-          // We should at least have the main module.
-          //
-          if (i == e)
-            fail (loc) << "invalid function list in build system module "
-                       << mmod;
+          imported = true;
         }
-        else
-          i = loaded_modules.emplace (move (mmod), nullptr).first;
+      }
+
+      // Now the iterator points to a submodule or to the main module, or to
+      // end if neither is found.
+      //
+      assert (j == je || i != ie); // Cache state consistecy sanity check.
+
+      if (i != ie)
+      {
+        // Note: these should remain stable after we release the lock.
+        //
+        mod = &i->first;
+        fun = &i->second.functions.get ();
+
+        // If this project hasn't imported this main module and we found the
+        // entry in the cache, then we have to perform the import_search()
+        // part of import_module() in order to cover items (2) and (3) above.
+        //
+        // There is one nuance: omit this for bundled modules since it's
+        // possible to first import them ad hoc and then, if we call
+        // import_search() again, to find them differently (e.g., as a
+        // subproject).
+        //
+        if (j == je && !imported && !bundled_module (mmod))
+        {
+          pair<module_load_function*, optional<dir_path>> ir (
+            import_module (true /* dry_run */, bs, mmod, loc, boot, opt));
+
+          if (ir.second)
+          {
+            if (i->first != mmod)
+            {
+              i = module_libraries.find (mmod);
+              assert (i != ie); // Has to be there.
+            }
+
+            const dir_path& cd (*ir.second);
+            const dir_path& pd (i->second.import_path);
+
+            if (cd != pd)
+            {
+              fail (loc) << "inconsistent build system module " << mmod
+                         << " importation" <<
+                info << rs << " imports it as "
+                     << (cd.empty () ? "ad hoc" : cd.representation ().c_str ()) <<
+                info << "previously imported as "
+                     << (pd.empty () ? "ad hoc" : pd.representation ().c_str ());
+            }
+          }
+          else
+          {
+            // This module is not found from this project.
+            //
+            mod = &mmod;
+            fun = nullptr;
+          }
+        }
+      }
+      else
+      {
+        mod = &mmod;
+        fun = nullptr;
       }
     }
 
+    // Cache the result in imported_modules if necessary.
+    //
+    if (j == je)
+      rs.root_extra->imported_modules.push_back (
+        module_import {mmod, fun != nullptr});
+
     // Reduce skipped external module to optional.
     //
-    if (boot && i->second == nullptr)
+    if (boot && fun == nullptr)
       opt = true;
 
-    // Now the iterator points to a submodule or to the main module, the
-    // latter potentially NULL.
+    // Handle optional.
     //
-    if (!opt)
+    if (fun == nullptr)
     {
-      if (i->second == nullptr)
-      {
-        fail (loc) << "unable to load build system module " << i->first;
-      }
-      else if (i->first != smod)
-      {
-        fail (loc) << "build system module " << i->first << " has no "
+      if (!opt)
+        fail (loc) << "unable to load build system module " << *mod;
+    }
+    else if (*mod != smod)
+    {
+      if (!opt)
+        fail (loc) << "build system module " << *mod << " has no "
                    << "submodule " << smod;
+      else
+      {
+        // Note that if the main module exists but has no such submodule, we
+        // return NULL rather than fail (think of an older version of a module
+        // that doesn't implement some extra functionality).
+        //
+        fun = nullptr;
       }
     }
 
-    // Note that if the main module exists but has no such submodule, we
-    // return NULL rather than fail (think of an older version of a module
-    // that doesn't implement some extra functionality).
-    //
-    return i->second;
+    return fun;
   }
 
   void
@@ -641,7 +783,7 @@ namespace build2
   {
     // First see if this modules has already been booted for this project.
     //
-    module_map& lm (rs.root_extra->modules);
+    module_state_map& lm (rs.root_extra->loaded_modules);
     auto i (lm.find (mod));
 
     if (i != lm.end ())
@@ -717,7 +859,7 @@ namespace build2
   {
     // First see if this modules has already been inited for this project.
     //
-    module_map& lm (rs.root_extra->modules);
+    module_state_map& lm (rs.root_extra->loaded_modules);
     auto i (lm.find (mod));
     bool f (i == lm.end ());
 
@@ -837,7 +979,7 @@ namespace build2
     if (cast_false<bool> (bs[name + ".loaded"]))
     {
       if (cast_false<bool> (bs[name + ".configured"]))
-        return rs.root_extra->modules.find (name)->module;
+        return rs.root_extra->loaded_modules.find (name)->module;
     }
     else
     {
@@ -859,7 +1001,7 @@ namespace build2
     //   attempt to load it was optional?
 
     return cast_false<bool> (bs[name + ".loaded"])
-      ? rs.root_extra->modules.find (name)->module
+      ? rs.root_extra->loaded_modules.find (name)->module
       : init_module (rs, bs, name, loc, false /* optional */, hints)->module;
   }
 }
diff --git a/libbuild2/module.hxx b/libbuild2/module.hxx
index 2f2d8a7..c638c70 100644
--- a/libbuild2/module.hxx
+++ b/libbuild2/module.hxx
@@ -161,7 +161,31 @@ namespace build2
   extern "C"
   using module_load_function = const module_functions* ();
 
-  // Module state.
+  // Imported module state.
+  //
+  // The module name is the main module (corresponding to the library). If
+  // found is false then this module could not be imported from this project.
+  //
+  struct module_import
+  {
+    const string name;
+    bool found;
+  };
+
+  struct module_import_map: vector<module_import>
+  {
+    iterator
+    find (const string& name)
+    {
+      return find_if (
+        begin (), end (),
+        [&name] (const module_import& i) {return i.name == name;});
+    }
+  };
+
+  // Loaded module state.
+  //
+  // Note that unlike import_state, the module name here could be a submodule.
   //
   struct module_state
   {
@@ -173,7 +197,7 @@ namespace build2
     optional<module_boot_init> boot_init;
   };
 
-  struct module_map: vector<module_state>
+  struct module_state_map: vector<module_state>
   {
     iterator
     find (const string& name)
@@ -274,23 +298,28 @@ namespace build2
     return static_cast<T&> (*load_module (root, base, name, l, config_hints));
   }
 
-  // Loaded modules (as in libraries).
+  // Loaded module libraries.
   //
-  // A NULL entry for the main module indicates that a module library was not
-  // found.
+  // Note that this map contains entries for all the submodules.
   //
-  using loaded_module_map = map<string, const module_functions*>;
+  struct module_library
+  {
+    reference_wrapper<const module_functions> functions;
+    dir_path import_path; // Only for main module.
+  };
+
+  using module_libraries_map = map<string, module_library>;
 
-  // The loaded_modules map is locked per top-level (as opposed to nested)
+  // The module_libraries map is locked per top-level (as opposed to nested)
   // context (see context.hxx for details).
   //
   // Note: should only be constructed during contexts-wide serial execution.
   //
-  class LIBBUILD2_SYMEXPORT loaded_modules_lock
+  class LIBBUILD2_SYMEXPORT module_libraries_lock
   {
   public:
     explicit
-    loaded_modules_lock (context& c)
+    module_libraries_lock (context& c)
       : ctx_ (c), lock_ (mutex_, defer_lock)
     {
       if (ctx_.modules_lock == nullptr)
@@ -300,7 +329,7 @@ namespace build2
       }
     }
 
-    ~loaded_modules_lock ()
+    ~module_libraries_lock ()
     {
       if (ctx_.modules_lock == this)
         ctx_.modules_lock = nullptr;
@@ -312,7 +341,7 @@ namespace build2
     mlock lock_;
   };
 
-  LIBBUILD2_SYMEXPORT extern loaded_module_map loaded_modules;
+  LIBBUILD2_SYMEXPORT extern module_libraries_map modules_libraries;
 
   // Load a builtin module (i.e., a module linked as a static/shared library
   // or that is part of the build system driver).
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index d84db4f..e57caa5 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -952,7 +952,7 @@ namespace build2
       //
       auto print_mods = [&rs] ()
       {
-        for (const module_state& ms: rs.root_extra->modules)
+        for (const module_state& ms: rs.root_extra->loaded_modules)
           cout << ' ' << ms.name;
       };
 
@@ -1123,12 +1123,12 @@ namespace build2
 
       // Print modules.
       //
-      if (!rs.root_extra->modules.empty ())
+      if (!rs.root_extra->loaded_modules.empty ())
       {
         s.member_name ("modules", false /* check */);
         s.begin_array ();
 
-        for (const module_state& ms: rs.root_extra->modules)
+        for (const module_state& ms: rs.root_extra->loaded_modules)
           s.value (ms.name, false /* check */);
 
         s.end_array ();
diff --git a/libbuild2/scope.hxx b/libbuild2/scope.hxx
index 0eabd17..5578d0c 100644
--- a/libbuild2/scope.hxx
+++ b/libbuild2/scope.hxx
@@ -510,9 +510,10 @@ namespace build2
       build2::meta_operations meta_operations;
       build2::operations operations;
 
-      // Modules loaded by this project.
+      // Modules imported/loaded by this project.
       //
-      module_map modules;
+      module_import_map imported_modules;
+      module_state_map  loaded_modules;
 
       // Buildfiles already loaded for this project.
       //
@@ -591,21 +592,21 @@ namespace build2
     bool
     find_module (const string& name) const
     {
-      return root_extra->modules.find_module<module> (name) != nullptr;
+      return root_extra->loaded_modules.find_module<module> (name) != nullptr;
     }
 
     template <typename T>
     T*
     find_module (const string& name)
     {
-      return root_extra->modules.find_module<T> (name);
+      return root_extra->loaded_modules.find_module<T> (name);
     }
 
     template <typename T>
     const T*
     find_module (const string& name) const
     {
-      return root_extra->modules.find_module<T> (name);
+      return root_extra->loaded_modules.find_module<T> (name);
     }
 
   public: