Bazaar

Merge lp:~jml/bzr/lp-login-oauth into lp:bzr

  1. lp-login-oauth
  2. Merge into bzr.dev
Proposed by Jonathan Lange
Status: Rejected
Rejected by: Jonathan Lange
Proposed branch: lp:~jml/bzr/lp-login-oauth
Merge into: lp:bzr
Diff against target: 299 lines (+184/-22)
4 files modified
bzrlib/plugins/launchpad/__init__.py (+24/-1)
bzrlib/plugins/launchpad/lp_api.py (+134/-0)
bzrlib/plugins/launchpad/lp_directory.py (+2/-8)
bzrlib/plugins/launchpad/lp_registration.py (+24/-13)
To merge this branch: bzr merge lp:~jml/bzr/lp-login-oauth
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Andrew Bennetts Needs Fixing
Review via email: mp+15853@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

Rough branch for adding launchpadlib support to Bazaar.

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

2009/12/9 Jonathan Lange <email address hidden>:
> Jonathan Lange has proposed merging lp:~jml/bzr/lp-login-oauth into lp:bzr.
>
>    Requested reviews:
>    bzr-core (bzr-core)
>
>
> Rough branch for adding launchpadlib support to Bazaar.

Thanks.

This is going to make us page in all the launchpadlib dependencies
every time bzr starts, which will probably slow startup even when it's
not needed.

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

I agree with Martin's concern about unnecessary imports.

jml and I just discussed this on IRC, and I suggested that the command should always exist, and that the dependency should be lazily loaded (via lazy_import or a regular function-local import, it doesn't matter). If the user tries the command and the dependency is missing, then it should fail with a reasonably friendly error telling the user about the missing dependency.

jml said on IRC: "I can imagine it being a little confusing to have lots of available commands that you can't actually use," but a) this concern seems a bit premature, and b) I think no more confusing than commands being missing for obscure reasons even though their plugin is installed.

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

Changed, following spiv's suggestions.

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

So... this doesn't address the fact than bzr startup time is probably significantly impacted by loading all of launchpad lib, even when not running lp commands. Why not refactor it the way Qbzr did it with:

class LaunchpadLibCommand(commands.Command):

  def run(self, *args, **kwargs):
    try:
      import launchpadlib
    except ImportError:
      trace.warning('The python module launchpadlib is not available'
                    ' and it is needed to run %s. Please install it and'
                    ' try again.' % (self.__class__.__name__,)) # not quite right
      return 3 # exceptions are error 3, IIRC
    return self._run_lp(*args, **kwargs)

You could even do that as

  try:
    ... import lp_api
  except ImportError

Then things seem to stay relatively isolated, and we don't slow down importing all of zope, and json parsers when running "bzr status".

I do understand the desire to not register a command that cannot be run (and we can't detect that without trying the import). But I think it is better to show the command, and have in the doc string "needs launchpadlib", and then have a clear error and how the user can resolve it at runtime.

Rather than adding overhead to all commands.

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

Thanks for the comments John. I think they were made on an out-of-date diff, since the one I'm looking at doesn't incorporate my most recent changes.

While I slept...

<spiv> jml: are you sure you pushed? I don't see any new revisions
<spiv> jml: oh, it's stacked on the old, non-rich-root, bzr trunk, so boom: https://code.edge.launchpad.net/~jml/bzr/lp-login-oauth
<spiv> jml: I think bzr and launchpad can take equal credit for that snafu ;)

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

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

Jonathan Lange wrote:
> Thanks for the comments John. I think they were made on an out-of-date diff, since the one I'm looking at doesn't incorporate my most recent changes.
>
> While I slept...
>
> <spiv> jml: are you sure you pushed? I don't see any new revisions
> <spiv> jml: oh, it's stacked on the old, non-rich-root, bzr trunk, so boom: https://code.edge.launchpad.net/~jml/bzr/lp-login-oauth
> <spiv> jml: I think bzr and launchpad can take equal credit for that snafu ;)

Most likely. Do you have something updated then? As the diff I still see
is out of date.

John
=:->

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

iEYEARECAAYFAksgDVoACgkQJdeBCYSNAAO2dwCeJjkTlKv39Iy4IfMmhBJpeqgH
17QAnils3Rj/jFIodZlWV2A+DmSdSit9
=1IGb
-----END PGP SIGNATURE-----

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

On Thu, Dec 10, 2009 at 7:49 AM, John Arbash Meinel
<email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Jonathan Lange wrote:
>> Thanks for the comments John. I think they were made on an out-of-date diff, since the one I'm looking at doesn't incorporate my most recent changes.
>>
>> While I slept...
>>
>> <spiv> jml: are you sure you pushed?  I don't see any new revisions
>> <spiv> jml: oh, it's stacked on the old, non-rich-root, bzr trunk, so boom: https://code.edge.launchpad.net/~jml/bzr/lp-login-oauth
>> <spiv> jml: I think bzr and launchpad can take equal credit for that snafu ;)
>
> Most likely. Do you have something updated then? As the diff I still see
> is out of date.
>

Given that you are around & willing, I'll make something right now!

jml

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

Please see https://code.edge.launchpad.net/~jml/bzr/lp-login-oauth-2/+merge/15897 instead.

Updating diff...

An updated diff will be available in a few minutes. Reload to see the changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/plugins/launchpad/__init__.py'
2--- bzrlib/plugins/launchpad/__init__.py 2009-12-07 15:47:05 +0000
3+++ bzrlib/plugins/launchpad/__init__.py 2009-12-09 07:20:40 +0000
4@@ -46,6 +46,11 @@
5 NotLaunchpadBranch,
6 )
7
8+try:
9+ from bzrlib.plugins.launchpad import lp_api
10+except ImportError:
11+ lp_api = None
12+
13
14 class cmd_register_branch(Command):
15 """Register a branch with launchpad.net.
16@@ -111,7 +116,7 @@
17 link_bug=None,
18 dry_run=False):
19 from bzrlib.plugins.launchpad.lp_registration import (
20- LaunchpadService, BranchRegistrationRequest, BranchBugLinkRequest,
21+ BranchRegistrationRequest, BranchBugLinkRequest,
22 DryRunLaunchpadService)
23 if public_url is None:
24 try:
25@@ -255,6 +260,24 @@
26 register_command(cmd_launchpad_login)
27
28
29+class cmd_launchpad_mirror(Command):
30+ """Ask Launchpad to mirror a branch now."""
31+
32+ aliases = ['lp-mirror']
33+ takes_args = ['location?']
34+
35+ def run(self, location='.'):
36+ service = LaunchpadService()
37+ launchpad = lp_api.login(service)
38+ branch = _mod_branch.Branch.open(location)
39+ lp_branch = lp_api.load_branch(launchpad, branch)
40+ lp_branch.requestMirror()
41+
42+
43+if lp_api is not None:
44+ register_command(cmd_launchpad_mirror)
45+
46+
47 def _register_directory():
48 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',
49 'LaunchpadDirectory',
50
51=== added file 'bzrlib/plugins/launchpad/lp_api.py'
52--- bzrlib/plugins/launchpad/lp_api.py 1970-01-01 00:00:00 +0000
53+++ bzrlib/plugins/launchpad/lp_api.py 2009-12-09 07:20:40 +0000
54@@ -0,0 +1,134 @@
55+# Copyright (C) 2009 Canonical Ltd
56+#
57+# This program is free software; you can redistribute it and/or modify
58+# it under the terms of the GNU General Public License as published by
59+# the Free Software Foundation; either version 2 of the License, or
60+# (at your option) any later version.
61+#
62+# This program is distributed in the hope that it will be useful,
63+# but WITHOUT ANY WARRANTY; without even the implied warranty of
64+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
65+# GNU General Public License for more details.
66+#
67+# You should have received a copy of the GNU General Public License
68+# along with this program; if not, write to the Free Software
69+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
70+
71+"""Tools for dealing with the Launchpad API."""
72+
73+import os
74+
75+from bzrlib import (
76+ errors,
77+ trace,
78+ )
79+from bzrlib.plugins.launchpad.lp_registration import (
80+ InvalidLaunchpadInstance,
81+ NotLaunchpadBranch,
82+ )
83+
84+from launchpadlib.credentials import Credentials
85+from launchpadlib.launchpad import (
86+ EDGE_SERVICE_ROOT,
87+ STAGING_SERVICE_ROOT,
88+ Launchpad,
89+ )
90+from lazr.uri import URI
91+
92+
93+CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
94+
95+
96+LAUNCHPAD_API_URLS = {
97+ 'production': EDGE_SERVICE_ROOT,
98+ 'edge': EDGE_SERVICE_ROOT,
99+ 'staging': STAGING_SERVICE_ROOT,
100+ 'dev': 'https://api.launchpad.dev/beta/',
101+ }
102+
103+
104+def _get_api_url(service):
105+ """Return the root URL of the Launchpad API.
106+
107+ e.g. For the 'edge' Launchpad service, this function returns
108+ launchpadlib.launchpad.EDGE_SERVICE_ROOT.
109+
110+ :param service: A `LaunchpadService` object.
111+ :return: A URL as a string.
112+ """
113+ if service._lp_instance is None:
114+ lp_instance = service.DEFAULT_INSTANCE
115+ else:
116+ lp_instance = service._lp_instance
117+ try:
118+ return LAUNCHPAD_API_URLS[lp_instance]
119+ except KeyError:
120+ raise InvalidLaunchpadInstance(lp_instance)
121+
122+
123+def _get_credential_path(service):
124+ """Return the path to cached credentials for 'service'.
125+
126+ :param service: A `LaunchpadService` object.
127+ :return: The path to a cached credentials file, which might not exist.
128+ """
129+ web_root_uri = URI(_get_api_url(service))
130+ credential_name = 'creds-%s-bzr' % (web_root_uri.host)
131+ return os.path.join(CACHE_DIRECTORY, credential_name)
132+
133+
134+def _login_from_cache(consumer_name, service_root, cache_dir,
135+ credential_cache, timeout=None, proxy_info=None):
136+ """Use cached credentials if they exist, log in otherwise."""
137+ try:
138+ credentials = Credentials.load_from_path(credential_cache)
139+ except (OSError, IOError):
140+ launchpad = Launchpad.get_token_and_login(
141+ consumer_name, service_root, cache_dir, timeout, proxy_info)
142+ launchpad.credentials.save_to_path(credential_cache)
143+ else:
144+ access_key = credentials.access_token.key
145+ access_secret = credentials.access_token.secret
146+ launchpad = Launchpad.login(
147+ consumer_name, access_key, access_secret, service_root,
148+ cache_dir, timeout, proxy_info)
149+ return launchpad
150+
151+
152+def login(service, timeout=None, proxy_info=None):
153+ """Log in to the Launchpad API.
154+
155+ :return: The root `Launchpad` object from launchpadlib.
156+ """
157+ credential_path = _get_credential_path(service)
158+ launchpad = _login_from_cache(
159+ 'bzr', _get_api_url(service), CACHE_DIRECTORY, credential_path,
160+ timeout, proxy_info)
161+ launchpad._service = service
162+ return launchpad
163+
164+
165+def load_branch(launchpad, branch):
166+ """Return the launchpadlib Branch object corresponding to 'branch'.
167+
168+ :param launchpad: The root `Launchpad` object from launchpadlib.
169+ :param branch: A `bzrlib.branch.Branch`.
170+ :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
171+ `branch`.
172+ :return: A launchpadlib Branch object.
173+ """
174+ service = launchpad._service
175+ for url in branch.get_public_branch(), branch.get_push_location():
176+ if url is None:
177+ continue
178+ try:
179+ path = service._guess_branch_path(url)
180+ except (errors.InvalidURL, NotLaunchpadBranch):
181+ pass
182+ else:
183+ trace.mutter('Guessing path: %s', path)
184+ uri = launchpad._root_uri.append(path)
185+ uri_str = str(uri)
186+ trace.mutter('Guessing url: %s', uri_str)
187+ return launchpad.load(uri_str)
188+ raise NotLaunchpadBranch(url)
189
190=== modified file 'bzrlib/plugins/launchpad/lp_directory.py'
191--- bzrlib/plugins/launchpad/lp_directory.py 2009-03-23 14:59:43 +0000
192+++ bzrlib/plugins/launchpad/lp_directory.py 2009-12-09 07:20:40 +0000
193@@ -63,15 +63,9 @@
194 _request_factory=ResolveLaunchpadPathRequest,
195 _lp_login=None):
196 """Resolve the base URL for this transport."""
197+ service = LaunchpadService.for_url(url)
198 result = urlsplit(url)
199- # Perform an XMLRPC request to resolve the path
200- lp_instance = result[1]
201- if lp_instance == '':
202- lp_instance = None
203- elif lp_instance not in LaunchpadService.LAUNCHPAD_INSTANCE:
204- raise errors.InvalidURL(path=url)
205 resolve = _request_factory(result[2].strip('/'))
206- service = LaunchpadService(lp_instance=lp_instance)
207 try:
208 result = resolve.submit(service)
209 except xmlrpclib.Fault, fault:
210@@ -79,7 +73,7 @@
211 path=url, extra=fault.faultString)
212
213 if 'launchpad' in debug.debug_flags:
214- trace.mutter("resolve_lp_path(%r) == %r", path, result)
215+ trace.mutter("resolve_lp_path(%r) == %r", url, result)
216
217 if _lp_login is None:
218 _lp_login = get_lp_login()
219
220=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
221--- bzrlib/plugins/launchpad/lp_registration.py 2009-11-05 18:32:31 +0000
222+++ bzrlib/plugins/launchpad/lp_registration.py 2009-12-09 07:20:40 +0000
223@@ -21,18 +21,15 @@
224 import urllib
225 import xmlrpclib
226
227-from bzrlib.lazy_import import lazy_import
228-lazy_import(globals(), """
229-from bzrlib import urlutils
230-""")
231-
232 from bzrlib import (
233 config,
234 errors,
235+ urlutils,
236 __version__ as _bzrlib_version,
237 )
238 from bzrlib.transport.http import _urllib2_wrappers
239
240+
241 # for testing, do
242 '''
243 export BZR_LP_XMLRPC_URL=http://xmlrpc.staging.launchpad.net/bazaar/
244@@ -123,7 +120,6 @@
245 % (_bzrlib_version, xmlrpclib.__version__)
246 self.transport = transport
247
248-
249 @property
250 def service_url(self):
251 """Return the http or https url for the xmlrpc server.
252@@ -141,6 +137,17 @@
253 else:
254 return self.DEFAULT_SERVICE_URL
255
256+ @classmethod
257+ def for_url(cls, url, **kwargs):
258+ """Return the Launchpad service corresponding to the given URL."""
259+ result = urlsplit(url)
260+ lp_instance = result[1]
261+ if lp_instance == '':
262+ lp_instance = None
263+ elif lp_instance not in cls.LAUNCHPAD_INSTANCE:
264+ raise errors.InvalidURL(path=url)
265+ return cls(lp_instance=lp_instance, **kwargs)
266+
267 def get_proxy(self, authenticated):
268 """Return the proxy for XMLRPC requests."""
269 if authenticated:
270@@ -216,13 +223,7 @@
271 instance = self._lp_instance
272 return self.LAUNCHPAD_DOMAINS[instance]
273
274- def get_web_url_from_branch_url(self, branch_url, _request_factory=None):
275- """Get the Launchpad web URL for the given branch URL.
276-
277- :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
278- Launchpad branch URL.
279- :return: The URL of the branch on Launchpad.
280- """
281+ def _guess_branch_path(self, branch_url, _request_factory=None):
282 scheme, hostinfo, path = urlsplit(branch_url)[:3]
283 if _request_factory is None:
284 _request_factory = ResolveLaunchpadPathRequest
285@@ -240,6 +241,16 @@
286 for domain in self.LAUNCHPAD_DOMAINS.itervalues())
287 if hostinfo not in domains:
288 raise NotLaunchpadBranch(branch_url)
289+ return path.lstrip('/')
290+
291+ def get_web_url_from_branch_url(self, branch_url, _request_factory=None):
292+ """Get the Launchpad web URL for the given branch URL.
293+
294+ :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
295+ Launchpad branch URL.
296+ :return: The URL of the branch on Launchpad.
297+ """
298+ path = self._guess_branch_path(branch_url, _request_factory)
299 return urlutils.join('https://code.%s' % self.domain, path)
300
301