[MERGE] make plugins use the right suffixes

[John Arbash Meinel]

Robert Collins wrote:
> On Mon, 2006-07-31 at 18:54 -0500, John Arbash Meinel wrote:
>> John Arbash Meinel wrote:
>>> Robert noticed that we would allow loading '.pyo' files even if we
>>> weren't running python -O. (it turns out python won't load .pyc if
>>> running -O, and won't load .pyo if running without it.)
>>>
>>> The attached bundle fixes it to use the right suffixes.
>>>
>> ...
>>
>> Now with a real patch :)
>>
>> Available from:
>>
>> http://bzr.arbash-meinel.com/branches/bzr/features/plugin-imports
> 
> 
> Thanks. I'm going to merge this as is. I've looked at imp a bit more
> though and I think we can be a *lot* cleaner in 0.10:
> 
> For instance:
> 

v- It's a shame that the python 'prefix' ">>>" is the same as emails
"This is a quote", it can be kind of hard to read these sometimes.

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

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

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

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

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)

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

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

>  * 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.'

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

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060801/9a0efd2f/attachment.pgp