-
Notifications
You must be signed in to change notification settings - Fork 311
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.
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
[runSofa] autoload plugins (2nd version) #301
[runSofa] autoload plugins (2nd version) #301
Conversation
…plication, and add option
c8088a8
to
3bc4c51
Compare
|
||
void SetUp() | ||
{ | ||
#ifdef WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a function in sofa::helper::Utils::getPluginDir() to avoid having #ifdef in the main application ?
|
||
TEST_F(runSofa_test, runSofa_autoload) | ||
{ | ||
sofa::helper::system::PluginManager& pm = sofa::helper::system::PluginManager::getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you to use:
'using sofa::helper::system::PluginManager ; '
then 'PluginManager' to avoid repeating the long namespace everywhere which is less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
{ | ||
std::ostringstream no_error_message; // no to get an error on the console if SofaPython does not exist | ||
sofa::helper::system::PluginManager::getInstance().loadPlugin("SofaPython",&no_error_message); | ||
if (sofa::helper::system::DataRepository.findFile(configPluginPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of repeating the full namespace, please use
using sofa::helper::system::DataRepository ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok (bis)
@@ -0,0 +1,84 @@ | |||
/****************************************************************************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding tests with your new features.
Hi fred, Thanks for this PR. I made some totally minor cosmetic comment in the source code. I understand correctly what it does is that is loads by default the plugin that are compiled with sofa. Maybe I misunderstood the initial purpose of your first PR but I though it was more to autoload a plugin if the one of its component was requested from the scene. Is this stil the purpose of the PR ? |
This first PR was initially to answer to the question how to load automatically plugins we deem "important" such as SofaPython. |
|
||
std::istringstream is(line); | ||
is >> pluginPath; | ||
is >> version; // information not used for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old ini file does not have the version number.
Maybe you can implement a backward compatibility hook, I'm not sure this could work like that but
here is the idea:
is >> pluginPath
if( is.eof() ){
msg_deprecated() << "You are using a deprecated version of the fiel.ini... blabblha"
...
Why not having the version number into the filename (as linux is doing libpng.1.0.so) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it already work with previous format (w/o version) but you're right about a deprecated message.
About the version thing in the filename, it is done already on Mac and Linux (e.g libFredPlugin.0.1.dylib is created and a symbolic link libFredPlugin points to it).
I guess you would like to load a specific version, e.g
CImgPlugin 0.5 then load CImgPlugin.0.5.dylib if not warning/error about the version, right ?
I think it should be the purpose of a future PR which will
- make version consistent between (all?) plugin's CMakefile.txt version (set_target_properties(${PROJECT_NAME} PROPERTIES VERSION ${PROJECT_VERSION})) and getVersion() in the initPluginXXXX.cpp
- add version to generated Windows libraries (it does not, and not really possible with symbolic link a la Unix)
- add checks and a version parameter to PluginManager loading functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick answer,
I agree this could be for a future PR.
Thanks @fredroy for taking the feedback into account. |
@fredroy in recent builds the test are failling can you have a look. |
@damienmarchal tests are fixed (modifying a map while iterating it is always bad...) ; a failure on gcc5.4 but it seems to be the hiccup failing test. |
Thanks @fredroy, I switch this one to ready and will merge it. |
Just one small concern ( that can be included if that is not already the case ). On the "bright side", if the functionality is not yet there in this PR, I presume all the necessary informations can be extracted from the cmake target properties, so it should be feasible. Also it would be more generic since the cmake target name, and the name of the generated binary can differ ( even if this is not a common practice ) |
…port/export macro in fixed_array
This feature is working in debug mode (just tested in windows, and it allowed me to find a bug when compiling SOFA with VS....). If we want to achieve that:
What do you people think ? |
In debug mode wouldn't it be possible to try the forced "_d" and if the file does not exist try the one without ? |
[ci-build] |
@damienmarchal |
[ci-build] |
Let's be pragmatic, |
[runSofa] autoload plugins (cherry picked from commit 33690ee) # Conflicts: # SofaKernel/framework/sofa/helper/fixed_array.h # SofaKernel/framework/sofa/helper/types/fixed_array.cpp
Implementation of the (2nd part of) proposal #281 .
Quick Reminder:
plugin_list.conf.default
) at the configuration stage.plugin_list.conf
, it will load this list instead.Everything is done in the runSofa application (+small patch in the macro sofa_add_generic() in SofaFramework CMake modules ; that allows us to have a list of all added targets which is not possible in CMake 3.1) .
This PR:
Reviewers will merge only if all these checks are true.