[runSofa] autoload plugins (2nd version) #301

fredroy · 2017-06-12T16:19:42Z

Implementation of the (2nd part of) proposal #281 .

Quick Reminder:

CMake generates the list of compiled plugins in a file (plugin_list.conf.default) at the configuration stage.
runSofa loads this list at startup
if the user creates a custom plugin_list.conf, it will load this list instead.
add an option to bypass the file (thus disable automatic loading) in runSofa.

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:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings.
does not generate new unit test failures.
does not generate new scene test failures.
does not break API compatibility.
is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

…plication, and add option

damienmarchal · 2017-06-13T20:01:25Z

applications/projects/runSofa/runSofa_test/runSofa_test.cpp

+
+    void SetUp()
+    {
+#ifdef WIN32


Could we have a function in sofa::helper::Utils::getPluginDir() to avoid having #ifdef in the main application ?

damienmarchal · 2017-06-13T20:02:50Z

applications/projects/runSofa/runSofa_test/runSofa_test.cpp

+
+TEST_F(runSofa_test, runSofa_autoload)
+{
+    sofa::helper::system::PluginManager& pm = sofa::helper::system::PluginManager::getInstance();


I suggest you to use:
'using sofa::helper::system::PluginManager ; '
then 'PluginManager' to avoid repeating the long namespace everywhere which is less readable.

damienmarchal · 2017-06-13T20:03:56Z

applications/projects/runSofa/Main.cpp

    {
-        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))


Instead of repeating the full namespace, please use
using sofa::helper::system::DataRepository ;

damienmarchal · 2017-06-13T20:05:51Z

applications/projects/runSofa/runSofa_test/runSofa_test.cpp

@@ -0,0 +1,84 @@
+/******************************************************************************


+1 for adding tests with your new features.

damienmarchal · 2017-06-13T20:10:43Z

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 ?

fredroy · 2017-06-14T07:49:04Z

This first PR was initially to answer to the question how to load automatically plugins we deem "important" such as SofaPython.
This PR was quite easy and fast to implement (especially because people agreed on the previous (failed :( ) PR #253 ). And it would lead to an other PR which will remove Image* things in the core of Sofa (and deps and the JPEG bug in ImageQt)
Anyway, in a second time, I(?) will think about the introspection system which will be detailed in the proposal page #281

damienmarchal · 2017-06-15T15:12:46Z

SofaKernel/framework/sofa/helper/system/PluginManager.cpp

+
+        std::istringstream is(line);
+        is >> pluginPath;
+        is >> version; // information not used for now


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) ?

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

Thanks for the quick answer,
I agree this could be for a future PR.

…est cleanup)

…_plugins2

damienmarchal · 2017-06-26T11:45:11Z

Thanks @fredroy for taking the feedback into account.
To me this PR is ready to merge because it fullfill our checklist and no one said "no go" in 15 days.
I rebuild from a fresh [ci-build]

damienmarchal · 2017-06-27T07:27:48Z

@fredroy in recent builds the test are failling can you have a look.

fredroy · 2017-06-29T10:57:16Z

@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.

damienmarchal · 2017-06-30T12:08:00Z

Thanks @fredroy, I switch this one to ready and will merge it.

fjourdes · 2017-06-30T12:44:15Z

Just one small concern ( that can be included if that is not already the case ).
Does the functionnality currently also work with a debug compilation ?
There may be also some extra care to be taken with multi configurations ide like visual studio, where the same build tree can generate multiple binairies in different folder, and possibly with different names ( for example the '_d' suffix that usually comes along with debug librairies )

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

fredroy · 2017-07-03T11:15:03Z

This feature is working in debug mode (just tested in windows, and it allowed me to find a bug when compiling SOFA with VS....).
But as you mentioned it won't work when you will want to load a binary not suffixed with _d and trying to runSofa in debug mode (e.g it could happen that you don't have access to the debug version of a plugin but still want to debug your application).
Basically PluginManager, if compiled in Debug mode, wants to load "_d" suffixed binaries.

If we want to achieve that:

remove the forced suffix "_d" when loading in debug mode,
the config file must have the "complete" "binary name" and not the target name (or just add the binary name with the target name)

What do you people think ?

damienmarchal · 2017-07-03T21:17:14Z

In debug mode wouldn't it be possible to try the forced "_d" and if the file does not exist try the one without ?

bcarrez · 2017-07-04T07:34:25Z

[ci-build]

fredroy · 2017-07-04T08:21:54Z

@damienmarchal
As @fjourdes stated, it could happen that the target name is just different from the binary (compiled) name.
For now, the CMake process generates the config name using the target name.

damienmarchal · 2017-07-04T21:15:05Z

[ci-build]

damienmarchal · 2017-07-05T08:25:56Z

Let's be pragmatic,
if there is no immediate needs we merge this one in the current state and, if needed, we will implement the non matching names case.

[runSofa] autoload plugins (cherry picked from commit 33690ee) # Conflicts: # SofaKernel/framework/sofa/helper/fixed_array.h # SofaKernel/framework/sofa/helper/types/fixed_array.cpp

fredroy added 7 commits June 12, 2017 15:06

CMake: generates at configure time list of all compiled plugins

0077687

SofaFramework: PluginManager read/write files handles version

649f61a

runSofa: load automatically plugins list (default or customized)

2cc04fb

runSofa: move all plugin config generation CMake code into runSofa ap…

5dea9be

…plication, and add option

Fix paths for Linux and Mac

47f42c5

[runSofa] add test for autoload

03b181a

[runSofa] reflect the change in the CHANGELOG

fb65261

fredroy added enhancement About a possible enhancement location: plugins labels Jun 12, 2017

fredroy requested a review from damienmarchal June 12, 2017 16:26

fredroy added location: project and removed location: plugins labels Jun 13, 2017

[runSofa_test] remove warning

3bc4c51

fredroy force-pushed the autoload_plugins2 branch from c8088a8 to 3bc4c51 Compare June 13, 2017 09:51

damienmarchal reviewed Jun 13, 2017

View reviewed changes

fredroy added 2 commits June 15, 2017 13:58

[SofaHelper] Utils: add helper function to retrieve the plugin directory

30e5bd3

[runSofa] cleanup and use previous implemented function

6f646a5

damienmarchal reviewed Jun 15, 2017

View reviewed changes

damienmarchal added the pr: status to review To notify reviewers to review this pull-request label Jun 20, 2017

fredroy added 2 commits June 23, 2017 17:06

[SofaHelper] PluginManager: add deprecation message + new test (and t…

844ab8c

…est cleanup)

Merge branch 'master' of github.com:sofa-framework/sofa into autoload…

eaf64af

…_plugins2

hugtalbot added the pr: status wip Development in the pull-request is still in progress label Jun 28, 2017

[SofaFramework_test] fix test

2d733a4

fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jun 29, 2017

damienmarchal added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Jun 30, 2017

damienmarchal added pr: status to review To notify reviewers to review this pull-request and removed pr: status ready Approved a pull-request, ready to be squashed labels Jun 30, 2017

[SofaHelper] Fix linking problem in MSVC/Debug mode due to missing im…

66d9d67

…port/export macro in fixed_array

damienmarchal added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Jul 5, 2017

damienmarchal merged commit 33690ee into sofa-framework:master Jul 5, 2017

guparan added this to the v17.12 milestone Jul 6, 2017

guparan modified the milestone: v17.12 Sep 13, 2017

fredroy deleted the autoload_plugins2 branch September 22, 2017 12:11

fredroy mentioned this pull request Feb 5, 2021

Plugins: loading & components introspection #281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runSofa] autoload plugins (2nd version) #301

[runSofa] autoload plugins (2nd version) #301

fredroy commented Jun 12, 2017

damienmarchal Jun 13, 2017

damienmarchal Jun 13, 2017

fredroy Jun 14, 2017

damienmarchal Jun 13, 2017

fredroy Jun 14, 2017

damienmarchal Jun 13, 2017

damienmarchal commented Jun 13, 2017

fredroy commented Jun 14, 2017

damienmarchal Jun 15, 2017

fredroy Jun 16, 2017

damienmarchal Jun 19, 2017

damienmarchal commented Jun 26, 2017 •

edited

Loading

damienmarchal commented Jun 27, 2017

fredroy commented Jun 29, 2017

damienmarchal commented Jun 30, 2017

fjourdes commented Jun 30, 2017 •

edited

Loading

fredroy commented Jul 3, 2017

damienmarchal commented Jul 3, 2017

bcarrez commented Jul 4, 2017

fredroy commented Jul 4, 2017

damienmarchal commented Jul 4, 2017

damienmarchal commented Jul 5, 2017

		@@ -0,0 +1,84 @@
		/******************************************************************************

[runSofa] autoload plugins (2nd version) #301

[runSofa] autoload plugins (2nd version) #301

Conversation

fredroy commented Jun 12, 2017

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

damienmarchal commented Jun 13, 2017

fredroy commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienmarchal commented Jun 26, 2017 • edited Loading

damienmarchal commented Jun 27, 2017

fredroy commented Jun 29, 2017

damienmarchal commented Jun 30, 2017

fjourdes commented Jun 30, 2017 • edited Loading

fredroy commented Jul 3, 2017

damienmarchal commented Jul 3, 2017

bcarrez commented Jul 4, 2017

fredroy commented Jul 4, 2017

damienmarchal commented Jul 4, 2017

damienmarchal commented Jul 5, 2017

damienmarchal commented Jun 26, 2017 •

edited

Loading

fjourdes commented Jun 30, 2017 •

edited

Loading