Bazaar

Merge lp:~jelmer/bzr/plugin-refactor into lp:~bzr/bzr/trunk-old

  1. plugin-refactor
  2. Merge into trunk-old
Proposed by Jelmer Vernooij
Status: Rejected
Rejected by: Jelmer Vernooij
Proposed branch: lp:~jelmer/bzr/plugin-refactor
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 157 lines
To merge this branch: bzr merge lp:~jelmer/bzr/plugin-refactor
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Abstain
Robert Collins (community) Disapprove
Review via email: mp+9005@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

The attached patch refactors the plugin loading code a bit so we
remember the suffix tuple when finding plugins so we can use it for
loading them later. This saves a few stats, and will make it possible
to load individual plugins in the future (this is what I hope to work
on next).

The next step (and my next patch hopefully) will be supporting something like
"bzr --with-plugin /foo/bar/bla".

Revision history for this message
Robert Collins (lifeless) wrote :

On Sat, 2009-07-18 at 21:15 +0000, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/plugin-refactor into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> The attached patch refactors the plugin loading code a bit so we
> remember the suffix tuple when finding plugins so we can use it for
> loading them later. This saves a few stats, and will make it possible
> to load individual plugins in the future (this is what I hope to work
> on next).

I think this may be buggy/introduce bugs. You change the load logic
from:
calculate the plugin path
calculate names to load
import them all

to
calculate the plugin path
calculate names-in-directories-to-load
import-those-names

You also catch an ImportError and squelch it somewhere where it looks
like we'd want to know that the error occurred.

The patch is also going away from using 'import' to using
'imp.find_module' + 'imp.load_module' - we had bugs in the past
*because* we used those rather than using the main python import code
path.

This doesn't seem like a good conceptual idea to me. I don't want to
stop you achieving what you need to, but - why can't you just put the
plugin you want on the plugins path.

 review -1

review: Disapprove
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

I'll put this in a plugin instead.

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/plugin.py'
2--- bzrlib/plugin.py 2009-03-24 01:53:42 +0000
3+++ bzrlib/plugin.py 2009-07-19 02:36:01 +0000
4@@ -187,70 +187,103 @@
5 load_from_dirs = load_from_path
6
7
8+def load_by_suffix_info(name, path, suffix_info):
9+ """Load a plugin given its path and suffix info.
10+
11+ :param name: Short name of the plugin (e.g. search)
12+ :param path: Path to the module
13+ :param suffix_info: Suffix info tuple as used by imp.load_module
14+ :return: Module object for the plugin
15+ """
16+ if re.search('\.|-| ', name):
17+ sanitised_name = re.sub('[-. ]', '_', name)
18+ if sanitised_name.startswith('bzr_'):
19+ sanitised_name = sanitised_name[len('bzr_'):]
20+ trace.warning("Unable to load %r in %r as a plugin because the "
21+ "file path isn't a valid module name; try renaming "
22+ "it to %r.", name, os.path.dirname(path), sanitised_name)
23+ return
24+ try:
25+ if suffix_info[1] != '':
26+ # Directory
27+ file = open(path, suffix_info[1])
28+ else:
29+ file = None
30+ plugin = imp.load_module("bzrlib.plugins.%s" % name, file, path,
31+ suffix_info)
32+ setattr(_mod_plugins, name, plugin)
33+ return plugin
34+ except KeyboardInterrupt:
35+ raise
36+ except errors.IncompatibleAPI, e:
37+ trace.warning("Unable to load plugin %r. It requested API version "
38+ "%s of module %s but the minimum exported version is %s, and "
39+ "the maximum is %s" %
40+ (name, e.wanted, e.api, e.minimum, e.current))
41+ except Exception, e:
42+ trace.warning("%s" % e)
43+ ## import pdb; pdb.set_trace()
44+ trace.warning('Unable to load plugin %r from %r',
45+ name, os.path.dirname(path))
46+ trace.log_exception_quietly()
47+ if 'error' in debug.debug_flags:
48+ trace.print_exception(sys.exc_info(), sys.stderr)
49+
50+
51+def find_module(file, path):
52+ """Find the suffix information for a plugin.
53+
54+ :param file: Short name of the plugin
55+ :param path: Path to the plugin
56+ :return: Tuple with short name, path and suffix_info
57+ (as used by imp.load_module)
58+ """
59+ # Get the list of valid python suffixes for __init__.py?
60+ # this includes .py, .pyc, and .pyo (depending on if we are running -O)
61+ # but it doesn't include compiled modules (.so, .dll, etc)
62+ if os.path.isdir(path):
63+ for suffix, mod_type, flags in imp.get_suffixes():
64+ if flags not in (imp.PY_SOURCE, imp.PY_COMPILED):
65+ continue
66+ entry = '__init__'+suffix
67+ # This directory should be a package, and thus added to
68+ # the list
69+ entry_path = osutils.pathjoin(path, entry)
70+ if os.path.isfile(entry_path):
71+ return file, path, ('', '', imp.PKG_DIRECTORY)
72+ raise ImportError("plugin %s is not a package" % path)
73+ else:
74+ for suffix_info in imp.get_suffixes():
75+ if file.endswith(suffix_info[0]):
76+ file = file[:-len(suffix_info[0])]
77+ if suffix_info[2] == imp.C_EXTENSION and f.endswith('module'):
78+ file = file[:-len('module')]
79+ return file, path, suffix_info
80+ raise ImportError("plugin %s is not a module (no known Python suffix)" % path)
81+
82+
83 def load_from_dir(d):
84 """Load the plugins in directory d.
85
86 d must be in the plugins module path already.
87 """
88- # Get the list of valid python suffixes for __init__.py?
89- # this includes .py, .pyc, and .pyo (depending on if we are running -O)
90- # but it doesn't include compiled modules (.so, .dll, etc)
91- valid_suffixes = [suffix for suffix, mod_type, flags in imp.get_suffixes()
92- if flags in (imp.PY_SOURCE, imp.PY_COMPILED)]
93- package_entries = ['__init__'+suffix for suffix in valid_suffixes]
94- plugin_names = set()
95+ plugins = {}
96 for f in os.listdir(d):
97 path = osutils.pathjoin(d, f)
98- if os.path.isdir(path):
99- for entry in package_entries:
100- # This directory should be a package, and thus added to
101- # the list
102- if os.path.isfile(osutils.pathjoin(path, entry)):
103- break
104- else: # This directory is not a package
105- continue
106- else:
107- for suffix_info in imp.get_suffixes():
108- if f.endswith(suffix_info[0]):
109- f = f[:-len(suffix_info[0])]
110- if suffix_info[2] == imp.C_EXTENSION and f.endswith('module'):
111- f = f[:-len('module')]
112- break
113- else:
114- continue
115+ try:
116+ f, path, suffix_info = find_module(f, path)
117+ except ImportError:
118+ continue
119 if f == '__init__':
120 continue # We don't load __init__.py again in the plugin dir
121 elif getattr(_mod_plugins, f, None):
122 trace.mutter('Plugin name %s already loaded', f)
123 else:
124 # trace.mutter('add plugin name %s', f)
125- plugin_names.add(f)
126+ plugins[f] = (path, suffix_info)
127
128- for name in plugin_names:
129- try:
130- exec "import bzrlib.plugins.%s" % name in {}
131- except KeyboardInterrupt:
132- raise
133- except errors.IncompatibleAPI, e:
134- trace.warning("Unable to load plugin %r. It requested API version "
135- "%s of module %s but the minimum exported version is %s, and "
136- "the maximum is %s" %
137- (name, e.wanted, e.api, e.minimum, e.current))
138- except Exception, e:
139- trace.warning("%s" % e)
140- ## import pdb; pdb.set_trace()
141- if re.search('\.|-| ', name):
142- sanitised_name = re.sub('[-. ]', '_', name)
143- if sanitised_name.startswith('bzr_'):
144- sanitised_name = sanitised_name[len('bzr_'):]
145- trace.warning("Unable to load %r in %r as a plugin because the "
146- "file path isn't a valid module name; try renaming "
147- "it to %r." % (name, d, sanitised_name))
148- else:
149- trace.warning('Unable to load plugin %r from %r' % (name, d))
150- trace.log_exception_quietly()
151- if 'error' in debug.debug_flags:
152- trace.print_exception(sys.exc_info(), sys.stderr)
153+ for name, (path, suffix_info) in plugins.iteritems():
154+ load_by_suffix_info(name, path, suffix_info)
155
156
157 def plugins():