Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for plugins that are built as debug or as manualload debug #550

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix for plugins that are built as debug or as manualload debug #550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8201958
fix for plugins not loading when built as debug or manualload debug
Jun 14, 2017
d045232
Merge branch 'master' into debug-plugin-fix
rpavlik Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ endif()

# Shouldn't be changed without being fully aware of the consequences.
set(OSVR_PLUGIN_IGNORE_SUFFIX ".manualload")
set(OSVR_PLUGIN_DEBUG_SUFFIX ".debug")

# Figure out what to do with osvr_json_to_c
if(CMAKE_CROSSCOMPILING OR ANDROID)
Expand Down
3 changes: 3 additions & 0 deletions cmake-local/osvrConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ set(OSVR_CACHED_PLUGIN_DIR "@OSVR_PLUGIN_DIR@" CACHE INTERNAL

set(OSVR_PLUGIN_IGNORE_SUFFIX "@OSVR_PLUGIN_IGNORE_SUFFIX@" CACHE INTERNAL
"The additional suffix for OSVR plugins that are not to be auto-loaded" FORCE)

set(OSVR_PLUGIN_DEBUG_SUFFIX "@OSVR_PLUGIN_DEBUG_SUFFIX@" CACHE INTERNAL
"The additional suffix for OSVR plugins that are built in debug mode" FORCE)

set(OSVR_CACHED_CONFIG_ROOT "@OSVR_CONFIG_ROOT@" CACHE INTERNAL
"The OSVR_CONFIG_ROOT variable for OSVR, for use in installing plugins' sample configs, display descriptors, etc." FORCE)
Expand Down
1 change: 1 addition & 0 deletions src/osvr/PluginHost/PathConfig.h.cmake_in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,6 @@
#define OSVR_PLUGIN_DIR "@OSVR_PLUGIN_DIR@"
#define OSVR_PLUGIN_EXTENSION "@CMAKE_SHARED_MODULE_SUFFIX@"
#define OSVR_PLUGIN_IGNORE_SUFFIX "@OSVR_PLUGIN_IGNORE_SUFFIX@"
#define OSVR_PLUGIN_DEBUG_SUFFIX "@OSVR_PLUGIN_DEBUG_SUFFIX@"

#endif // INCLUDED_PathConfig_h_GUID_ACBCE484_669E_4BA5_6A87_51519A19EE5A
16 changes: 4 additions & 12 deletions src/osvr/PluginHost/RegistrationContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@
namespace osvr {
namespace pluginhost {
static const auto PLUGIN_HOST_LOGGER_NAME = "PluginHost";
#ifdef _MSC_VER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the .debug suffix is no longer MSVC-only, then you should also modify cmake-local/osvrAddPlugin.cmake accordingly (lines 45–48).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See src/osvr/PluginHost/RegistrationContext.cpp, circa line 168 where we check to see if the .manualload suffix appears at the end of the name.

static const auto PLUGIN_HOST_DEBUG_SUFFIX = ".debug";
#endif
namespace fs = boost::filesystem;

struct RegistrationContext::Impl : private boost::noncopyable {
Expand Down Expand Up @@ -88,16 +85,11 @@ namespace pluginhost {
std::string const &name,
OSVR_PluginRegContext ctx,
bool shouldRethrow = false) {
#if defined(_MSC_VER) && !defined(NDEBUG)
// Visual C++ debug runtime: we append to the plugin name.
const std::string decoratedPluginName = name + PLUGIN_HOST_DEBUG_SUFFIX;
#else
const std::string &decoratedPluginName = name;
#endif

log.debug() << "Trying to load a plugin with the name "
<< decoratedPluginName;
<< name;
try {
plugin = libfunc::loadPluginByName(decoratedPluginName, ctx);
plugin = libfunc::loadPluginByName(name, ctx);
return true;
} catch (std::runtime_error const &e) {
log.debug() << "Failed: " << e.what();
Expand Down Expand Up @@ -175,7 +167,7 @@ namespace pluginhost {
// Visual C++ debug runtime: we append to the plugin name. Must only
// load debug plugins iff we're a debug server
const auto isDebugRuntimePlugin =
boost::iends_with(pluginBaseName, PLUGIN_HOST_DEBUG_SUFFIX);
boost::iends_with(pluginBaseName, OSVR_PLUGIN_DEBUG_SUFFIX);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the answer to this, but if we have two suffixes (.debug and .manualload), which order must they appear in if they're both used? Is that order imposed properly by the build tools? Do we want to support allowing the suffixes to appear in any order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the latest OSVR Core snapshot, I'm seeing that plugins that are debug and manualload are named as follows: com_osvr_pluginName.debug.manualload.dll

From osvrAddPlugin.cmake script I confirmed that

  1. Plugin SUFFIX is set to manualload.dll (if it is a manualload plugin) which gets appended to the end of the target name (according to the https://cmake.org/cmake/help/v3.6/prop_tgt/SUFFIX.html)
  2. Plugin DEBUG_POSTIFX is set to .debug which is appended to the target file name (https://cmake.org/cmake/help/v3.6/prop_tgt/CONFIG_POSTFIX.html)
    Hence the order would always be as follows com_osvr_pluginName.debug.manualload.dll for debug, manualload plugins.

#if defined(NDEBUG)
/// This is a non-debug build.
if (isDebugRuntimePlugin) {
Expand Down
17 changes: 13 additions & 4 deletions src/osvr/PluginHost/SearchPath.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
// limitations under the License.

// Internal Includes
#include <osvr/PluginHost/SearchPath.h>
#include <osvr/PluginHost/PathConfig.h>
#include <osvr/PluginHost/SearchPath.h>
#include <osvr/Util/BinaryLocation.h>
#include <osvr/Util/Verbosity.h>

Expand All @@ -33,9 +33,9 @@
#include <boost/range/iterator_range.hpp>

// Standard includes
#include <vector>
#include <string>
#include <algorithm>
#include <string>
#include <vector>

namespace osvr {
namespace pluginhost {
Expand Down Expand Up @@ -172,11 +172,20 @@ namespace pluginhost {
}
const auto pluginBaseName =
pluginCandidate.filename().stem().generic_string();

#if defined(_MSC_VER) && !defined(NDEBUG)
// Visual C++ debug runtime: we append to the plugin name.
const std::string decoratedPluginName =
pluginName + OSVR_PLUGIN_DEBUG_SUFFIX;
#else
const std::string &decoratedPluginName = pluginName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is decoratedPluginName a reference here but not on line 178 above? (Actually, I know the answer. But it's probably better to store a copy in both cases for consistency's sake.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably because it's a copypasta from its original location. I moved this piece of code from RegistrationContext.cpp. Good catch! I'll change that

#endif

/// If the name is right or has the manual load suffix, this is
/// a good one.
if ((pluginBaseName == pluginName) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't pluginName here also be decoratedPluginName?

(pluginBaseName ==
pluginName + OSVR_PLUGIN_IGNORE_SUFFIX)) {
decoratedPluginName + OSVR_PLUGIN_IGNORE_SUFFIX)) {
return pluginPathName.path().generic_string();
}
}
Expand Down