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.

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] FIX PluginRepository initialization #502

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

guparan
Copy link
Contributor

@guparan guparan commented Oct 30, 2017

This should fix failing scene tests due to issofa_plugin merge
Failures were due to wrong plugin_list.conf path implying no plugin loading.

Added some output to understand better were this list is loaded from (especially in case of failure).


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.

@guparan guparan added location: project pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: fix Fix a bug labels Oct 30, 2017
@guparan
Copy link
Contributor Author

guparan commented Oct 30, 2017

[ci-build] [with-scene-tests]

@guparan guparan assigned guparan and unassigned guparan Oct 30, 2017
@guparan guparan added the pr: status to review To notify reviewers to review this pull-request label Oct 30, 2017
@hugtalbot
Copy link
Contributor

Looks awesome, this fix looks like a dream to me!

@hugtalbot hugtalbot merged commit 3c0a5a7 into sofa-framework:master Oct 30, 2017
@hugtalbot hugtalbot 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 Oct 30, 2017
@fjourdes
Copy link
Contributor

fjourdes commented Oct 30, 2017

Is not the problem that the PluginRepository should contain "../lib" instead of "./" on gcc ? I do not remember where libraries are installed on this platform ( ./bin ? ../lib ? ). If ../lib I guess there is already built in trick (maybe in CMake) so that the LD_LIBRARY_PATH ( or RPATH ) is properly set.
Also I believe there is some kind of code duplicate which also brings confusion between PluginRepository global variable and the Utils::getPluginRepository. They should always return the same thing so why not

Utils::getPluginDirectory()
{
return PluginRepository::getFirstPath(); 
}

Or is there some legitimate reason for doing otherwise ?

@fjourdes
Copy link
Contributor

fjourdes commented Nov 7, 2017

@guparan: ultimately what I wanted to achieve with this PR was to roll back to ca0402d in terms of what the PluginRepository contains. I made a mistake when it came to the content of the PluginRepository except for the windows platform which is my most common development environment.
There have been multiple changes made to this file, most for no good, and mostly undocumented, reasons, this was working in the first place.

@guparan
Copy link
Contributor Author

guparan commented Nov 7, 2017

@fjourdes: Ok I understand your suggestion but I didn't touch to PluginRepository construction because I didn't understand what you wanted to do in f93e2b9
Could you explain?

@fjourdes
Copy link
Contributor

fjourdes commented Nov 7, 2017

In f93e2b9 I wanted to restore the default behavior of ca0402d which had PluginRepository initialised with

  • the current working directory ( hence the "./" ) of the applications since on windows platform. By default, on windows the dlls are located in the same directory as the application. For example runSofa and SofaPython.dll are both in the same directory
  • the ../lib directory for other platforms like linux, where the plugins are usually stored in a lib sub folder of the parent directory where the application reside. For example from the directory where the runSofa executable is, you can look for the directory where SofaPython.lib is located by doing cd ../lib

These paths are relative and are compatible both with a build tree and an install tree.

Prior to f93e2b9 the PluginRepository was empty by default, leaving all the executables the responsibility to fill it with these values. Behavior which was changed multiple times compared to ca0402d for a reason yet to be explained. The side effect this change introduced was the requirement to have every unit test executable which attempts to load a plugin to explicitly fill the PluginRepository with the paths where the libraries are located with respect to the executable. This requirement was covered by having most of the unit tests depend on SofaGTest which initialize these default paths for you, so that test scenes that use a RequiredPlugin component do not utterly fail.
Unless a valid argument is raised, the aforementioned paths should be the default ones to use to initialize the PluginRepository global object.
I strongly believe that would these paths have been properly restored by f93e2b9 (my bad), this PR would have never existed on the first place. I mean how much time has been lost already in fixes, and discussions because of this ! Just to in the end roll back to the implementation that was done 5 years ago which was just working

The other thing I mentionned in the comment #502 (comment) is that there should be only one place to store the paths for possible plugin locations so either the method Utils::getPluginDirectory() should use internally of the PluginRepository global object, or the method should be removed entirely for consistency.

@fjourdes
Copy link
Contributor

fjourdes commented Nov 7, 2017

I mean the only thing that could be considered is whether on platforms like gcc the PluginRepository should contain both the working directory and ../lib. But this question has never been an issue so far, so the default behaviour for initialisation should remain the one exposed in ca0402d

@guparan
Copy link
Contributor Author

guparan commented Nov 8, 2017

the ../lib directory for other platforms like linux, where the plugins are usually stored in a lib sub folder of the parent directory where the application reside. For example from the directory where the runSofa executable is, you can look for the directory where SofaPython.lib is located by doing cd ../lib

I understand that and I agree but I'm confused because that's NOT what you were doing here with FileRepository PluginRepository("SOFA_PLUGIN_PATH", "./");.
Nevermind, I'm gonna open a new PR to change this and fix the consistency issue with Utils::getPluginDirectory().

@fjourdes
Copy link
Contributor

fjourdes commented Nov 8, 2017

This is exactly what I mentionned when I said "I strongly believe that would these paths have been properly restored by f93e2b9 (my bad)..."
It should have been restored, but I did not do it properly for the linux plaform because it is not my main working environment ( so it is easy to slip an error here ) and also probably because I was kind of upset to have to deal with this issue on the first place... [EDIT] I did not take the time to go back enough in the log to find back the last version which had the paths properly set. [/EDIT]

@fjourdes
Copy link
Contributor

fjourdes commented Nov 8, 2017

Also further above "I made a mistake when it came to the content of the PluginRepository except for the windows platform which is my most common development environment."

@guparan
Copy link
Contributor Author

guparan commented Nov 8, 2017

Ok it's all clear now, many thanks for your time :-)

@guparan guparan added this to the v17.12 milestone Dec 14, 2017
@guparan guparan deleted the fix_pluginrepository branch February 19, 2018 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants