From bc582eb32448bf7eb61ab3b1c76597885c8ec02a Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 19 Apr 2023 10:27:14 +0200 Subject: Fix several issues in build system module importation logic --- libbuild2/context.cxx | 2 +- libbuild2/context.hxx | 12 +-- libbuild2/file.cxx | 11 +- libbuild2/module.cxx | 282 ++++++++++++++++++++++++++++++++++++------------ libbuild2/module.hxx | 51 +++++++-- libbuild2/operation.cxx | 6 +- libbuild2/scope.hxx | 11 +- 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 mc, - const loaded_modules_lock* ml, + const module_libraries_lock* ml, const 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 module_context = nullptr, - const loaded_modules_lock* inherited_mudules_lock = nullptr, + const module_libraries_lock* inherited_modules_lock = nullptr, const 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> 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> 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> 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...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 (hs.second); + r.first = function_cast (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> 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> 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 (bs[name + ".loaded"])) { if (cast_false (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 (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 + { + 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 boot_init; }; - struct module_map: vector + struct module_state_map: vector { iterator find (const string& name) @@ -274,23 +298,28 @@ namespace build2 return static_cast (*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; + struct module_library + { + reference_wrapper functions; + dir_path import_path; // Only for main module. + }; + + using module_libraries_map = map; - // 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 (name) != nullptr; + return root_extra->loaded_modules.find_module (name) != nullptr; } template T* find_module (const string& name) { - return root_extra->modules.find_module (name); + return root_extra->loaded_modules.find_module (name); } template const T* find_module (const string& name) const { - return root_extra->modules.find_module (name); + return root_extra->loaded_modules.find_module (name); } public: -- cgit v1.1