Fix for plugins that are built as debug or as manualload debug #550
Conversation
| @@ -51,9 +51,6 @@ | |||
| namespace osvr { | |||
| namespace pluginhost { | |||
| static const auto PLUGIN_HOST_LOGGER_NAME = "PluginHost"; | |||
| #ifdef _MSC_VER | |||
There was a problem hiding this comment.
If the .debug suffix is no longer MSVC-only, then you should also modify cmake-local/osvrAddPlugin.cmake accordingly (lines 45–48).
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
- Plugin
SUFFIXis set tomanualload.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) - Plugin
DEBUG_POSTIFXis set to.debugwhich 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 followscom_osvr_pluginName.debug.manualload.dllfor debug, manualload plugins.
| const std::string decoratedPluginName = | ||
| pluginName + OSVR_PLUGIN_DEBUG_SUFFIX; | ||
| #else | ||
| const std::string &decoratedPluginName = pluginName; |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
| #else | ||
| const std::string &decoratedPluginName = pluginName; | ||
| #endif | ||
|
|
||
| /// If the name is right or has the manual load suffix, this is | ||
| /// a good one. | ||
| if ((pluginBaseName == pluginName) || |
There was a problem hiding this comment.
Shouldn't pluginName here also be decoratedPluginName?
|
@mars979 is this still an issue/open? Any response to the review comments? |
When plugin is built as debug or as manulload debug you would run into this scenarios:
This PR fixes the issue by avoiding appending extra extensions for debug plugins and use the correct order for manualload debug plugins.