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

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

Merged
merged 14 commits into from
Jan 29, 2019
Merged

Support BLOCKFACTORY_PLUGIN_PATH environment variable #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9f527ea
Added support to shlibpp to extend plugin search path
diegoferigo Jan 27, 2019
2f4f17b
Introduced new BLOCKFACTORY_PLUGIN_PATH env variable
diegoferigo Jan 27, 2019
aa28a13
Added MockPlugin for unit testing purposes
diegoferigo Jan 27, 2019
064c9b2
Added FactoryUnitTest.cpp file
diegoferigo Jan 27, 2019
8492e65
Fixed checks on minimum CMake version
diegoferigo Jan 28, 2019
29ceb0c
Fix Travis
diegoferigo Jan 28, 2019
f76da67
Handle the Windows folder separator in shlibpp
diegoferigo Jan 28, 2019
48a9ac2
Add debug suffix to windows dll name
diegoferigo Jan 28, 2019
f801272
Disabled test that fails
diegoferigo Jan 28, 2019
f90ae60
Force BUILD_SHARED_LIBS=ON on Windows
diegoferigo Jan 28, 2019
57c50ef
Use the factory counter when allocating objects
diegoferigo Jan 29, 2019
3f1b197
Deallocate the factory in FactoryUnitTest before exiting
diegoferigo Jan 29, 2019
c8adc06
Moved OS-specific shared library name handling to shlibpp
diegoferigo Jan 29, 2019
04b9bed
Improved OS-specific handling of shared library name
diegoferigo Jan 29, 2019
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 .ci/env-file
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ TRAVIS_OS_NAME
TRAVIS_BUILD_DIR
TRAVIS_BUILD_TYPE
TRAVIS_CMAKE_GENERATOR
BUILD_TESTING
VALGRIND_TESTS
35 changes: 20 additions & 15 deletions .ci/script.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ if [ "$TRAVIS_CMAKE_GENERATOR" = "Visual Studio 15 2017" ] ; then
cmake -G"$TRAVIS_CMAKE_GENERATOR" \
-A"${TRAVIS_CMAKE_ARCHITECTURE}" \
-DCMAKE_INSTALL_PREFIX="$INSTALL_PREFIX" \
-DBUILD_TESTING:BOOL=ON .. \
-DBUILD_TESTING:BOOL=${BUILD_TESTING:-0} .. \
..
cmake --build . --config $TRAVIS_BUILD_TYPE
cmake --build . --target INSTALL
Expand All @@ -32,7 +32,7 @@ else
cmake -G"$TRAVIS_CMAKE_GENERATOR" \
-DCMAKE_BUILD_TYPE=$TRAVIS_BUILD_TYPE \
-DCMAKE_INSTALL_PREFIX="$INSTALL_PREFIX" \
-DBUILD_TESTING:BOOL=ON .. \
-DBUILD_TESTING:BOOL=${BUILD_TESTING:-0} .. \
..
cmake --build .
cmake --build . --target install
Expand All @@ -43,22 +43,27 @@ else
cmake --build .
fi

# Run the unit tests with ctest
cd $TRAVIS_BUILD_DIR/build
ctest --output-on-failure
if [ ${BUILD_TESTING:-0} -eq 1 ] ; then

if [ "${VALGRIND_TESTS:-0}" -eq 1 ] ; then
# Run the unit tests with ctest
cd $TRAVIS_BUILD_DIR/build
ctest -T memcheck --output-on-failure || FAILED=1
ctest --output-on-failure

if [ ${FAILED:-0} -eq 1 ] ; then
for log in $(ls Testing/Temporary/MemoryChecker.*.log) ; do
echo
echo "$log"
echo
cat $log
done
exit 1
# Run the memcheck tests with ctest
if [ ${VALGRIND_TESTS:-0} -eq 1 ] ; then
cd $TRAVIS_BUILD_DIR/build
ctest -T memcheck --output-on-failure || FAILED=1

# Print to stdout the output if memcheck fails
if [ ${FAILED:-0} -eq 1 ] ; then
for log in $(ls Testing/Temporary/MemoryChecker.*.log) ; do
echo
echo "$log"
echo
cat $log
done
exit 1
fi
fi

fi
10 changes: 7 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ env:
matrix:
- TRAVIS_BUILD_TYPE="Release" UBUNTU="xenial"
- TRAVIS_BUILD_TYPE="Debug" UBUNTU="xenial"
- TRAVIS_BUILD_TYPE="Release" UBUNTU="bionic"
- TRAVIS_BUILD_TYPE="Debug" UBUNTU="bionic"
- TRAVIS_BUILD_TYPE="Debug" UBUNTU="bionic" VALGRIND_TESTS=1
- TRAVIS_BUILD_TYPE="Release" UBUNTU="bionic" BUILD_TESTING=1
- TRAVIS_BUILD_TYPE="Debug" UBUNTU="bionic" BUILD_TESTING=1
- TRAVIS_BUILD_TYPE="Debug" UBUNTU="bionic" BUILD_TESTING=1 VALGRIND_TESTS=1

# ===================
# STAGE: test (linux)
Expand Down Expand Up @@ -147,11 +147,13 @@ jobs:
after_failure: skip
after_success: skip
env:
BUILD_TESTING=1
TRAVIS_CMAKE_GENERATOR="Xcode"
TRAVIS_BUILD_TYPE="Debug"
- <<: *osx_template
compiler: clang
env:
BUILD_TESTING=1
TRAVIS_CMAKE_GENERATOR="Unix Makefiles"
TRAVIS_BUILD_TYPE="Debug"
# -------------
Expand All @@ -171,12 +173,14 @@ jobs:
after_failure: skip
after_success: skip
env:
BUILD_TESTING=1
TRAVIS_CMAKE_GENERATOR="Visual Studio 15 2017"
TRAVIS_CMAKE_ARCHITECTURE="x64"
TRAVIS_BUILD_TYPE="Debug"
- <<: *windows_template
install: choco install ninja
env:
BUILD_TESTING=1
TRAVIS_CMAKE_GENERATOR="Ninja"
TRAVIS_BUILD_TYPE="Debug"
# ------------
Expand Down
9 changes: 4 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,9 @@ if(NOT CMAKE_CONFIGURATION_TYPES)
endif()

# Libraries type
if(MSVC)
option(BUILD_SHARED_LIBS "Compile BlockFactory as a shared library" FALSE)
else()
option(BUILD_SHARED_LIBS "Compile BlockFactory as a shared library" TRUE)
option(BUILD_SHARED_LIBS "Compile BlockFactory as a shared library" TRUE)
if(MSVC AND NOT ${BUILD_SHARED_LIBS})
message(FATAL_ERROR "BUILD_SHARED_LIBS=OFF is not currently supported on Windows")
endif()

# Build position independent code
Expand Down Expand Up @@ -112,7 +111,7 @@ else()
ENG_LIBRARY
MAIN_PROGRAM
SIMULINK)
if(NOT CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.7)
if(NOT ${CMAKE_MINIMUM_REQUIRED_VERSION} VERSION_LESS 3.7)
message(AUTHOR_WARNING "CMake minimum required version greater than 3.7. You can remove this.")
endif()
endif()
Expand Down
4 changes: 2 additions & 2 deletions cmake/BlockFactoryPlugin.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ function(install_blockfactory_plugin)
PUBLIC_HEADER DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${plugin_name}/Block")
endfunction()

if(${CMAKE_VERSION} VERSION_GREATER_EQUAL 3.13)
if(NOT CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.13)
if(NOT ${CMAKE_VERSION} VERSION_LESS 3.13)
if(NOT ${CMAKE_MINIMUM_REQUIRED_VERSION} VERSION_LESS 3.13)
message(AUTHOR_WARNING
"This version of CMake comes with an improved version of target_sources. "
"Consider to switch the logic substituting global properties.")
Expand Down
118 changes: 116 additions & 2 deletions deps/sharedlibpp/src/SharedLibraryFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@
#include <shlibpp/SharedLibraryClassApi.h>
#include <shlibpp/SharedLibrary.h>

#include <cstdlib>
#include <fstream>
#include <string>
#include <sstream>
#include <sys/stat.h>
#include <vector>

#if defined(_WIN32)
# define shlibpp_struct_stat struct _stat
# define shlibpp_stat ::_stat
Expand All @@ -19,6 +25,12 @@
# define shlibpp_stat ::stat
#endif

#if defined(WIN32) || defined(_WIN32)
#define PATH_SEPARATOR "\\"
#else
#define PATH_SEPARATOR "/"
#endif

class shlibpp::SharedLibraryFactory::Private
{
public:
Expand All @@ -31,6 +43,11 @@ class shlibpp::SharedLibraryFactory::Private
bool isValid() const;
bool useFactoryFunction(void *factory);

void extendSearchPath(const std::string& path);
void readExtendedPathFromEnvironment();
std::string findLibraryInExtendedPath(const std::string& libraryName);
static std::vector<std::string> platformSpecificLibName(const std::string& library);

SharedLibrary lib;
SharedLibraryFactory::Status status;
SharedLibraryClassApi api;
Expand All @@ -45,6 +62,9 @@ class shlibpp::SharedLibraryFactory::Private
int32_t endCheck;
int32_t systemVersion;
const char* factoryName;

std::vector<std::string> extendedPath;
std::string pluginPathEnvVar = "SHLIBPP_PLUGIN_PATH";
};


Expand All @@ -64,6 +84,44 @@ shlibpp::SharedLibraryFactory::Private::Private(int32_t startCheck,
memset(&api, 0, sizeof(SharedLibraryClassApi));
}

std::vector<std::string> shlibpp::SharedLibraryFactory::Private::platformSpecificLibName(const std::string& library)
{

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 .

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#else
return {library + "d.dll", library + ".dll", "lib" + library + ".dll"};
#endif
#elif defined(__linux__)
return {"lib" + library + ".so"};
#elif defined(__APPLE__)
return {"lib" + library + ".dylib"};
#else
#error "This platform not supported by this project"
#endif
}

std::string shlibpp::SharedLibraryFactory::Private::findLibraryInExtendedPath(const std::string& libraryName)
{
std::size_t found = libraryName.find_first_of("\\/");
if (found != std::string::npos) {
return {};
}

for (const auto& path: extendedPath) {
for (const auto& osLibName : platformSpecificLibName(libraryName)){
std::string absolutePath = path + PATH_SEPARATOR + osLibName;

if (std::ifstream(absolutePath)) {
return absolutePath;
}
}
}

return {};
}

bool shlibpp::SharedLibraryFactory::Private::open(const char* dll_name)
{
returnValue = 0;
Expand All @@ -73,9 +131,17 @@ bool shlibpp::SharedLibraryFactory::Private::open(const char* dll_name)
status = Status::None;
error = "";
api.startCheck = 0;
if (!lib.open(dll_name)) {

readExtendedPathFromEnvironment();
std::string pathToLib = findLibraryInExtendedPath(dll_name);

if (pathToLib.empty()) {
pathToLib = dll_name;
}

if (!lib.open(pathToLib.c_str())) {
shlibpp_struct_stat dummy;
if (shlibpp_stat(dll_name, &dummy) != 0) {
if (shlibpp_stat(pathToLib.c_str(), &dummy) != 0) {
status = Status::LibraryNotFound;
} else {
status = Status::LibraryNotLoaded;
Expand Down Expand Up @@ -138,6 +204,44 @@ bool shlibpp::SharedLibraryFactory::Private::useFactoryFunction(void *factory)
return isValid();
}

void shlibpp::SharedLibraryFactory::Private::extendSearchPath(const std::string& path)
{
std::string pathToAdd = path;

if (pathToAdd.back() == '/' || pathToAdd.back() == '\\') {
pathToAdd.pop_back();
}

for (const auto& storedPath : extendedPath) {
if (storedPath == pathToAdd) {
return;
}
}

extendedPath.push_back(pathToAdd);
}

void shlibpp::SharedLibraryFactory::Private::readExtendedPathFromEnvironment()
{
std::string path;
auto content = std::getenv(pluginPathEnvVar.c_str());

if (!content) {
return;
}

std::stringstream envStream(content);

#if defined(_WIN32)
char delim = ';';
#else
char delim = ':';
#endif

while (getline(envStream, path, delim)) {
extendSearchPath(path);
}
}

shlibpp::SharedLibraryFactory::SharedLibraryFactory(int32_t startCheck,
int32_t endCheck,
Expand Down Expand Up @@ -194,6 +298,16 @@ bool shlibpp::SharedLibraryFactory::open(const char* dll_name, const char* facto
return mPriv->open(dll_name);
}

void shlibpp::SharedLibraryFactory::setPluginPathEnvVarName(const std::string &env_var)
{
mPriv->pluginPathEnvVar = env_var;
}

void shlibpp::SharedLibraryFactory::extendSearchPath(const std::string& path)
{
mPriv->extendSearchPath(path);
}

bool shlibpp::SharedLibraryFactory::isValid() const
{
return mPriv->isValid();
Expand Down
13 changes: 13 additions & 0 deletions deps/sharedlibpp/src/shlibpp/SharedLibraryFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,19 @@ class SHLIBPP_API SharedLibraryFactory
bool open(const char *dll_name,
const char *factoryName = nullptr);

/**
* Set the name of the environment variable that extends the search path
* @param env_var The name of the environment variable
*/
void setPluginPathEnvVarName(const std::string& env_var);

/**
* Add path to search for plugins
*
* @param path The new path to be added.
*/
void extendSearchPath(const std::string& path);

/**
* Check if factory is configured and present.
*
Expand Down
7 changes: 7 additions & 0 deletions sources/Core/include/BlockFactory/Core/FactorySingleton.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ class blockfactory::core::ClassFactorySingleton
* @return True if the factory was destroyed, false otherwise
*/
bool destroyFactory(const ClassFactoryData& factorydata);

/**
* @brief Add path to search for plugins
*
* @param path The new path to be added.
*/
void extendPluginSearchPath(const std::string& path);
};

#endif
Loading