Bazaar

Merge lp:~mbp/bzr/lazy-commands into lp:bzr

  1. lazy-commands
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/lazy-commands
Merge into: lp:bzr
Diff against target: 244 lines (+72/-28)
4 files modified
bzrlib/builtins.py (+0/-8)
bzrlib/commands.py (+60/-16)
bzrlib/tests/test_commands.py (+3/-4)
bzrlib/tests/test_import_tariff.py (+9/-0)
To merge this branch: bzr merge lp:~mbp/bzr/lazy-commands
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+21910@code.launchpad.net

Commit message

(mbp) allow builtin commands to be lazy loaded; make bundle-info lazy

Description of the change

This allows builtin commands to be lazy-loaded and makes bundle-info so.

CommandRegistry needs to handle aliases itself, rather than constructing a
command instance to look them up, because doing so implies loading the
implementation.

_builtin_commands (previously private) is deprecated.

More can be done but I thought I'd put this up for review.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/lazy-commands into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This allows builtin commands to be lazy-loaded and makes bundle-info so.
>
> CommandRegistry needs to handle aliases itself, rather than constructing a
> command instance to look them up, because doing so implies loading the
> implementation.
>
> _builtin_commands (previously private) is deprecated.
>
> More can be done but I thought I'd put this up for review.
>

This looks good to me. It would be nice to have explicit tests on
aliases/lazily imported commands/overriding lazy commands. However, at
least forcing one command to be lazy is good. It would probably be
better to make it a commonly used command (bzr st), so that devs would
get active use out of it. That can be the next step, though.

I wonder if explicitly registering is better than trying to 'import from
XXX' if you want command XXX. Certainly plugins still need to register,
and registering for stuff that they override is tricker in those
circumstances.

Anyway, this is going to be better than what we have. I just wonder if
it is going to get us the best result.

John
=:->

 merge: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuo62sACgkQJdeBCYSNAAO66gCfWgc8rOYyNWoQfBrt/iDWWxIR
AQcAoKagEZjZnP7l7WO7y14qrztGsoER
=sw27
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

On 24 March 2010 03:27, John A Meinel <email address hidden> wrote:
> This looks good to me. It would be nice to have explicit tests on
> aliases/lazily imported commands/overriding lazy commands. However, at
> least forcing one command to be lazy is good.

I was counting on there already being tests for that, but when I come
back to this I will review the tests and see if any need to be added
or cleaned up.

> It would probably be
> better to make it a commonly used command (bzr st), so that devs would
> get active use out of it. That can be the next step, though.

mm, I just picked this one because it was already in a selfcontained
module. Eventually we can either get rid of implicit registration of
everything in builtins.py, and/or split them into smaller files.

>
> I wonder if explicitly registering is better than trying to 'import from
> XXX' if you want command XXX. Certainly plugins still need to register,
> and registering for stuff that they override is tricker in those
> circumstances.

I don't understand.

> Anyway, this is going to be better than what we have. I just wonder if
> it is going to get us the best result.

It's definitely not the whole story - but do you think anything here
is actually in the wrong direction?

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/lazy-commands http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev

Revision history for this message
Martin Pool (mbp) wrote :

fails in

ERROR: bzrlib.tests.test_commands.TestExtendCommandHook.test_fires_on_get_cmd_object
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
 File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
   return fn(*args)
 File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
   testMethod()
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_commands.py", line 224, in test_fires_on_get_cmd_object
   cmd = commands.get_cmd_object('test-extend-command-hook')
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/commands.py", line 245, in get_cmd_object
   raise errors.BzrCommandError('unknown command "%s"' % cmd_name)
BzrCommandError: unknown command "test-extend-command-hook"

Revision history for this message
Jonathan Lange (jml) wrote :

On Wed, Mar 24, 2010 at 5:12 AM, Martin Pool <email address hidden> wrote:
> Fed to pqm
>
> star-merge http://bazaar.launchpad.net/~mbp/bzr/lazy-commands http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev
>

Why am I now getting these emails and how do I make it stop?

jml

Revision history for this message
Martin Pool (mbp) wrote :

On 24 March 2010 22:09, Jonathan Lange <email address hidden> wrote:
> On Wed, Mar 24, 2010 at 5:12 AM, Martin Pool <email address hidden> wrote:
>> Fed to pqm
>>
>> star-merge http://bazaar.launchpad.net/~mbp/bzr/lazy-commands http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev
>>
>
> Why am I now getting these emails and how do I make it stop?

Jono, you asked the same question a week ago and I answered it.
Please read the reply, or at least read
https://bugs.edge.launchpad.net/hydrazine/+bug/541586

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/lazy-commands http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jonathan Lange wrote:
> On Wed, Mar 24, 2010 at 5:12 AM, Martin Pool <email address hidden> wrote:
>> Fed to pqm
>>
>> star-merge http://bazaar.launchpad.net/~mbp/bzr/lazy-commands http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev
>>
>
> Why am I now getting these emails and how do I make it stop?
>
> jml

Martin's 'feed-pqm' script as part of lp:hydrazine, I believe. I don't
think there is any way to 'make it stop' since it is just another
comment on the merge proposal. (Along with the side effect of modifying
the 'commit message'.) Certainly I don't find it ideal. As a single
merge request, plus a single review approve, then generates 3 more emails

 1) Request review
 2) review: approve
  2b) If you use email, you can 'merge:approve' in one email, but using
      the web api, it generates a second mail.
 3) Commit comment updated
 4) Fed to pqm
 5) Merged

Basically, only 2 out of 5/6 emails have any real content. Which is a
rather poor signal-to-noise ratio.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkurlYkACgkQJdeBCYSNAAP/5QCdEQSafASDqtVA/Zq8piYCJpvu
/uQAn1qO5x6+JOWAkwtJ/Ysdawsfqfoq
=Am65
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

On 26 March 2010 03:55, John Arbash Meinel <email address hidden> wrote:
>> Why am I now getting these emails and how do I make it stop?
>>
>> jml
>
> Martin's 'feed-pqm' script as part of lp:hydrazine, I believe. I don't
> think there is any way to 'make it stop' since it is just another
> comment on the merge proposal. (Along with the side effect of modifying
> the 'commit message'.) Certainly I don't find it ideal. As a single
> merge request, plus a single review approve, then generates 3 more emails

I don't know if jml's objection was to getting the mails at all, or to
the fact that, until bug 541586 was fixed, they showed up in separate
gmail threads.

Anyhow, we can trivially make feed-pqm stop posting comments to the
bug. I think it would be a bit useful to let others see that the
merge was at least attempted, but perhaps it is not worth the noise.
Ideally this would be tracked using structured data on the mp, but
that needs more implementation on the Launchpad side. (But it might
be sufficiently exposed over the api to work.)

--
Martin <http://launchpad.net/~mbp/>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2010-03-22 13:59:33 +0000
3+++ bzrlib/builtins.py 2010-03-25 06:05:45 +0000
4@@ -5974,15 +5974,7 @@
5 self.outf.write('%s %s\n' % (path, location))
6
7
8-# these get imported and then picked up by the scan for cmd_*
9-# TODO: Some more consistent way to split command definitions across files;
10-# we do need to load at least some information about them to know of
11-# aliases. ideally we would avoid loading the implementation until the
12-# details were needed.
13 from bzrlib.cmd_version_info import cmd_version_info
14 from bzrlib.conflicts import cmd_resolve, cmd_conflicts, restore
15-from bzrlib.bundle.commands import (
16- cmd_bundle_info,
17- )
18 from bzrlib.foreign import cmd_dpush
19 from bzrlib.sign_my_commits import cmd_sign_my_commits
20
21=== modified file 'bzrlib/commands.py'
22--- bzrlib/commands.py 2010-03-01 09:27:39 +0000
23+++ bzrlib/commands.py 2010-03-25 06:05:45 +0000
24@@ -15,9 +15,6 @@
25 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
26
27
28-# TODO: probably should say which arguments are candidates for glob
29-# expansion on windows and do that at the command level.
30-
31 # TODO: Define arguments by objects, rather than just using names.
32 # Those objects can specify the expected type of the argument, which
33 # would help with validation and shell completion. They could also provide
34@@ -25,9 +22,6 @@
35
36 # TODO: Specific "examples" property on commands for consistent formatting.
37
38-# TODO: "--profile=cum", to change sort order. Is there any value in leaving
39-# the profile output behind so it can be interactively examined?
40-
41 import os
42 import sys
43
44@@ -78,6 +72,22 @@
45
46
47 class CommandRegistry(registry.Registry):
48+ """Special registry mapping command names to command classes.
49+
50+ :ivar overridden_registry: Look in this registry for commands being
51+ overridden by this registry. This can be used to tell plugin commands
52+ about the builtin they're decorating.
53+ """
54+
55+ def __init__(self):
56+ registry.Registry.__init__(self)
57+ self.overridden_registry = None
58+ # map from aliases to the real command that implements the name
59+ self._alias_dict = {}
60+
61+ def get(self, command_name):
62+ real_name = self._alias_dict.get(command_name, command_name)
63+ return registry.Registry.get(self, real_name)
64
65 @staticmethod
66 def _get_name(command_name):
67@@ -99,7 +109,12 @@
68 try:
69 previous = self.get(k_unsquished)
70 except KeyError:
71- previous = _builtin_commands().get(k_unsquished)
72+ previous = None
73+ if self.overridden_registry:
74+ try:
75+ previous = self.overridden_registry.get(k_unsquished)
76+ except KeyError:
77+ pass
78 info = CommandInfo.from_command(cmd)
79 try:
80 registry.Registry.register(self, k_unsquished, cmd,
81@@ -110,6 +125,8 @@
82 sys.modules[cmd.__module__])
83 trace.warning('Previously this command was registered from %r' %
84 sys.modules[previous.__module__])
85+ for a in cmd.aliases:
86+ self._alias_dict[a] = k_unsquished
87 return previous
88
89 def register_lazy(self, command_name, aliases, module_name):
90@@ -122,12 +139,20 @@
91 key = self._get_name(command_name)
92 registry.Registry.register_lazy(self, key, module_name, command_name,
93 info=CommandInfo(aliases))
94+ for a in aliases:
95+ self._alias_dict[a] = key
96
97
98 plugin_cmds = CommandRegistry()
99+builtin_command_registry = CommandRegistry()
100+plugin_cmds.overridden_registry = builtin_command_registry
101
102
103 def register_command(cmd, decorate=False):
104+ """Register a plugin command.
105+
106+ Should generally be avoided in favor of lazy registration.
107+ """
108 global plugin_cmds
109 return plugin_cmds.register(cmd, decorate)
110
111@@ -140,9 +165,27 @@
112 return cmd[4:].replace('_','-')
113
114
115+@deprecated_function(deprecated_in((2, 2, 0)))
116 def _builtin_commands():
117+ """Return a dict of {name: cmd_class} for builtin commands.
118+
119+ :deprecated: Use the builtin_command_registry registry instead
120+ """
121+ # return dict(name: cmd_class)
122+ return dict(builtin_command_registry.items())
123+
124+
125+def _register_builtin_commands():
126+ if builtin_command_registry.keys():
127+ # only load once
128+ return
129 import bzrlib.builtins
130- return _scan_module_for_commands(bzrlib.builtins)
131+ for cmd_class in _scan_module_for_commands(bzrlib.builtins).values():
132+ builtin_command_registry.register(cmd_class)
133+ # lazy builtins
134+ builtin_command_registry.register_lazy('cmd_bundle_info',
135+ [],
136+ 'bzrlib.bundle.commands')
137
138
139 def _scan_module_for_commands(module):
140@@ -155,7 +198,10 @@
141
142
143 def _list_bzr_commands(names):
144- """Find commands from bzr's core and plugins."""
145+ """Find commands from bzr's core and plugins.
146+
147+ This is not the public interface, just the default hook called by all_command_names.
148+ """
149 # to eliminate duplicates
150 names.update(builtin_command_names())
151 names.update(plugin_command_names())
152@@ -179,7 +225,7 @@
153 Use of all_command_names() is encouraged rather than builtin_command_names
154 and/or plugin_command_names.
155 """
156- return _builtin_commands().keys()
157+ return builtin_command_registry.keys()
158
159
160 def plugin_command_names():
161@@ -263,15 +309,12 @@
162
163 def _get_bzr_command(cmd_or_None, cmd_name):
164 """Get a command from bzr's core."""
165- cmds = _builtin_commands()
166 try:
167- return cmds[cmd_name]()
168+ cmd_class = builtin_command_registry.get(cmd_name)
169 except KeyError:
170 pass
171- # look for any command which claims this as an alias
172- for real_cmd_name, cmd_class in cmds.iteritems():
173- if cmd_name in cmd_class.aliases:
174- return cmd_class()
175+ else:
176+ return cmd_class()
177 return cmd_or_None
178
179
180@@ -1116,6 +1159,7 @@
181 :return: exit code of bzr command.
182 """
183 argv = _specified_or_unicode_argv(argv)
184+ _register_builtin_commands()
185 ret = run_bzr_catch_errors(argv)
186 bzrlib.ui.ui_factory.log_transport_activity(
187 display=('bytes' in debug.debug_flags))
188
189=== modified file 'bzrlib/tests/test_commands.py'
190--- bzrlib/tests/test_commands.py 2010-03-01 09:27:39 +0000
191+++ bzrlib/tests/test_commands.py 2010-03-25 06:05:45 +0000
192@@ -211,14 +211,13 @@
193 commands.Command.hooks.install_named_hook(
194 "extend_command", hook_calls.append, None)
195 # create a command, should not fire
196- class ACommand(commands.Command):
197+ class cmd_test_extend_command_hook(commands.Command):
198 """A sample command."""
199- cmd = ACommand()
200 self.assertEqual([], hook_calls)
201 # -- as a builtin
202 # register the command class, should not fire
203 try:
204- builtins.cmd_test_extend_command_hook = ACommand
205+ commands.builtin_command_registry.register(cmd_test_extend_command_hook)
206 self.assertEqual([], hook_calls)
207 # and ask for the object, should fire
208 cmd = commands.get_cmd_object('test-extend-command-hook')
209@@ -228,7 +227,7 @@
210 self.assertSubset([cmd], hook_calls)
211 del hook_calls[:]
212 finally:
213- del builtins.cmd_test_extend_command_hook
214+ commands.builtin_command_registry.remove('test-extend-command-hook')
215 # -- as a plugin lazy registration
216 try:
217 # register the command class, should not fire
218
219=== modified file 'bzrlib/tests/test_import_tariff.py'
220--- bzrlib/tests/test_import_tariff.py 2010-02-10 02:17:15 +0000
221+++ bzrlib/tests/test_import_tariff.py 2010-03-25 06:05:45 +0000
222@@ -36,6 +36,14 @@
223 """
224
225 def run_command_check_imports(self, args, forbidden_imports):
226+ """Run bzr ARGS in a subprocess and check its imports.
227+
228+ This is fairly expensive because we start a subprocess, so we aim to
229+ cover representative rather than exhaustive cases.
230+
231+ :param forbidden_imports: List of fully-qualified Python module names
232+ that should not be loaded while running this command.
233+ """
234 # We use PYTHON_VERBOSE rather than --profile-importts because in
235 # experimentation the profile-imports output seems to not always show
236 # the modules you'd expect; this can be debugged but python -v seems
237@@ -82,6 +90,7 @@
238 # 'st' in a working tree shouldn't need many modules
239 self.make_branch_and_tree('.')
240 self.run_command_check_imports(['st'], [
241+ 'bzrlib.bundle.commands',
242 'bzrlib.remote',
243 'bzrlib.smart',
244 'smtplib',