[PATCH] devtool: Only register each plugin once

[PATCH] devtool: Only register each plugin once

[Ola x Nilsson]

When a devtool plugin is shadowed in a higher-priorty layer the
register_commands method was called on the shadowing plugin once for
each found plugin with that name.  A simple unique operation on the list
of loaded plugins solves that problem.  It may still be a problem that
each plugin - shadowed or not - is loaded and initialized.

Signed-off-by: Ola x Nilsson <ola.x.nilsson@axis.com>
---
 scripts/devtool | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/devtool b/scripts/devtool
index 0c32c50..7b134a6 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -317,7 +317,7 @@ def main():
         parser_create_workspace.add_argument('--create-only', action="store_true", help='Only create the workspace layer, do not alter configuration')
         parser_create_workspace.set_defaults(func=create_workspace, no_workspace=True)
 
-    for plugin in plugins:
+    for plugin in set(plugins):
         if hasattr(plugin, 'register_commands'):
             plugin.register_commands(subparsers, context)
 
-- 
2.1.4

Re: [PATCH] devtool: Only register each plugin once

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On Mon, Oct 10, 2016 at 7:19 AM, Ola x Nilsson <ola.x.nilsson@axis.com>
wrote:

> When a devtool plugin is shadowed in a higher-priorty layer the
> register_commands method was called on the shadowing plugin once for
> each found plugin with that name.  A simple unique operation on the list
> of loaded plugins solves that problem.  It may still be a problem that
> each plugin - shadowed or not - is loaded and initialized.
>
> Signed-off-by: Ola x Nilsson <ola.x.nilsson@axis.com>
> ---
>  scripts/devtool | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/devtool b/scripts/devtool
> index 0c32c50..7b134a6 100755
> --- a/scripts/devtool
> +++ b/scripts/devtool
> @@ -317,7 +317,7 @@ def main():
>          parser_create_workspace.add_argument('--create-only',
> action="store_true", help='Only create the workspace layer, do not alter
> configuration')
>          parser_create_workspace.set_defaults(func=create_workspace,
> no_workspace=True)
>
> -    for plugin in plugins:
> +    for plugin in set(plugins):
>          if hasattr(plugin, 'register_commands'):
>              plugin.register_commands(subparsers, context)
>

This is directly comparing the plugin modules or classes, which, if there’s
shadowing going on, will almost certainly be different, and we’ll still end
up with the same commands registered multiple times. And of course, use of
set will also change the plugin command registration order. If we’re going
to ignore commands on some of the plugins, it should have avoided loading
them entirely, not load them and then prevent registration. We should
change the plugin *loading* to only load the highest priority (first seen
in bbpath) file for a given .py, based on the file / module name, IMO.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2548 bytes --]

Re: [PATCH] devtool: Only register each plugin once

[Ola x Nilsson]

> From: kergoth@gmail.com [mailto:kergoth@gmail.com] On Behalf Of Christopher Larson
> Sent: den 10 oktober 2016 21:13
>> On Mon, Oct 10, 2016 at 7:19 AM, Ola x Nilsson <ola.x.nilsson@axis.com> wrote:
>> When a devtool plugin is shadowed in a higher-priorty layer the
>> register_commands method was called on the shadowing plugin once for
>> each found plugin with that name.  A simple unique operation on the list
>> of loaded plugins solves that problem.  It may still be a problem that
>> each plugin - shadowed or not - is loaded and initialized.
>>
>> Signed-off-by: Ola x Nilsson <ola.x.nilsson@axis.com>
>> ---
>> scripts/devtool | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/devtool b/scripts/devtool
>> index 0c32c50..7b134a6 100755
>> --- a/scripts/devtool
>> +++ b/scripts/devtool
>> @@ -317,7 +317,7 @@ def main():
>>         parser_create_workspace.add_argument('--create-only', action="store_true", help='Only create the workspace layer, do not alter configuration')
>>         parser_create_workspace.set_defaults(func=create_workspace, no_workspace=True)
>>
>> -    for plugin in plugins:
>> +    for plugin in set(plugins):
>>         if hasattr(plugin, 'register_commands'):
>>             plugin.register_commands(subparsers, context)
>>
>> This is directly comparing the plugin modules or classes, which, if there’s shadowing going on, 
>> will almost certainly be different, and we’ll still end up with the same commands registered multiple times. 
>> And of course, use of set will also change the plugin command registration order. If we’re going to ignore 
>> commands on some of the plugins, it should have avoided loading them entirely, not load them and then 
>> prevent registration. We should change the plugin *loading* to only load the highest priority 
>>(first seen in bbpath) file for a given .py, based on the file / module name, IMO.

Agreed, this does not work as intended.  Please ignore this patch, I'll try to produce something better.

The way the plugin loader works, there can only be a single plugin loaded with a specific file name. 
So shadowing occurs, but probably not the way we want. I think the last found plugin will win,  that is the plugin in the lowest priority layer.

Do we want shadowing to work on plugin or command level?

-- 
Ola Nilsson