Review Request 121775: Correctly load dependencies for plugins

[Milian Wolff]

> On Jan. 3, 2015, 2:23 p.m., Milian Wolff wrote:
> > I wonder why I never saw this issue. Can you explain what you are encountering? Also, adding a unit test would help here to ensure we never run into this issue again.
> > 
> > Generally, the code for checkForDependencies ensures that all dependencies are available and none are disabled. Only then should potential dependencies be loaded. Your code now always tries to load dependencies, no? Even when one of them is disabled or not existing.
> > 
> > so -1 from my side until you give more information on what issue you are encountering and trying to solve.
> 
> Sergey Kalinichev wrote:
>     >Generally, the code for checkForDependencies ensures that all dependencies are available and none are disabled.
>     
>     Not exactly. It checks all loaded plugins to see if dependencies satisfied. And returns all interfaces that's not yet loaded/not available
>     
>     >Only then should potential dependencies be loaded. 
>     
>     Yes.
>     
>     >Your code now always tries to load dependencies, no?
>     
>     No. That's what happens now. Run KDevelop with debug channels enabled and search for "Missing dependencies:" messages. You won't find any because checkForDependencies checks all available plugins instead of loaded ones. So at the point after checkForDependencies call there could be some plugins that not yet loaded. And then loadDependencies tries to load all dependendencies (even if some are already loaded), and only because of that everything works.
>     
>     >I wonder why I never saw this issue.
>     
>     Because currently everything works
>     
>     >so -1 from my side until you give more information on what issue you are encountering and trying to solve
>     
>     Some plugins didn't load for me. So I began investigating and encountered this weird behaviour. But in the end turned out that they didn't load because weren't rebuild properly.
>     
>     >Also, adding a unit test would help here to ensure we never run into this issue again.
>     
>     I don't think we need this. If it dosn't work some plugins won't load -> some unit tests won't pass.
> 
> Milian Wolff wrote:
>     I'm not convinced by your analysis of what checkForDependencies is currently doing. Especially, if everything works, then why do we need to change anything? I don't get any "missing dependencies" because all plugins are normally enabled. If I disable e.g. git, the kdeprovider plugin won't be loaded either. Is that not the case? I can't test right now, but will do so later. Please also test this with your patch.
>     
>     I still think that if this goes in, it should get a unit test. Failing "integration tests" are much harder to debug, compared to a simple test failure in "TestPluginController::testDependencyLoading".
> 
> Sergey Kalinichev wrote:
>     >Especially, if everything works, then why do we need to change anything?
>     
>     Like I already told, it works only by accident.
>     
>     >If I disable e.g. git, the kdeprovider plugin won't be loaded either. Is that not the case?
>     
>     Yes, it's.
>     
>     >I don't get any "missing dependencies" because all plugins are normally enabled.
>     
>     In your case kdeprovider depends on git. Let's say first kdeprovider is loaded. The controller sees, that it depends on git and it's not yet loaded, so git is a "missing dependency" for kdeprovider and must be loaded. That how it works with the patch.
>     But now it works like this: First kdeprovider is loaded. The controller sees no dependencies (as checkForDependencies checks all *available* plugins). And then it tries to load *all* dependencies anyway(including git)!
>     
>     >Failing "integration tests" are much harder to debug, compared to a simple test failure in "TestPluginController::testDependencyLoading".
>     
>     Yes, I agree. Still I'm very reluctand to do so. Also there is e.g. a standardoutputview test, that'll assert if the plugin is not loaded, so it shouldn't be very hard to find the culprit...
>     But, if you insist I'll see what I could do.
> 
> Milian Wolff wrote:
>     checkForDependencies is simply not taking loaded/unloaded into account, and there is no reason it should. It only cares about enabled/disabled, which is something else alltogether. As you say, the code ensures all dependencies are loaded, which is fine at that point, since we verified before that all dependencies are enabled and can thus be fulfilled.
>     
>     Why do you think this is an "accident"?
> 
> Sergey Kalinichev wrote:
>     >checkForDependencies is simply not taking loaded/unloaded into account, and there is no reason it should. It only cares about enabled/disabled
>     
>     Ok, but there is already loadDependencies that does exactly the same.
>     
>     >Why do you think this is an "accident"?
>     
>     Well, once again, please, look at the code!
>     
>     Especially at: plugincontroller.cpp\:511 At that point missingInterfaces can never contain anything (because if it contains something checkForDependencies returns false). That should already tell you, that whoever wrote the code didn't fully understand how it works.
>     Second, with current approach, what is the point in having checkForDependencies? No matter what dependencies it finds, they never used, instead code below loads **all** dependencies anew. Equally well you could simply remove checkForDependencies and call directly loadDependencies, and if can't load something then report an error.

I did look at the code just now, again. There was indeed a new code path that tried to output the missing dependencies when checkDependencies returned true - which can never be the case. So this definitely can be removed. But I've gone ahead and cleaned up the whole loadPluginInternal method, while at it. I'll put up a new review request for that.


- Milian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121775/#review73014
-----------------------------------------------------------


On Jan. 5, 2015, 12:30 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121775/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2015, 12:30 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> It fixes 2 bugs:
> 
> 1) checkForDependencies returns false if there are some dependencies, but the code that calls this method threats false return code as an error, so it doesn't try to load any dependencies at all.
> 2) checkForDependencies must check loaded plugins (instead of all available) for satisfied dependencies. As a result currently it almost always returns true
> 
> So because of these 2 bugs interconnected currently everything works.
> 
> 
> Diffs
> -----
> 
>   shell/plugincontroller.h 89b0b8b 
>   shell/plugincontroller.cpp f70fc64 
> 
> Diff: https://git.reviewboard.kde.org/r/121775/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150106/1bd60859/attachment.html>