[MERGE] make plugins use the right suffixes

[Robert Collins]

On Tue, 2006-08-01 at 08:41 -0500, John Arbash Meinel wrote:
> Robert Collins wrote:
> >>>> import imp
> >>>> # handwave - do a directory listing of both system plugins and user
> >>>> # plugins and strip back the paths to the first '.'. Then:
> >>>> for name in candidate_set:
> >>>>     try:
> >>>>         mod_info = imp.find_module(name, [user_plugin_dir,
> > system_plugin_dir])
> >>>>     except ImportError:
> >>>>         # no module by this name -
> >>>>         mutter()
> >>>>     module = imp.load_module('bzrlib.plugins.' + name, mod_info[0],
> > mod_info[1], mod_info[2])
> >>>>     setattr('bzrlib.plugins', name, module)
> > 
> > That should be all we need to do. find_module takes care of pyo vs pyc
> > vs __init__ etc etc etc.
> 
> So the problem with this is it is assuming that every file that may be
> in the path is a real module that you want to load.
> So if I dropped in a 'README.txt' it would try to load the 'README'
> plugin, and naturally it would fail. You do handle that and just have a
> simple mutter, so it isn't terrible.
> 
> For modules ending in 'module.' we could also create 2 entries. foo and
> foomodule, preferring to load the first form. (Very few people would
> name their module foomodule.py).

We could, so the question is 'which form of confusion is better' :). I'm
inclinded to say 'supporting foomodule.so is worse' simply because itse
more code for us.

> > 
> > Now, we can go a step further and get 'import bzrlib.plugins.pluginname'
> > working.
> > 
> > All this takes is (in bzrlib/__init__.py):
> > import bzrlib.plugins
> > bzrlib.plugins.__path__ = [per_user_path, system_path]
> 
> Wouldn't this be better inside 'bzrlib/plugins/__init__.py:'
> 
> #hand wave a bit, we need to be more complete here
> # especially since BZR_PLUGIN_PATH can have multiple entries
> user_path = os.environ.get('BZR_PLUGIN_PATH', '~/.bazaar/plugins')
> __path__.insert(0, user_path)

This will need an expanduser call on it too :)

> I do believe the 'system' path is the natural path.

The default value of __path__ there is indeed that. So yes, we can put
it in the module itself. I wasn't sure when __path__ was set - PEP302
wasn't entirely clear. Its probably safe enough to put that in
plugins/__init__.py though.

> > 
> > (This is supported and intentional according to PEP 302). This would
> > make our 'load all plugins' code be:
> > 
> >>>> import imp
> >>>> # handwave - do a directory listing of both system plugins and user
> >>>> # plugins and strip back the paths to the first '.'. Then:
> >>>> for name in candidate_set:
> >>>>     try:
> >>>>         mod_info = imp.find_module(name, [user_plugin_dir,
> > system_plugin_dir])
> >>>>     except ImportError:
> >>>>         # no importable module by this name.
> >>>>     good_modules.append(name)
> >>>> for name in good_modules:
> >>>>    try:
> >>>>        __import__('bzrlib.plugins.' + name, globals(), locals(), [])
> >>>>    except ImportError, error:
> >>>>        mutter(... error)
> 
> So why do you need the 'find_module' in the first portion, if you are
> just going to load it based on path in the second section?

So that foo.py~ which is trimmed to foo, but will trigger ImportError
from find_module is ignored.

> It's up to you, but it seems that just having:
> 
> for name in candidate_set:
>   try:
>     __import__('bzrlib.plugins.'+name, ...)
>   except ImportError, error:
>     mutter()
> 
> I suppose we might only want to mutter if it was a real module that we
> were trying to load. And if that is your thought, it seems good. (I
> would include that if we are doing that, it can be helpful to mutter()
> the traceback)

There are two forms of failure: Failure in names and failure in content.
The find_module failures are failures in name only, and thus I dont
think worth logging. (Perhaps we could detail the activity with a -v
option to 'bzr plugins' for debugging).

> > 
> > This should be done for 0.10 IMO. It has a number of ramifications:
> >  * the disk name for plugins should be valid python identifiers. I think
> > this is a good thing.
> 
> ^- only downside is that a lot of plugin names will have to change. And
> it means you have to be very explicit about the naming structure. So
> that someone can only do:
> bzr get http://my/plugin/foo
> 
> If you don't have:
> plugin/dev
> plugin/mainline
> plugin/bzr-0.8
> 
> I really think it might be better to load the plugin, and look for
> something in the main file like OFFICIAL_PLUGIN_NAME='foo', and then set
> bzrlib.plugins.foo = imported_plugin
> 
> Then it doesn't matter what the path on disk is.

This is much more complex to maintain and leads to problems where you
have the same plugin twice in your plugin dir. I'm really against this
complexity for a number of reasons:
 - stock python does not have this. Because of this its not predictable
to people that our plugins will do it. It stops them being 'just python'
and makes them something special.
 - We'll need to write much more code if we want to support direct
import of the plugins - we'll need a sys_meta hook, and thats about 5-10
times as complex conceptually.
 - We'll have more work to do to figure out what any plugin is meant to
be.
 - Its not directly understandable by a user - even after documenting
this I expect we'll see problems where people think that renaming things
on disk is enough to disambiguate rather than moving them out of the
search path because the internal name is different.

> >  * module load order will become nondeterministic unless we explicitly
> > sort the set. I think this is a good thing too - for overloading
> > commands we need a better mechanism than renaming modules - because
> > renaming breaks code sharing between plugins.
> 
> Actually, whatever system we use, the plugin could do:
> 
> try:
>   # Make sure this is loaded first if present
>   # because we hijack it.
>   import bzrlib.plugins.other_plugin
> except ImportError, e:
>   pass
> 
> So you can force another module to be loaded before you, but I don't see
> any way to force yourself to get loaded first.

Sure, importing the other is a quick-hack to be loaded later - but in
fact what I'm saying is that for some things load order should be
decoupled from override order. For instance, to let a user choose which
copy of 'shelve' they want - the bzrtools one or the 'official' one.

> >  * we wont support 'foomodule.so' style plugins - but as its our plugin
> > system I don't think that is a big loss.
> 
> It isn't very hard to strip both back to the '.' and strip off 'module.'

At a cost though, see above :)

> > 
> > and the big one:
> >  * plugins can now share code between each other, so you can have a
> > plugin (for instance) 'bzrlib.plugins.graphcore' which the plugins
> > 'bzrk', 'olive' both import functionality from.
> 
> Yeah. I do think we need some more sharing.
> 
> We have the same issue with 'pqm-submit' and your 'email' plugin (send
> an email after a commit). They really should share the common email
> infrastructure.
> 
> > 
> > In terms of user experience, dropping a plugin to a specific name on
> > disk should not be hard at all. We can add a runtime check to see its
> > been done right - just check an attribute in the plugin for its expected
> > name. Where plugins do share code the user will need to install both
> > (naturally), but for a distribution shipping plugins into the system
> > path this should make things easier as well - they can package each bit
> > separately.
> > 
> > Rob
> 
> I think we want the attribute in the plugin. So as you say, we can at
> least make sure it is named correctly. Because of the 'plugin A can load
> plugin B before B is even loaded yet', I'm willing to go with your
> suggestion. (I think plugin A could normally just delay loading plugin
> B, but it isn't critical).
> 
> If we have the attribute, it could be used to override the name...

Only by doing a full hook, which is really not worth it IMO. I'd use the
attribute as an optional hint - if its there, great, if its not, no
problem.

So sounds like this is agreed in the large shape, I'll make a spec for
this today.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060802/44594bd5/attachment.pgp