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
=== modified file 'bzrlib/plugins/launchpad/__init__.py'
--- bzrlib/plugins/launchpad/__init__.py 2009-12-07 15:47:05 +0000
+++ bzrlib/plugins/launchpad/__init__.py 2009-12-09 07:20:40 +0000
@@ -46,6 +46,11 @@
46 NotLaunchpadBranch,46 NotLaunchpadBranch,
47 )47 )
4848
49try:
50 from bzrlib.plugins.launchpad import lp_api
51except ImportError:
52 lp_api = None
53
4954
50class cmd_register_branch(Command):55class cmd_register_branch(Command):
51 """Register a branch with launchpad.net.56 """Register a branch with launchpad.net.
@@ -111,7 +116,7 @@
111 link_bug=None,116 link_bug=None,
112 dry_run=False):117 dry_run=False):
113 from bzrlib.plugins.launchpad.lp_registration import (118 from bzrlib.plugins.launchpad.lp_registration import (
114 LaunchpadService, BranchRegistrationRequest, BranchBugLinkRequest,119 BranchRegistrationRequest, BranchBugLinkRequest,
115 DryRunLaunchpadService)120 DryRunLaunchpadService)
116 if public_url is None:121 if public_url is None:
117 try:122 try:
@@ -255,6 +260,24 @@
255register_command(cmd_launchpad_login)260register_command(cmd_launchpad_login)
256261
257262
263class cmd_launchpad_mirror(Command):
264 """Ask Launchpad to mirror a branch now."""
265
266 aliases = ['lp-mirror']
267 takes_args = ['location?']
268
269 def run(self, location='.'):
270 service = LaunchpadService()
271 launchpad = lp_api.login(service)
272 branch = _mod_branch.Branch.open(location)
273 lp_branch = lp_api.load_branch(launchpad, branch)
274 lp_branch.requestMirror()
275
276
277if lp_api is not None:
278 register_command(cmd_launchpad_mirror)
279
280
258def _register_directory():281def _register_directory():
259 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',282 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',
260 'LaunchpadDirectory',283 'LaunchpadDirectory',
261284
=== added file 'bzrlib/plugins/launchpad/lp_api.py'
--- bzrlib/plugins/launchpad/lp_api.py 1970-01-01 00:00:00 +0000
+++ bzrlib/plugins/launchpad/lp_api.py 2009-12-09 07:20:40 +0000
@@ -0,0 +1,134 @@
1# Copyright (C) 2009 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17"""Tools for dealing with the Launchpad API."""
18
19import os
20
21from bzrlib import (
22 errors,
23 trace,
24 )
25from bzrlib.plugins.launchpad.lp_registration import (
26 InvalidLaunchpadInstance,
27 NotLaunchpadBranch,
28 )
29
30from launchpadlib.credentials import Credentials
31from launchpadlib.launchpad import (
32 EDGE_SERVICE_ROOT,
33 STAGING_SERVICE_ROOT,
34 Launchpad,
35 )
36from lazr.uri import URI
37
38
39CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
40
41
42LAUNCHPAD_API_URLS = {
43 'production': EDGE_SERVICE_ROOT,
44 'edge': EDGE_SERVICE_ROOT,
45 'staging': STAGING_SERVICE_ROOT,
46 'dev': 'https://api.launchpad.dev/beta/',
47 }
48
49
50def _get_api_url(service):
51 """Return the root URL of the Launchpad API.
52
53 e.g. For the 'edge' Launchpad service, this function returns
54 launchpadlib.launchpad.EDGE_SERVICE_ROOT.
55
56 :param service: A `LaunchpadService` object.
57 :return: A URL as a string.
58 """
59 if service._lp_instance is None:
60 lp_instance = service.DEFAULT_INSTANCE
61 else:
62 lp_instance = service._lp_instance
63 try:
64 return LAUNCHPAD_API_URLS[lp_instance]
65 except KeyError:
66 raise InvalidLaunchpadInstance(lp_instance)
67
68
69def _get_credential_path(service):
70 """Return the path to cached credentials for 'service'.
71
72 :param service: A `LaunchpadService` object.
73 :return: The path to a cached credentials file, which might not exist.
74 """
75 web_root_uri = URI(_get_api_url(service))
76 credential_name = 'creds-%s-bzr' % (web_root_uri.host)
77 return os.path.join(CACHE_DIRECTORY, credential_name)
78
79
80def _login_from_cache(consumer_name, service_root, cache_dir,
81 credential_cache, timeout=None, proxy_info=None):
82 """Use cached credentials if they exist, log in otherwise."""
83 try:
84 credentials = Credentials.load_from_path(credential_cache)
85 except (OSError, IOError):
86 launchpad = Launchpad.get_token_and_login(
87 consumer_name, service_root, cache_dir, timeout, proxy_info)
88 launchpad.credentials.save_to_path(credential_cache)
89 else:
90 access_key = credentials.access_token.key
91 access_secret = credentials.access_token.secret
92 launchpad = Launchpad.login(
93 consumer_name, access_key, access_secret, service_root,
94 cache_dir, timeout, proxy_info)
95 return launchpad
96
97
98def login(service, timeout=None, proxy_info=None):
99 """Log in to the Launchpad API.
100
101 :return: The root `Launchpad` object from launchpadlib.
102 """
103 credential_path = _get_credential_path(service)
104 launchpad = _login_from_cache(
105 'bzr', _get_api_url(service), CACHE_DIRECTORY, credential_path,
106 timeout, proxy_info)
107 launchpad._service = service
108 return launchpad
109
110
111def load_branch(launchpad, branch):
112 """Return the launchpadlib Branch object corresponding to 'branch'.
113
114 :param launchpad: The root `Launchpad` object from launchpadlib.
115 :param branch: A `bzrlib.branch.Branch`.
116 :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
117 `branch`.
118 :return: A launchpadlib Branch object.
119 """
120 service = launchpad._service
121 for url in branch.get_public_branch(), branch.get_push_location():
122 if url is None:
123 continue
124 try:
125 path = service._guess_branch_path(url)
126 except (errors.InvalidURL, NotLaunchpadBranch):
127 pass
128 else:
129 trace.mutter('Guessing path: %s', path)
130 uri = launchpad._root_uri.append(path)
131 uri_str = str(uri)
132 trace.mutter('Guessing url: %s', uri_str)
133 return launchpad.load(uri_str)
134 raise NotLaunchpadBranch(url)
0135
=== modified file 'bzrlib/plugins/launchpad/lp_directory.py'
--- bzrlib/plugins/launchpad/lp_directory.py 2009-03-23 14:59:43 +0000
+++ bzrlib/plugins/launchpad/lp_directory.py 2009-12-09 07:20:40 +0000
@@ -63,15 +63,9 @@
63 _request_factory=ResolveLaunchpadPathRequest,63 _request_factory=ResolveLaunchpadPathRequest,
64 _lp_login=None):64 _lp_login=None):
65 """Resolve the base URL for this transport."""65 """Resolve the base URL for this transport."""
66 service = LaunchpadService.for_url(url)
66 result = urlsplit(url)67 result = urlsplit(url)
67 # Perform an XMLRPC request to resolve the path
68 lp_instance = result[1]
69 if lp_instance == '':
70 lp_instance = None
71 elif lp_instance not in LaunchpadService.LAUNCHPAD_INSTANCE:
72 raise errors.InvalidURL(path=url)
73 resolve = _request_factory(result[2].strip('/'))68 resolve = _request_factory(result[2].strip('/'))
74 service = LaunchpadService(lp_instance=lp_instance)
75 try:69 try:
76 result = resolve.submit(service)70 result = resolve.submit(service)
77 except xmlrpclib.Fault, fault:71 except xmlrpclib.Fault, fault:
@@ -79,7 +73,7 @@
79 path=url, extra=fault.faultString)73 path=url, extra=fault.faultString)
8074
81 if 'launchpad' in debug.debug_flags:75 if 'launchpad' in debug.debug_flags:
82 trace.mutter("resolve_lp_path(%r) == %r", path, result)76 trace.mutter("resolve_lp_path(%r) == %r", url, result)
8377
84 if _lp_login is None:78 if _lp_login is None:
85 _lp_login = get_lp_login()79 _lp_login = get_lp_login()
8680
=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
--- bzrlib/plugins/launchpad/lp_registration.py 2009-11-05 18:32:31 +0000
+++ bzrlib/plugins/launchpad/lp_registration.py 2009-12-09 07:20:40 +0000
@@ -21,18 +21,15 @@
21import urllib21import urllib
22import xmlrpclib22import xmlrpclib
2323
24from bzrlib.lazy_import import lazy_import
25lazy_import(globals(), """
26from bzrlib import urlutils
27""")
28
29from bzrlib import (24from bzrlib import (
30 config,25 config,
31 errors,26 errors,
27 urlutils,
32 __version__ as _bzrlib_version,28 __version__ as _bzrlib_version,
33 )29 )
34from bzrlib.transport.http import _urllib2_wrappers30from bzrlib.transport.http import _urllib2_wrappers
3531
32
36# for testing, do33# for testing, do
37'''34'''
38export BZR_LP_XMLRPC_URL=http://xmlrpc.staging.launchpad.net/bazaar/35export BZR_LP_XMLRPC_URL=http://xmlrpc.staging.launchpad.net/bazaar/
@@ -123,7 +120,6 @@
123 % (_bzrlib_version, xmlrpclib.__version__)120 % (_bzrlib_version, xmlrpclib.__version__)
124 self.transport = transport121 self.transport = transport
125122
126
127 @property123 @property
128 def service_url(self):124 def service_url(self):
129 """Return the http or https url for the xmlrpc server.125 """Return the http or https url for the xmlrpc server.
@@ -141,6 +137,17 @@
141 else:137 else:
142 return self.DEFAULT_SERVICE_URL138 return self.DEFAULT_SERVICE_URL
143139
140 @classmethod
141 def for_url(cls, url, **kwargs):
142 """Return the Launchpad service corresponding to the given URL."""
143 result = urlsplit(url)
144 lp_instance = result[1]
145 if lp_instance == '':
146 lp_instance = None
147 elif lp_instance not in cls.LAUNCHPAD_INSTANCE:
148 raise errors.InvalidURL(path=url)
149 return cls(lp_instance=lp_instance, **kwargs)
150
144 def get_proxy(self, authenticated):151 def get_proxy(self, authenticated):
145 """Return the proxy for XMLRPC requests."""152 """Return the proxy for XMLRPC requests."""
146 if authenticated:153 if authenticated:
@@ -216,13 +223,7 @@
216 instance = self._lp_instance223 instance = self._lp_instance
217 return self.LAUNCHPAD_DOMAINS[instance]224 return self.LAUNCHPAD_DOMAINS[instance]
218225
219 def get_web_url_from_branch_url(self, branch_url, _request_factory=None):226 def _guess_branch_path(self, branch_url, _request_factory=None):
220 """Get the Launchpad web URL for the given branch URL.
221
222 :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
223 Launchpad branch URL.
224 :return: The URL of the branch on Launchpad.
225 """
226 scheme, hostinfo, path = urlsplit(branch_url)[:3]227 scheme, hostinfo, path = urlsplit(branch_url)[:3]
227 if _request_factory is None:228 if _request_factory is None:
228 _request_factory = ResolveLaunchpadPathRequest229 _request_factory = ResolveLaunchpadPathRequest
@@ -240,6 +241,16 @@
240 for domain in self.LAUNCHPAD_DOMAINS.itervalues())241 for domain in self.LAUNCHPAD_DOMAINS.itervalues())
241 if hostinfo not in domains:242 if hostinfo not in domains:
242 raise NotLaunchpadBranch(branch_url)243 raise NotLaunchpadBranch(branch_url)
244 return path.lstrip('/')
245
246 def get_web_url_from_branch_url(self, branch_url, _request_factory=None):
247 """Get the Launchpad web URL for the given branch URL.
248
249 :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
250 Launchpad branch URL.
251 :return: The URL of the branch on Launchpad.
252 """
253 path = self._guess_branch_path(branch_url, _request_factory)
243 return urlutils.join('https://code.%s' % self.domain, path)254 return urlutils.join('https://code.%s' % self.domain, path)
244255
245256