Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

diegoferigo · 2019-01-29T10:41:45Z

Introduced preliminary support to shlibpp to extend plugin search path
Introduced support to SHLIBPP_PLUGIN_PATH environment variable inside shlibpp
Introduced support to BLOCKFACTORY_PLUGIN_PATH environment variable for finding plugins
Added new test FactoryUnitTest.cpp

P.s. for the time being I will keep the shlibpp changes local to this project. I will open a PR upstream in the future.

The path can be extended in two ways: - Defining a SHLIBPP_PLUGIN_PATH env variables - Calling the new extendSearchPath method

The paths contained in this environment variable will have the priority over the system paths of the linker

On xenial the Catch2 CMake functions fail if there is more than one test. This commit disables temporary ctest on xenial.

diegoferigo · 2019-01-29T10:45:45Z

What's still puzzling me that in Windows everything works only if BUILD_SHARED_LIBS is turned on. @traversaro any clue?

traversaro · 2019-01-29T10:52:06Z

What's still puzzling me that in Windows everything works only if BUILD_SHARED_LIBS is turned on. @traversaro any clue?

What is strange about that?

.ci/script.sh

traversaro · 2019-01-29T10:55:42Z

You are exporting all symbols in https://github.com/robotology/blockfactory/pull/32/files#diff-af3b638bc2a3e6c650974192a53c7291L92 , so that should work fine with shared libraries unless you have some static variables to export.

traversaro · 2019-01-29T10:59:24Z

For all the file path stuff did you considered to use std:::filesystem/std::experimental::filesystem?

traversaro

The only important comment is the order with which plugins are found in Windows when both Release and Debug plugins are available.
Furthermore, it is probably necessary to document the search PATH of the plugins, but this is probably something that can be discussed at the shlibpp PR level.

deps/sharedlibpp/src/SharedLibraryFactory.cpp

traversaro · 2019-01-29T11:11:35Z

deps/sharedlibpp/src/SharedLibraryFactory.cpp

+{
+
+#if defined(_WIN32)
+    return {library + ".dll", library + "d.dll", "lib" + library + ".dll"};


I am a bit confused by this. Why is "lib" + library + ".dll" is included and "lib" + library + "d.dll" not?

Furthermore, if the order of the vector indicates the order in which the candidates are searched for, I think that depending on whether BlockFactory is compiled in release or in debug, the order needs to be changed to make sure that in Debug builds the library + "d.dll" is searched first. It could make sense to still have the normal one as a fallback, but if both are found and BlockFactory is compiled in debug, for sure library + "d.dll" is the one to load.

I am a bit confused by this. Why is "lib" + library + ".dll" is included and "lib" + library + "d.dll" not?

Because this case happens only if you don't use the VS generator. This is for instance the case with Ninja, and since it is not a multi-config generator the debug suffix is not used.

For what concern the order, yes, on windows it actually makes sense to define it wrt to the CMAKE_BUILD_TYPE. There's always the caveat of linking Release code to Release code, and I think this applies also to plugin libraries.

Because this case happens only if you don't use the VS generator. This is for instance the case with Ninja, and since it is not a multi-config generator the debug suffix is not used.

Cool. So if you use Ninja and compile BlockFactory in Debug, the libraries are compiled with the lib prefix and without the d suffix, even if CMAKE_DEBUG_POSTFIX is set to d in

blockfactory/CMakeLists.txt

Line 95 in 5a0b977

set(CMAKE_DEBUG_POSTFIX "d")

?

Ack then, even if it really seems to be a CMake+Ninja+MSVC bug.

For what concern the order, yes, on windows it actually makes sense to define it wrt to the CMAKE_BUILD_TYPE. There's always the caveat of linking Release code to Release code, and I think this applies also to plugin libraries.

Nitpick: CMAKE_BUILD_TYPE is ignored for multiple config generator. Do you have any idea of where this logic in YARP? As far as I know, in YARP that logic is is working fine.

Ack then, even if it really seems to be a CMake+Ninja+MSVC bug.

In my tests I was using g++ with Ninja (check travis). I am not 100% sure of the outcome of the combo of MSVC compiler + Ninja generator. I guess it generates the same things of using just MSVC.

Ahh, sorry I did not understand we wanted to support Mingw.
In that case, in that case it make sense, the CMAKE_DEBUG_POSTFIX is only set for MSVC, not for WIN32.

Nitpick: CMAKE_BUILD_TYPE is ignored for multiple config generator. Do you have any idea of where this logic in YARP? As far as I know, in YARP that logic is is working fine.

You mean in shlibpp? I didn't find it, I think that this portion of code already handles it:

blockfactory/deps/sharedlibpp/src/SharedLibrary.cpp

Lines 56 to 69 in 7b9033c

#if defined(_WIN32)

mPriv->implementation = (void*)LoadLibrary(filename);

LPTSTR msg = nullptr;

FormatMessage(

FORMAT_MESSAGE_FROM_SYSTEM |FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_IGNORE_INSERTS,

nullptr, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),

(LPTSTR)&msg, 0, nullptr);

if(msg != nullptr) {

mPriv->err_message = std::string(msg);

// release memory allocated by FormatMessage()

LocalFree(msg); msg = nullptr;

}

return (mPriv->implementation != nullptr);

In the example of shlibpp you must pass the OS-specific name (see test). I added the support of doing it automatically in this PR.

Actually, I just realized that if the library is not in the extended search path, I pass only the libname. This case would cover the case where the library is in the linker search path, and I'm not sure dlopen can be called without specifying the prefix (lib) and suffix (.so) of the library.

You mean in shlibpp? I didn't find it, I think that this portion of code already handles it:

No, I meant in YARP. I found it, it is in https://github.com/robotology/yarp/blob/master/src/libYARP_OS/src/YarpPlugin.cpp#L69 .

Regarding mingw, in theory the mingw would be searched first if BlockFactory is compiled with mingw, as Mingw's and MSVC's C++ ABI are not compatible. However, the trick of <name>d.dll, name.dll to distinguish the various ABI is just a dirty trick (there are a few other differences that should be considered to fully determine the ABI compatibility), so I guess it is not tragic if we search it later, as in most cases Mingw-libraries and MSVC-libraries are not installed in the same prefix.

No, I meant in YARP. I found it, it is in https://github.com/robotology/yarp/blob/master/src/libYARP_OS/src/YarpPlugin.cpp#L69 .

👍

Btw I think that YARP code does not work well with icc ^^ I'm not sure tho if we support it.

Regarding mingw, in theory the mingw would be searched first if BlockFactory is compiled with mingw, as Mingw's and MSVC's C++ ABI are not compatible. However, the trick of d.dll, name.dll to distinguish the various ABI is just a dirty trick (there are a few other differences that should be considered to fully determine the ABI compatibility), so I guess it is not tragic if we search it later, as in most cases Mingw-libraries and MSVC-libraries are not installed in the same prefix.

These are edge cases at this stage. We never stated the we support Mingw and the installation instructions on Windows only mention MSVC. I think we can postpone safely these cases.

diegoferigo · 2019-01-29T11:14:01Z

For all the file path stuff did you considered to use std:::filesystem/std::experimental::filesystem?

Yes but I was not 100% sure that it would work on all compilers. Do you have any experience?

traversaro · 2019-01-29T11:22:54Z

Yet but I was not 100% sure that it would work on all compilers. Do you have any experience?

Yes, but just in Ubuntu, see https://github.com/robotology/gazebo-fmi/blob/master/.travis.yml . It works fine (with the proper flags, see https://github.com/robotology/gazebo-fmi/blob/733ef7c66cdf8241ab8c91c83fb34148f4716ed7/libraries/private-utils/CMakeLists.txt#L24). Apparently there could be problems on macOS, see https://stackoverflow.com/questions/49577343/filesystem-with-c17-doesnt-work-on-my-mac-os-x-high-sierra/49600206 . It is probably indeed better to avoid it for now.

diegoferigo · 2019-01-29T11:23:07Z

You are exporting all symbols in https://github.com/robotology/blockfactory/pull/32/files#diff-af3b638bc2a3e6c650974192a53c7291L92 , so that should work fine with shared libraries unless you have some static variables to export.

I didn't get it entirely. The factory test fails on Windows if BUILD_SHARED_LIBS=OFF, and this means that the plugin is not found / fails to load. But it is something not related to the plugin library itself since it is forced to get compiled as SHARED:

blockfactory/cmake/BlockFactoryPlugin.cmake

Lines 54 to 57 in 7b9033c

    
               add_library(${plugin_name} SHARED 
        
                   "${plugin_headers}" 
        
                   "${plugin_sources}" 
        
                   "${${prefix}_EXTRA_SOURCES}")

I still have to figure out what can be the problem.

traversaro · 2019-01-29T11:32:16Z

I didn't get it entirely.

Sorry, I was confused and thought you were surprised that it worked with BUILD_SHARED_LIBS to ON, not that it was not working with BUILD_SHARED_LIBS set to OFF.

diegoferigo · 2019-01-29T12:06:51Z

Do you think if would be safe setting by default BUILD_SHARED_LIBS=ON as a non-cache variable, at least on windows?

traversaro · 2019-01-29T13:19:15Z

Do you think if would be safe setting by default BUILD_SHARED_LIBS=ON as a non-cache variable, at least on windows?

Not sure, I would probably just print a clear error if BUILD_SHARED_LIBS set to OFF is used with Windows, setting BUILD_SHARED_LIBS without the possibility to change it may be counterintuitive w.r.t. to the typical CMake user experience.

diegoferigo · 2019-01-29T14:20:52Z

@traversaro Addressed all the requested changes

diegoferigo added 9 commits January 28, 2019 14:29

Added support to shlibpp to extend plugin search path

9f527ea

The path can be extended in two ways: - Defining a SHLIBPP_PLUGIN_PATH env variables - Calling the new extendSearchPath method

Introduced new BLOCKFACTORY_PLUGIN_PATH env variable

2f4f17b

The paths contained in this environment variable will have the priority over the system paths of the linker

Added MockPlugin for unit testing purposes

aa28a13

Added FactoryUnitTest.cpp file

064c9b2

Fixed checks on minimum CMake version

8492e65

Fix Travis

29ceb0c

On xenial the Catch2 CMake functions fail if there is more than one test. This commit disables temporary ctest on xenial.

Handle the Windows folder separator in shlibpp

f76da67

Add debug suffix to windows dll name

48a9ac2

Disabled test that fails

f801272

diegoferigo added complexity::medium component::core pr::status::feedback labels Jan 29, 2019

diegoferigo self-assigned this Jan 29, 2019

diegoferigo requested a review from traversaro January 29, 2019 10:45

traversaro reviewed Jan 29, 2019

View reviewed changes

.ci/script.sh Show resolved Hide resolved

traversaro requested changes Jan 29, 2019

View reviewed changes

diegoferigo added 4 commits January 29, 2019 15:16

Force BUILD_SHARED_LIBS=ON on Windows

f90ae60

Use the factory counter when allocating objects

57c50ef

Deallocate the factory in FactoryUnitTest before exiting

3f1b197

Moved OS-specific shared library name handling to shlibpp

c8adc06

diegoferigo force-pushed the feature/pluginLocation branch from 74a7a0e to 954d04a Compare January 29, 2019 14:17

Improved OS-specific handling of shared library name

04b9bed

diegoferigo force-pushed the feature/pluginLocation branch from 954d04a to 04b9bed Compare January 29, 2019 14:19

traversaro approved these changes Jan 29, 2019

View reviewed changes

diegoferigo merged commit 39b3d44 into robotology:master Jan 29, 2019

diegoferigo deleted the feature/pluginLocation branch January 29, 2019 15:52

diegoferigo added pr::resolution::merged and removed pr::status::feedback labels Jan 29, 2019

diegoferigo mentioned this pull request Jan 31, 2019

Add <prefix>/lib/rtf to LD_LIBRARY_PATH ? robotology/robotology-superbuild#95

Closed

traversaro mentioned this pull request Apr 3, 2019

Add support for using SHLIBPP_PLUGIN_PATH or a custom *_PLUGIN_PATH enviromental variable to specify the location of plugins in the system ami-iit/sharedlibpp#7

Closed

traversaro mentioned this pull request Jul 3, 2020

Extend plugin paths robotology/blocktest#34

Merged

3 tasks

traversaro mentioned this pull request Nov 3, 2024

Align shlibpp with the version vendored in blockfactory and add support for loading plugins by looking in SHLIBPP_PLUGIN_PATH env variable ami-iit/sharedlibpp#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

diegoferigo commented Jan 29, 2019 •

edited

Loading

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro left a comment

traversaro Jan 29, 2019

diegoferigo Jan 29, 2019

traversaro Jan 29, 2019

traversaro Jan 29, 2019

diegoferigo Jan 29, 2019

traversaro Jan 29, 2019

diegoferigo Jan 29, 2019

traversaro Jan 29, 2019

traversaro Jan 29, 2019

diegoferigo Jan 29, 2019

diegoferigo commented Jan 29, 2019 •

edited

Loading

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

	#if defined(_WIN32)
	mPriv->implementation = (void*)LoadLibrary(filename);
	LPTSTR msg = nullptr;
	FormatMessage(
	FORMAT_MESSAGE_FROM_SYSTEM \|FORMAT_MESSAGE_ALLOCATE_BUFFER \| FORMAT_MESSAGE_IGNORE_INSERTS,
	nullptr, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
	(LPTSTR)&msg, 0, nullptr);

	if(msg != nullptr) {
	mPriv->err_message = std::string(msg);
	// release memory allocated by FormatMessage()
	LocalFree(msg); msg = nullptr;
	}
	return (mPriv->implementation != nullptr);

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

Conversation

diegoferigo commented Jan 29, 2019 • edited Loading

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro commented Jan 29, 2019

traversaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo commented Jan 29, 2019 • edited Loading

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

traversaro commented Jan 29, 2019

diegoferigo commented Jan 29, 2019

diegoferigo commented Jan 29, 2019 •

edited

Loading

diegoferigo commented Jan 29, 2019 •

edited

Loading