Load plugins using LoadLibraryEx with LOAD_WITH_ALTERED_SEARCH_PATH on Windows #2368
Conversation
|
Filed as internal issue #USD-8175 |
There was a problem hiding this comment.
Hi @SimonBoorer, I left some notes but it's just some reorganization of what you've already got. Let me know what you think, thanks!
| #if defined(ARCH_OS_WINDOWS) | ||
| _handle = TfDlopen(ArchWindowsPreferredPath(_path).c_str(), ARCH_LIBRARY_ALTERED_SEARCH_PATH, &dsoError); | ||
| #else | ||
| _handle = TfDlopen(_path.c_str(), ARCH_LIBRARY_NOW, &dsoError); | ||
| #endif |
There was a problem hiding this comment.
Instead of having an #ifdef here I think it'd be simpler to have the one function call and just combine ARCH_LIBRARY_NOW | ARCH_LIBRARY_ALTERED_SEARCH_PATH since these are already set to platform-specific values in arch/library.h.
There was a problem hiding this comment.
Could you please add a comment about how ARCH_LIBRARY_ALTERED_SEARCH_PATH on Windows ensures that .dll dependencies in the same directory as the specified library will be picked up without requiring modifications to the environment?
There was a problem hiding this comment.
Also, I just noticed TfDlopen takes a std::string and not a const char*, so if you could drop the .c_str() while you're here that'd be great
| @@ -44,7 +44,7 @@ void* ArchLibraryOpen(const std::string &filename, int flag) | |||
| { | |||
| #if defined(ARCH_OS_WINDOWS) | |||
| arch_lastLibraryError = 0; | |||
| if (void* result = LoadLibrary(filename.c_str())) { | |||
| if (void* result = LoadLibraryEx(filename.c_str(), NULL, flag)) { | |||
There was a problem hiding this comment.
Interestingly enough the docs for the old LoadLibrary mention the same backslash requirement -- I guess it just didn't make an actual difference until this change.
In any case, if we follow the letter of the docs then the filename we pass to LoadLibraryEx must always use backslashes, so I'd advocate for doing the conversion to preferred path here. That way, the logic is centralized and downstream code wouldn't have to be platform-aware. Does that sound OK?
Description of Change(s)
Search for USD plugin dependencies in the folder that the plugin is loaded from as well as the application's directory on Windows. This is to support situations where USD itself is a plugin for an application. See #2367 for more information.
I found when testing LoadLibraryEx that the LOAD_WITH_ALTERED_SEARCH_PATH flag only works when using back slashes instead of forward slashes, so I've added a ArchWindowsPreferredPath method to convert between them based off of std::filesystem::path::make_preferred in C++17.
This limitation is mentioned here:
I originally added ArchPreferredPath that simply returned the input path for MacOS/Linux but decided against the extra copy.
I also made converting to back slashes the responsibility of the caller (PlugPlugin::_Load) rather than ArchLibraryOpen in keeping with LoadLibraryEx.
Happy to revisit either of these decisions.
Fixes Issue(s)
#2367