From f027a0314d1889b9159c23103f05baea7c1dbdf3 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Fri, 9 Aug 2024 17:22:57 -0400 Subject: [PATCH 1/6] Represent plugins explicitly --- src/libraries/JANA/Services/JPluginLoader.cc | 47 ++++++++++---------- src/libraries/JANA/Services/JPluginLoader.h | 28 +++++++++--- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index 76dae3421..b77f2c10b 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/src/libraries/JANA/Services/JPluginLoader.cc @@ -13,6 +13,7 @@ #include #include #include +#include class JApplication; @@ -163,7 +164,7 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) { LOG_DEBUG(m_logger) << "Found!" << LOG_END; try { jcm->next_plugin(plugin_shortname); - attach_plugin(fullpath.c_str()); + attach_plugin(plugin_shortname, fullpath); paths_checked << "Loaded successfully" << std::endl; found_plugin = true; break; @@ -213,7 +214,7 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) { } -void JPluginLoader::attach_plugin(std::string soname) { +void JPluginLoader::attach_plugin(std::string name, std::string path) { /// Attach a plugin by opening the shared object file and running the /// InitPlugin_t(JApplication* app) global C-style routine in it. @@ -230,10 +231,10 @@ void JPluginLoader::attach_plugin(std::string soname) { /// // Open shared object - void* handle = dlopen(soname.c_str(), RTLD_LAZY | RTLD_GLOBAL | RTLD_NODELETE); + void* handle = dlopen(path.c_str(), RTLD_LAZY | RTLD_GLOBAL | RTLD_NODELETE); if (!handle) { std::string err = dlerror(); - LOG_DEBUG(m_logger) << "Plugin dlopen() failed: " << err << LOG_END; + LOG_ERROR(m_logger) << "Plugin dlopen() failed: " << err << LOG_END; throw JException("Plugin dlopen() failed: %s", err.c_str()); } @@ -241,35 +242,35 @@ void JPluginLoader::attach_plugin(std::string soname) { typedef void InitPlugin_t(JApplication* app); InitPlugin_t* initialize_proc = (InitPlugin_t*) dlsym(handle, "InitPlugin"); if (initialize_proc) { - LOG_INFO(m_logger) << "Initializing plugin \"" << soname << "\"" << LOG_END; + LOG_INFO(m_logger) << "Initializing plugin \"" << name << "\" at path \"" << path << "\"" << LOG_END; (*initialize_proc)(GetApplication()); - m_sohandles[soname] = handle; + auto plugin = std::make_unique(name, path); + plugin->m_app = GetApplication(); + plugin->m_logger = m_logger; + plugin->m_handle = handle; + m_plugin_index[name] = plugin.get(); + m_plugins.push_front(std::move(plugin)); + // We push to the front so that plugins get unloaded in the reverse order they were originally added } else { dlclose(handle); - LOG_DEBUG(m_logger) << "Plugin \"" << soname + LOG_ERROR(m_logger) << "Plugin \"" << name << "\" does not have an InitPlugin() function. Ignoring." << LOG_END; } } -JPluginLoader::JPluginLoader() {} +JPlugin::~JPlugin() { -JPluginLoader::~JPluginLoader() { - - // Loop over open plugin handles. - // Call FinalizePlugin if it has one and close it in all cases. + // Call FinalizePlugin() typedef void FinalizePlugin_t(JApplication* app); - for( auto p :m_sohandles ){ - auto soname = p.first; - auto handle = p.second; - FinalizePlugin_t* finalize_proc = (FinalizePlugin_t*) dlsym(handle, "FinalizePlugin"); - if (finalize_proc) { - LOG_INFO(m_logger) << "Finalizing plugin \"" << soname << "\"" << LOG_END; - (*finalize_proc)(GetApplication()); - } - - // Close plugin handle - dlclose(handle); + FinalizePlugin_t* finalize_proc = (FinalizePlugin_t*) dlsym(m_handle, "FinalizePlugin"); + if (finalize_proc) { + LOG_INFO(m_logger) << "Finalizing plugin \"" << m_name << "\"" << LOG_END; + (*finalize_proc)(m_app); } + + // Close plugin handle + dlclose(m_handle); + LOG_DEBUG(m_logger) << "Unloaded plugin \"" << m_name << "\"" << LOG_END; } diff --git a/src/libraries/JANA/Services/JPluginLoader.h b/src/libraries/JANA/Services/JPluginLoader.h index 3f6958a3b..8da4e4ab4 100644 --- a/src/libraries/JANA/Services/JPluginLoader.h +++ b/src/libraries/JANA/Services/JPluginLoader.h @@ -11,23 +11,38 @@ #include #include +#include class JComponentManager; class JApplication; +class JPlugin { + friend class JPluginLoader; + JApplication* m_app; + JLogger m_logger; + std::string m_name; + std::string m_path; + void* m_handle; +public: + JPlugin(std::string name, std::string path) : m_name(name), m_path(path) {}; + ~JPlugin(); + std::string GetName() { return m_name; } + std::string GetPath() { return m_path; } +}; + class JPluginLoader : public JService { public: - JPluginLoader(); - ~JPluginLoader() override; + JPluginLoader() = default; + ~JPluginLoader() override = default; void Init() override; void add_plugin(std::string plugin_name); void add_plugin_path(std::string path); void attach_plugins(JComponentManager* jcm); - void attach_plugin(std::string plugin_name); + void attach_plugin(std::string name, std::string path); void resolve_plugin_paths(); private: @@ -37,10 +52,13 @@ class JPluginLoader : public JService { std::vector m_plugins_to_exclude; std::vector m_plugin_paths; std::string m_plugin_paths_str; - std::map m_sohandles; // key=plugin name val=dlopen handle - bool m_verbose = false; + // We wish to preserve each plugin's insertion order + // This way, plugins are unloaded in the reverse order they were added + std::list> m_plugins; + std::map m_plugin_index; + bool m_verbose = false; }; From 403b81fe520520d53c246e61d33623dd217463e1 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Sun, 11 Aug 2024 02:56:44 -0400 Subject: [PATCH 2/6] Reorganize JPluginLoader --- src/libraries/JANA/Services/JPluginLoader.cc | 264 +++++++++++-------- src/libraries/JANA/Services/JPluginLoader.h | 6 + 2 files changed, 155 insertions(+), 115 deletions(-) diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index b77f2c10b..6f4444eb0 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/src/libraries/JANA/Services/JPluginLoader.cc @@ -8,8 +8,10 @@ #include "JParameterManager.h" #include +#include #include #include +#include #include #include #include @@ -60,28 +62,23 @@ void JPluginLoader::add_plugin(std::string plugin_name) { void JPluginLoader::resolve_plugin_paths() { // Build our list of plugin search paths. - // 1. First we look for plugins in the local directory - add_plugin_path("."); - - // 2. Next we look for plugins in locations specified via parameters. (Colon-separated) + // 1. First we look for plugins in locations specified via parameters. (Colon-separated) std::stringstream param_ss(m_plugin_paths_str); std::string path; while (getline(param_ss, path, ':')) add_plugin_path(path); - // 3. Next we look for plugins in locations specified via environment variable. (Colon-separated) - const char* jpp = getenv("JANA_PLUGIN_PATH"); - if (jpp) { + // 2. Next we look for plugins in locations specified via environment variable. (Colon-separated) + if (const char* jpp = getenv("JANA_PLUGIN_PATH")) { std::stringstream envvar_ss(jpp); while (getline(envvar_ss, path, ':')) add_plugin_path(path); } - // 4. Next we look in the plugin directories relative to $JANA_HOME + // 3. Next we look in the plugin directories relative to $JANA_HOME if (const char* jana_home = getenv("JANA_HOME")) { add_plugin_path(std::string(jana_home) + "/lib/JANA/plugins"); // In case we did a system install and want to avoid conflicts. - add_plugin_path(std::string(jana_home) + "/plugins"); } - // 5. Finally we look in the JANA install directory. + // 4. Finally we look in the JANA install directory. // By checking here, the user no longer needs to set JANA_HOME in order to run built-in plugins // such as janadot and JTest. The install directory is supposed to be the same as JANA_HOME, // but we can't guarantee that because the user can set JANA_HOME to be anything they want. @@ -119,97 +116,62 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) { /// Loop over list of plugin names added via AddPlugin() and /// actually attach and initialize them. See AddPlugin method /// for more. + + // Figure out the search paths for plugins. For now, plugins _cannot_ add to the search paths resolve_plugin_paths(); - // Add plugins specified via PLUGINS configuration parameter - // (comma separated list). + // Figure out the set of plugins to exclude. Note that this applies to plugins added + // by other plugins as well. std::set exclusions(m_plugins_to_exclude.begin(), m_plugins_to_exclude.end()); - // Loop over plugins - // It is possible for plugins to add additional plugins that will also need to - // be attached. To accommodate this we wrap the following chunk of code in - // a lambda function so we can run it over the additional plugins recursively - // until all are attached. (see below) - auto add_plugins_lamda = [=, this](std::vector &plugins) { - std::stringstream paths_checked; - for (const std::string& plugin : plugins) { - // The user might provide a short name like "JTest", or a long name like "JTest.so". - // We assume that the plugin extension is always ".so". This may pose a problem on macOS - // where the extension might default to ".dylib". - std::string plugin_shortname; - std::string plugin_fullname; - if (plugin.substr(plugin.size() - 3) != ".so") { - plugin_fullname = plugin + ".so"; - } - else { - plugin_fullname = plugin; - } - - plugin_shortname = std::filesystem::path(plugin_fullname).filename().stem().string(); + // Loop over all requested plugins as per the `plugins` parameter. Note that this vector may grow as + // plugins themselves request additional plugins. These get appended to the back of `m_plugins_to_include`. + for (int i=0; inext_plugin(name); - if (exclusions.find(plugin_shortname) != exclusions.end() || - exclusions.find(plugin_fullname) != exclusions.end()) { + if (exclusions.find(name) != exclusions.end() || exclusions.find(name) != exclusions.end()) { + LOG_INFO(m_logger) << "Excluding plugin `" << name << "`" << LOG_END; + continue; + } + // TODO: Check if plugin has already been loaded!!! - LOG_INFO(m_logger) << "Excluding plugin `" << plugin << "`" << LOG_END; - continue; + bool found_plugin = false; + if (path != std::nullopt) { + found_plugin = validate_path(*path); + if (found_plugin) { + paths_checked << " " << *path << " => Found" << std::endl; } - - // Loop over paths - bool found_plugin = false; - for (std::string path : m_plugin_paths) { - std::string fullpath = path + "/" + plugin_fullname; - LOG_DEBUG(m_logger) << "Looking for '" << fullpath << "' ...." << LOG_END; - paths_checked << " " << fullpath << " => "; - if (access(fullpath.c_str(), F_OK) != -1) { - LOG_DEBUG(m_logger) << "Found!" << LOG_END; - try { - jcm->next_plugin(plugin_shortname); - attach_plugin(plugin_shortname, fullpath); - paths_checked << "Loaded successfully" << std::endl; - found_plugin = true; - break; - } - catch (JException& e) { - LOG_WARN(m_logger) << "Exception loading plugin: " << e << LOG_END; - paths_checked << "JException: " << e.GetMessage() << std::endl; - continue; - } - catch (std::exception& e) { - LOG_WARN(m_logger) << "Exception loading plugin: " << e.what() << LOG_END; - paths_checked << "Exception: " << e.what() << std::endl; - continue; - } - catch (...) { - LOG_WARN(m_logger) << "Unknown exception loading plugin" << LOG_END; - paths_checked << "Unknown exception" << std::endl; - continue; - } - } - else { - paths_checked << "File not found" << std::endl; - - } - LOG_DEBUG(m_logger) << "Failed to attach '" << fullpath << "'" << LOG_END; + else { + paths_checked << " " << *path << " => Not found" << std::endl; } + } + else { + // User didn't provide a path, so we have to search + path = find_first_valid_path(name, paths_checked); + found_plugin = (path != std::nullopt); + } + if (!found_plugin) { - // If we didn't find the plugin, then complain and quit - if (!found_plugin) { - LOG_ERROR(m_logger) << "Couldn't load plugin '" << plugin << "'\n" << - " Make sure that JANA_HOME and/or JANA_PLUGIN_PATH environment variables are set correctly.\n" - << - " Paths checked:\n" << paths_checked.str() << LOG_END; - throw JException("Couldn't find plugin '%s'", plugin.c_str()); - } + LOG_ERROR(m_logger) << "Couldn't find plugin '" << name << "'\n" << + " Make sure that JANA_HOME and/or JANA_PLUGIN_PATH environment variables are set correctly.\n" + << + " Paths checked:\n" << paths_checked.str() << LOG_END; + throw JException("Couldn't find plugin '%s'", name.c_str()); } - }; - - // Recursively loop over the list of plugins to ensure new plugins added by ones being - // attached are also attached. - uint64_t inext = 0; - while(inext < m_plugins_to_include.size() ){ - std::vector myplugins(m_plugins_to_include.begin() + inext, m_plugins_to_include.end()); - inext = m_plugins_to_include.size(); // new plugins will be attached to end of vector - add_plugins_lamda(myplugins); + + // At this point, the plugin has been found, and we are going to try to attach it. + // If the attachment fails for any reason, so does attach_plugins() and ultimately JApplication::Initialize(). + // We do not attempt to search for non-failing plugins by looking further down the search path, because this + // masks the "root cause" error and causes much greater confusion. A specific example is working in an environment + // that has a read-only system install of JANA, e.g. Singularity or CVMFS. If the host application is built using + // a newer version of JANA which is not binary-compatible with the system install, and both locations end up on the + // plugin search path, a legitimate error loading the correct plugin would be suppressed, and the user would instead + // see an extremely difficult-to-debug error (usually a segfault) stemming from the binary incompatibility + // between the host application and the plugin. + + attach_plugin(name, *path); // Throws JException on failure } } @@ -221,41 +183,66 @@ void JPluginLoader::attach_plugin(std::string name, std::string path) { /// An exception will be thrown if the plugin is not successfully opened. /// Users will not need to call this directly since it is called automatically /// from Initialize(). - /// - /// @param soname name of shared object file to attach. This may include - /// an absolute or relative path. - /// - /// @param verbose if set to true, failed attempts will be recorded via the - /// JLog. Default is false so JANA can silently ignore files - /// that are not valid plugins. - /// // Open shared object + dlerror(); // Clear any earlier dlerrors void* handle = dlopen(path.c_str(), RTLD_LAZY | RTLD_GLOBAL | RTLD_NODELETE); if (!handle) { std::string err = dlerror(); - LOG_ERROR(m_logger) << "Plugin dlopen() failed: " << err << LOG_END; - throw JException("Plugin dlopen() failed: %s", err.c_str()); + LOG_ERROR(m_logger) << "Plugin \"" << name << "\" dlopen() failed: " << err << LOG_END; + throw JException("Plugin '%s' dlopen() failed: %s", name.c_str(), err.c_str()); } - // Look for an InitPlugin symbol + // Retrieve the InitPlugin symbol typedef void InitPlugin_t(JApplication* app); InitPlugin_t* initialize_proc = (InitPlugin_t*) dlsym(handle, "InitPlugin"); - if (initialize_proc) { - LOG_INFO(m_logger) << "Initializing plugin \"" << name << "\" at path \"" << path << "\"" << LOG_END; - (*initialize_proc)(GetApplication()); - auto plugin = std::make_unique(name, path); - plugin->m_app = GetApplication(); - plugin->m_logger = m_logger; - plugin->m_handle = handle; - m_plugin_index[name] = plugin.get(); - m_plugins.push_front(std::move(plugin)); - // We push to the front so that plugins get unloaded in the reverse order they were originally added - } else { + if (!initialize_proc) { dlclose(handle); - LOG_ERROR(m_logger) << "Plugin \"" << name - << "\" does not have an InitPlugin() function. Ignoring." << LOG_END; + LOG_ERROR(m_logger) << "Plugin \"" << name << "\" is missing 'InitPlugin' symbol" << LOG_END; + throw JException("Plugin '%s' is missing 'InitPlugin' symbol", name.c_str()); + } + + // Run InitPlugin() and wrap exceptions as needed + LOG_INFO(m_logger) << "Initializing plugin \"" << name << "\" at path \"" << path << "\"" << LOG_END; + + try { + (*initialize_proc)(GetApplication()); + } + catch (JException& ex) { + if (ex.function_name.empty()) ex.function_name = "attach_plugin"; + if (ex.type_name.empty()) ex.type_name = "JPluginLoader"; + if (ex.instance_name.empty()) ex.instance_name = m_prefix; + if (ex.plugin_name.empty()) ex.plugin_name = name; + throw ex; + } + catch (std::exception& e) { + auto ex = JException(e.what()); + ex.exception_type = JTypeInfo::demangle_current_exception_type(); + ex.nested_exception = std::current_exception(); + ex.function_name = "attach_plugin"; + ex.type_name = "JPluginLoader"; + ex.instance_name = m_prefix; + ex.plugin_name = name; + throw ex; } + catch (...) { + auto ex = JException("Unknown exception"); + ex.exception_type = JTypeInfo::demangle_current_exception_type(); + ex.nested_exception = std::current_exception(); + ex.function_name = "attach_plugin"; + ex.type_name = "JPluginLoader"; + ex.instance_name = m_prefix; + ex.plugin_name = name; + throw ex; + } + + // Do some bookkeeping + auto plugin = std::make_unique(name, path); + plugin->m_app = GetApplication(); + plugin->m_logger = m_logger; + plugin->m_handle = handle; + m_plugin_index[name] = plugin.get(); + m_plugins.push_front(std::move(plugin)); } @@ -274,3 +261,50 @@ JPlugin::~JPlugin() { LOG_DEBUG(m_logger) << "Unloaded plugin \"" << m_name << "\"" << LOG_END; } +std::pair> JPluginLoader::extract_name_and_maybe_path(std::string user_name_or_path) { + if (user_name_or_path.find('/') != -1) { + std::string name = std::filesystem::path(user_name_or_path).filename().stem().string(); + std::string path = user_name_or_path; + return {name, path}; + } + else { + if (user_name_or_path.substr(user_name_or_path.size() - 3) == ".so") { + user_name_or_path = user_name_or_path.substr(0, user_name_or_path.size() - 3); + } + return {user_name_or_path, std::nullopt}; + } +} + + +std::string JPluginLoader::make_path_from_name(std::string name, const std::string& path_prefix) { + std::ostringstream oss; + oss << path_prefix; + if (!path_prefix.ends_with('/')) { + oss << "/"; + } + oss << name; + oss << ".so"; + return oss.str(); +} + +std::optional JPluginLoader::find_first_valid_path(std::string name, std::ostringstream& debug_log) { + + for (const std::string& path_prefix : m_plugin_paths) { + auto path = make_path_from_name(name, path_prefix); + + if (validate_path(path)) { + debug_log << " " << path << " => Found" << std::endl; + return path; + } + else { + debug_log << " " << path << " => Not found" << std::endl; + } + } + return std::nullopt; +} + +bool JPluginLoader::validate_path(const std::string& path) { + return (access(path.c_str(), F_OK) != -1); +} + + diff --git a/src/libraries/JANA/Services/JPluginLoader.h b/src/libraries/JANA/Services/JPluginLoader.h index 8da4e4ab4..d51f49816 100644 --- a/src/libraries/JANA/Services/JPluginLoader.h +++ b/src/libraries/JANA/Services/JPluginLoader.h @@ -12,6 +12,7 @@ #include #include #include +#include class JComponentManager; @@ -45,6 +46,11 @@ class JPluginLoader : public JService { void attach_plugin(std::string name, std::string path); void resolve_plugin_paths(); + std::pair> extract_name_and_maybe_path(std::string user_plugin); + std::optional find_first_valid_path(std::string name, std::ostringstream& debug_log); + std::string make_path_from_name(std::string name, const std::string& path_prefix); + bool validate_path(const std::string& path); + private: Service m_params {this}; From 629ca916984b7de0bbf4e9771e1d84ab3f337d64 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Mon, 12 Aug 2024 13:57:43 -0400 Subject: [PATCH 3/6] Add optional exception on plugin loading to JTest This lets us write integration tests for the plugin loader --- src/plugins/JTest/JTestMain.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/plugins/JTest/JTestMain.cc b/src/plugins/JTest/JTestMain.cc index b19433086..f7c87cb4f 100644 --- a/src/plugins/JTest/JTestMain.cc +++ b/src/plugins/JTest/JTestMain.cc @@ -23,6 +23,11 @@ void InitPlugin(JApplication *app){ app->Add(new JFactoryGeneratorT()); app->Add(new JFactoryGeneratorT()); + bool except_on_loading = app->RegisterParameter("jtest:except_on_loading", false); + if (except_on_loading) { + throw JException("Planned exception on loading!"); + } + bool write_csv = false; app->SetDefaultParameter("jtest:write_csv", write_csv); if (write_csv) { From d15281a08c6652f4171c3294ece6343dbab81ed9 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 14 Aug 2024 13:37:50 -0400 Subject: [PATCH 4/6] Reimplement basic string ops for C++17 compatibility --- src/libraries/JANA/Services/JPluginLoader.cc | 24 ++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index 6f4444eb0..702958047 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/src/libraries/JANA/Services/JPluginLoader.cc @@ -261,9 +261,22 @@ JPlugin::~JPlugin() { LOG_DEBUG(m_logger) << "Unloaded plugin \"" << m_name << "\"" << LOG_END; } +std::string path_filename_stem(const std::string& filesystem_path) { + // Ideally we would just do this, but we want to be C++17 compatible + // return std::filesystem::path(filesystem_path).filename().stem().string(); + + size_t pos_begin = filesystem_path.find_last_of('/'); + if (pos_begin == std::string::npos) pos_begin = 0; + + size_t pos_end = filesystem_path.find_last_of('.'); + //if (pos_end == std::string::npos) pos_end = filesystem_path.size(); + + return filesystem_path.substr(pos_begin+1, pos_end-pos_begin-1); +} + std::pair> JPluginLoader::extract_name_and_maybe_path(std::string user_name_or_path) { if (user_name_or_path.find('/') != -1) { - std::string name = std::filesystem::path(user_name_or_path).filename().stem().string(); + std::string name = path_filename_stem(user_name_or_path); std::string path = user_name_or_path; return {name, path}; } @@ -275,11 +288,17 @@ std::pair> JPluginLoader::extract_name_a } } +bool ends_with(const std::string& s, char c) { + // Ideally we would just do s.ends_with(c), but we want to be C++17 compatible + if (s.empty()) return false; + return s.back() == c; +} + std::string JPluginLoader::make_path_from_name(std::string name, const std::string& path_prefix) { std::ostringstream oss; oss << path_prefix; - if (!path_prefix.ends_with('/')) { + if (!ends_with(path_prefix, '/')) { oss << "/"; } oss << name; @@ -303,6 +322,7 @@ std::optional JPluginLoader::find_first_valid_path(std::string name return std::nullopt; } + bool JPluginLoader::validate_path(const std::string& path) { return (access(path.c_str(), F_OK) != -1); } From 69c61123bd991a57c999f76d291f5a8fab56729e Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 14 Aug 2024 15:02:31 -0400 Subject: [PATCH 5/6] Get rid of std::optional for C++14 compatibility --- CMakeLists.txt | 2 +- src/libraries/JANA/Services/JPluginLoader.cc | 75 +++++++++++--------- src/libraries/JANA/Services/JPluginLoader.h | 7 +- 3 files changed, 46 insertions(+), 38 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b170b1d29..ad0705997 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,7 +7,7 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON) # Enable -fPIC for all targets # Default the C++ standard to C++17, and validate that they provided one we can use set(CMAKE_CXX_STANDARD 17 CACHE STRING "Set the C++ standard to be used") -if(NOT CMAKE_CXX_STANDARD MATCHES "17|20|23") +if(NOT CMAKE_CXX_STANDARD MATCHES "14|17|20|23") message(FATAL_ERROR "Unsupported C++ standard: ${CMAKE_CXX_STANDARD}") endif() diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index 702958047..13cc617dc 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/src/libraries/JANA/Services/JPluginLoader.cc @@ -128,31 +128,40 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) { // plugins themselves request additional plugins. These get appended to the back of `m_plugins_to_include`. for (int i=0; inext_plugin(name); + const std::string& user_plugin = m_plugins_to_include[i]; + + std::string name; + bool user_provided_path = is_path(user_plugin); + if (user_provided_path) { + name = extract_name_from_path(user_plugin); + } + else { + name = normalize_name(user_plugin); + } if (exclusions.find(name) != exclusions.end() || exclusions.find(name) != exclusions.end()) { LOG_INFO(m_logger) << "Excluding plugin `" << name << "`" << LOG_END; continue; } - // TODO: Check if plugin has already been loaded!!! - bool found_plugin = false; - if (path != std::nullopt) { - found_plugin = validate_path(*path); - if (found_plugin) { - paths_checked << " " << *path << " => Found" << std::endl; + std::string path; + if (user_provided_path) { + if(validate_path(user_plugin)) { + paths_checked << " " << user_plugin << " => Found" << std::endl; + path = user_plugin; } else { - paths_checked << " " << *path << " => Not found" << std::endl; + // User entered an invalid path; `path` variable stays enpty + paths_checked << " " << user_plugin << " => Not found" << std::endl; } } else { - // User didn't provide a path, so we have to search path = find_first_valid_path(name, paths_checked); - found_plugin = (path != std::nullopt); + // User didn't provide a path, so we have to search + // If no valid paths found, `path` variable stays empty } - if (!found_plugin) { + + if (path.empty()) { LOG_ERROR(m_logger) << "Couldn't find plugin '" << name << "'\n" << " Make sure that JANA_HOME and/or JANA_PLUGIN_PATH environment variables are set correctly.\n" @@ -171,7 +180,8 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) { // see an extremely difficult-to-debug error (usually a segfault) stemming from the binary incompatibility // between the host application and the plugin. - attach_plugin(name, *path); // Throws JException on failure + jcm->next_plugin(name); + attach_plugin(name, path); // Throws JException on failure } } @@ -261,31 +271,29 @@ JPlugin::~JPlugin() { LOG_DEBUG(m_logger) << "Unloaded plugin \"" << m_name << "\"" << LOG_END; } -std::string path_filename_stem(const std::string& filesystem_path) { + +bool JPluginLoader::is_path(const std::string user_plugin) { + return user_plugin.find('/') != -1; +} + +std::string JPluginLoader::normalize_name(const std::string user_plugin) { + if (user_plugin.substr(user_plugin.size() - 3) == ".so") { + return user_plugin.substr(0, user_plugin.size() - 3); + } + return user_plugin; +} + +std::string JPluginLoader::extract_name_from_path(const std::string user_plugin) { // Ideally we would just do this, but we want to be C++17 compatible // return std::filesystem::path(filesystem_path).filename().stem().string(); - size_t pos_begin = filesystem_path.find_last_of('/'); + size_t pos_begin = user_plugin.find_last_of('/'); if (pos_begin == std::string::npos) pos_begin = 0; - size_t pos_end = filesystem_path.find_last_of('.'); + size_t pos_end = user_plugin.find_last_of('.'); //if (pos_end == std::string::npos) pos_end = filesystem_path.size(); - return filesystem_path.substr(pos_begin+1, pos_end-pos_begin-1); -} - -std::pair> JPluginLoader::extract_name_and_maybe_path(std::string user_name_or_path) { - if (user_name_or_path.find('/') != -1) { - std::string name = path_filename_stem(user_name_or_path); - std::string path = user_name_or_path; - return {name, path}; - } - else { - if (user_name_or_path.substr(user_name_or_path.size() - 3) == ".so") { - user_name_or_path = user_name_or_path.substr(0, user_name_or_path.size() - 3); - } - return {user_name_or_path, std::nullopt}; - } + return user_plugin.substr(pos_begin+1, pos_end-pos_begin-1); } bool ends_with(const std::string& s, char c) { @@ -294,7 +302,6 @@ bool ends_with(const std::string& s, char c) { return s.back() == c; } - std::string JPluginLoader::make_path_from_name(std::string name, const std::string& path_prefix) { std::ostringstream oss; oss << path_prefix; @@ -306,7 +313,7 @@ std::string JPluginLoader::make_path_from_name(std::string name, const std::stri return oss.str(); } -std::optional JPluginLoader::find_first_valid_path(std::string name, std::ostringstream& debug_log) { +std::string JPluginLoader::find_first_valid_path(const std::string& name, std::ostringstream& debug_log) { for (const std::string& path_prefix : m_plugin_paths) { auto path = make_path_from_name(name, path_prefix); @@ -319,7 +326,7 @@ std::optional JPluginLoader::find_first_valid_path(std::string name debug_log << " " << path << " => Not found" << std::endl; } } - return std::nullopt; + return ""; } diff --git a/src/libraries/JANA/Services/JPluginLoader.h b/src/libraries/JANA/Services/JPluginLoader.h index d51f49816..e5689c9a1 100644 --- a/src/libraries/JANA/Services/JPluginLoader.h +++ b/src/libraries/JANA/Services/JPluginLoader.h @@ -12,7 +12,6 @@ #include #include #include -#include class JComponentManager; @@ -46,8 +45,10 @@ class JPluginLoader : public JService { void attach_plugin(std::string name, std::string path); void resolve_plugin_paths(); - std::pair> extract_name_and_maybe_path(std::string user_plugin); - std::optional find_first_valid_path(std::string name, std::ostringstream& debug_log); + bool is_path(const std::string user_plugin); + std::string normalize_name(const std::string user_plugin); + std::string extract_name_from_path(const std::string user_plugin); + std::string find_first_valid_path(const std::string& name, std::ostringstream& debug_log); std::string make_path_from_name(std::string name, const std::string& path_prefix); bool validate_path(const std::string& path); From 8d257666a18e6d1de7176cbcbbffa38ebe85b8d1 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 14 Aug 2024 15:32:06 -0400 Subject: [PATCH 6/6] Take JANA_HOME off of the plugin loader search path --- src/libraries/JANA/Services/JPluginLoader.cc | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index 13cc617dc..e9a5545b6 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/src/libraries/JANA/Services/JPluginLoader.cc @@ -73,17 +73,7 @@ void JPluginLoader::resolve_plugin_paths() { while (getline(envvar_ss, path, ':')) add_plugin_path(path); } - // 3. Next we look in the plugin directories relative to $JANA_HOME - if (const char* jana_home = getenv("JANA_HOME")) { - add_plugin_path(std::string(jana_home) + "/lib/JANA/plugins"); // In case we did a system install and want to avoid conflicts. - } - - // 4. Finally we look in the JANA install directory. - // By checking here, the user no longer needs to set JANA_HOME in order to run built-in plugins - // such as janadot and JTest. The install directory is supposed to be the same as JANA_HOME, - // but we can't guarantee that because the user can set JANA_HOME to be anything they want. - // It would be nice if nothing in the JANA codebase itself relied on JANA_HOME, although we - // won't be removing it anytime soon because of build_scripts. + // 3. Finally we look in the JANA install directory. add_plugin_path(JVersion::GetInstallDir() + "/lib/JANA/plugins"); }