Bazaar

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

  1. lp-login-oauth-2
  2. Merge into bzr.dev
Proposed by Jonathan Lange
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jml/bzr/lp-login-oauth-2
Merge into: lp:bzr
Diff against target: 523 lines (+305/-33)
7 files modified
NEWS (+11/-0)
bzrlib/plugins/launchpad/__init__.py (+31/-8)
bzrlib/plugins/launchpad/lp_api.py (+135/-0)
bzrlib/plugins/launchpad/lp_directory.py (+2/-8)
bzrlib/plugins/launchpad/lp_registration.py (+24/-13)
bzrlib/plugins/launchpad/test_lp_api.py (+101/-0)
bzrlib/win32utils.py (+1/-4)
To merge this branch: bzr merge lp:~jml/bzr/lp-login-oauth-2
Reviewer Review Type Date Requested Status
John A Meinel Approve
Martin Pool Needs Information
Review via email: mp+15897@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

Hello,

This branch adds a command 'lp-mirror' that requests Launchpad mirror a branch now. It uses launchpadlib, and is most interesting for the fact that it integrates launchpadlib dependencies into Bazaar.

jml

Revision history for this message
Alexander Belchenko (bialix) wrote :

If you want to make it really "soft" dependency you may think about using lazy_import for it. IMO.

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

For what, exactly?

Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (4.6 KiB)

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

Jonathan Lange wrote:
> Jonathan Lange has proposed merging lp:~jml/bzr/lp-login-oauth-2 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> Hello,
>
> This branch adds a command 'lp-mirror' that requests Launchpad mirror a branch now. It uses launchpadlib, and is most interesting for the fact that it integrates launchpadlib dependencies into Bazaar.
>
> jml
>

=== 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 21:04:18 +0000
@@ -111,7 +111,7 @@
             link_bug=None,
             dry_run=False):
         from bzrlib.plugins.launchpad.lp_registration import (
- - LaunchpadService, BranchRegistrationRequest,
BranchBugLinkRequest,
+ BranchRegistrationRequest, BranchBugLinkRequest,
             DryRunLaunchpadService)
         if public_url is None:
             try:

^- This change made me wonder, but it looks like we are importing it at
the module level. But if we are, then why aren't we importing the rest.

Also, it seems like it might be nicer to lazily import lp_registration.
I wouldn't expect a huge improvement to bzr startup time, but maybe
something?

Anyway, nothing you have to tweak, just a surprise on my part.

Most importantly, I can confirm that with this patch, and without
launchpadlib:

1) 'bzr lp-open' still works
2) 'bzr lp-mirror' fails with:
$ bzr lp-mirror
bzr: ERROR: launchpad-mirror requires launchpadlib

Which is a nice clean error.

I can't confirm that anything works, because I'm not sure (yet) how to
install everything for launchpadlib on win32.

Some basic comments by just looking at the code:

+CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')

Doesn't seem quite right for Windows. This does generally have a value,
but it isn't the place where you would put that sort of thing as a
Windows user.

It also seems surprising that 'lp-login' doesn't actually use the same
credentials path or storage that 'lp_api.login()' ends up using.

Which means that after running "bzr lp-login" doing "bzr lp-mirror" will
 (unless I'm misreading something) not actually work.

def login(service, timeout=None, proxy_info=None):
    """Log in to the Launchpad API.

    :return: The root `Launchpad` object from launchpadlib.
    """
    credential_path = _get_credential_path(service)
    launchpad = _login_from_cache(
        'bzr', _get_api_url(service), CACHE_DIRECTORY, credential_path,
        timeout, proxy_info)
    launchpad._service = service

^- Something seems a bit odd in this line, too. Why are you setting a
private member of a class? Given that _login_from_cache takes the
service root, it would seem that it should already have a reasonable
service.

I guess looking back at cmd_lp_mirror you have:
        service = LaunchpadService()
        lp_api = self._get_lp_api()
        launchpad = lp_api.login(service)

Which means that the LaunchpadService instance that login is using
*isn't* the launchpadlib api one, but it is the one we have already
coded in bzrlib.

This seems like a significant ...

Read more...

review: Needs Fixing
Revision history for this message
Alexander Belchenko (bialix) wrote :

> For what, exactly?

For py2exe and bzr.exe.

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

On Thu, Dec 10, 2009 at 9:47 PM, Alexander Belchenko <email address hidden> wrote:
>> For what, exactly?
>
> For py2exe and bzr.exe.

I meant, using lazy_import for what?

jml

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

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

Jonathan Lange wrote:
> On Thu, Dec 10, 2009 at 9:47 PM, Alexander Belchenko <email address hidden> wrote:
>>> For what, exactly?
>> For py2exe and bzr.exe.
>
> I meant, using lazy_import for what?
>
> jml

If you use lazy_import it won't be detected by py2exe, and thus won't
try to require you to have launchpadlib available to build the installer.

That said, I'd defer that until we actually try to build an installer
once this patch has landed.

John
=:->

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

iEYEARECAAYFAkshcFIACgkQJdeBCYSNAAMxXQCeOFarlbYEkrLvVdjgNbVTIyZU
CFQAnixpcuD7RJiGtRSui1ZKZMKpTtFR
=5bpz
-----END PGP SIGNATURE-----

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

On Fri, Dec 11, 2009 at 9:06 AM, John A Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Jonathan Lange wrote:
>> On Thu, Dec 10, 2009 at 9:47 PM, Alexander Belchenko <email address hidden> wrote:
>>>> For what, exactly?
>>> For py2exe and bzr.exe.
>>
>> I meant, using lazy_import for what?
>>
>> jml
>
> If you use lazy_import it won't be detected by py2exe, and thus won't
> try to require you to have launchpadlib available to build the installer.
>
> That said, I'd defer that until we actually try to build an installer
> once this patch has landed.
>

Sorry if I'm being a bit dense here, but *where* should I use
lazr_import? Which import should I change? All of them? The one in the
cmd_launchpad_mirror.run method?

jml

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

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

Jonathan Lange wrote:
> On Fri, Dec 11, 2009 at 9:06 AM, John A Meinel <email address hidden> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Jonathan Lange wrote:
>>> On Thu, Dec 10, 2009 at 9:47 PM, Alexander Belchenko <email address hidden> wrote:
>>>>> For what, exactly?
>>>> For py2exe and bzr.exe.
>>> I meant, using lazy_import for what?
>>>
>>> jml
>> If you use lazy_import it won't be detected by py2exe, and thus won't
>> try to require you to have launchpadlib available to build the installer.
>>
>> That said, I'd defer that until we actually try to build an installer
>> once this patch has landed.
>>
>
> Sorry if I'm being a bit dense here, but *where* should I use
> lazr_import? Which import should I change? All of them? The one in the
> cmd_launchpad_mirror.run method?
>
> jml

It would be the one in _get_lp_api, but I'm not actually advocating that
you do so until we've determined it is really necessary.

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

iEYEARECAAYFAkshwQwACgkQJdeBCYSNAANtAQCfWH3J9+Drq+dNXQdorCcN7sd0
XAsAoITrsVue6hFzBkeClD7ZevfbdXjW
=Wj76
-----END PGP SIGNATURE-----

Revision history for this message
Jonathan Lange (jml) wrote :
Download full text (6.4 KiB)

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Jonathan Lange wrote:
> > Jonathan Lange has proposed merging lp:~jml/bzr/lp-login-oauth-2 into
> lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> >
> >
> > Hello,
> >
> > This branch adds a command 'lp-mirror' that requests Launchpad mirror a
> branch now. It uses launchpadlib, and is most interesting for the fact that it
> integrates launchpadlib dependencies into Bazaar.
> >
> > jml
> >
>
>
> === 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 21:04:18 +0000
> @@ -111,7 +111,7 @@
> link_bug=None,
> dry_run=False):
> from bzrlib.plugins.launchpad.lp_registration import (
> - - LaunchpadService, BranchRegistrationRequest,
> BranchBugLinkRequest,
> + BranchRegistrationRequest, BranchBugLinkRequest,
> DryRunLaunchpadService)
> if public_url is None:
> try:
>
> ^- This change made me wonder, but it looks like we are importing it at
> the module level. But if we are, then why aren't we importing the rest.
>
> Also, it seems like it might be nicer to lazily import lp_registration.
> I wouldn't expect a huge improvement to bzr startup time, but maybe
> something?
>
> Anyway, nothing you have to tweak, just a surprise on my part.
>

I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.

>
> Most importantly, I can confirm that with this patch, and without
> launchpadlib:
>
> 1) 'bzr lp-open' still works
> 2) 'bzr lp-mirror' fails with:
> $ bzr lp-mirror
> bzr: ERROR: launchpad-mirror requires launchpadlib
>
> Which is a nice clean error.
>

Thanks.

I also confirmed that 'bzr rocks' doesn't try to import launchpadlib, or indeed anything expensive from the launchpad plugin.

> I can't confirm that anything works, because I'm not sure (yet) how to
> install everything for launchpadlib on win32.
>

:(

>
> Some basic comments by just looking at the code:
>
> +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
>
> Doesn't seem quite right for Windows. This does generally have a value,
> but it isn't the place where you would put that sort of thing as a
> Windows user.
>

I've tweaked this a bit so that it behaves like config_dir(). I've also changed the unix path to be ~/.cache/launchpadlib, which is slightly more standards compliant.

No tests, sadly.

>
> It also seems surprising that 'lp-login' doesn't actually use the same
> credentials path or storage that 'lp_api.login()' ends up using.
>

Well, surprise is an emotion and who am I to belittle or even predict the feelings of others.

lp-login actually has nothing to do with lp-mirror or with launchpadlib. lp-login is just a crappy thing to enable some of the other Launchpad interactions, particularly the directory service.

One day, I'd like lp-login to do things with launchpadlib, but I don't really know what the desired behaviour is. I think it's a good and safe thing to defer this thinking to a later patch.

> Which means that after...

Read more...

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

2009/12/16 Jonathan Lange <email address hidden>:
>> Also, it seems like it might be nicer to lazily import lp_registration.
>> I wouldn't expect a huge improvement to bzr startup time, but maybe
>> something?
>>
>> Anyway, nothing you have to tweak, just a surprise on my part.
>>
>
> I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.

jml and i looked at this - lazy import is no better than just not
loading the submodule til it's needed, and as it turns out it's
harder.

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

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

needs a news entry

115 + except ImportError:
116 + raise BzrCommandError(
117 + "%s requires launchpadlib" % self.name())
118 + return lp_api

I suspect this is duplicated but it's tolerable.

178 +
179 +# XXX: Not the right value for Windows
180 +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')

217 + credential_name = 'creds-%s-bzr' % (web_root_uri.host)
218 + return os.path.join(CACHE_DIRECTORY, credential_name)

I thought you were going to tweak that? Did you forget to push? I think it would be worth changing it because it's going to create persistent data, and there are win32utils things to get the right path. On Unix I'd like it to be either xdg compliant or inside ~/.bazaar.

Also, are you sure lplib save_to() will take care of setting the permissions?

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

On Wed, Dec 16, 2009 at 6:53 PM, Martin Pool <email address hidden> wrote:
> Review: Needs Information
> needs a news entry
>
> 115     + except ImportError:
> 116     + raise BzrCommandError(
> 117     + "%s requires launchpadlib" % self.name())
> 118     + return lp_api
>
> I suspect this is duplicated but it's tolerable.
>

Cool.

> 178     +
> 179     +# XXX: Not the right value for Windows
> 180     +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
>
>
> 217     + credential_name = 'creds-%s-bzr' % (web_root_uri.host)
> 218     + return os.path.join(CACHE_DIRECTORY, credential_name)
>
> I thought you were going to tweak that?  Did you forget to push?  I think it would be worth changing it because it's going to create persistent data, and there are win32utils things to get the right path.  On Unix I'd like it to be either xdg compliant or inside ~/.bazaar.
>

I did tweak it, you are looking at an out-of-date diff, methinks.

> Also, are you sure lplib save_to() will take care of setting the permissions?

No, it won't. Again, I think you are looking at an out-of-date diff.

I'll add a NEWS entry.

jml

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

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

Martin Pool wrote:
> 2009/12/16 Jonathan Lange <email address hidden>:
>>> Also, it seems like it might be nicer to lazily import lp_registration.
>>> I wouldn't expect a huge improvement to bzr startup time, but maybe
>>> something?
>>>
>>> Anyway, nothing you have to tweak, just a surprise on my part.
>>>
>> I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.
>
> jml and i looked at this - lazy import is no better than just not
> loading the submodule til it's needed, and as it turns out it's
> harder.
>

Yeah, lazy import + deciding whether or not to do something based on
whether it is available is ... tricky. Because the import occurs during
the attribute lookup.

If we want to make it a dependency that py2exe won't find, then we would
do something like:

try:
  lp_api = __import__('lp_api')
except ImportError:
  ...

If it still finds it, then

lazy_import(globals(), """
import lp_api
""")
try:
  getattr(lp_api, 'foo', None)
except ImportError:
 ...

It is pretty ugly, and I think we can just add a "don't include X" to
the setup.py code rather than hack around it that way.

John
=:->

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

iEYEARECAAYFAkso8YMACgkQJdeBCYSNAAOjhwCeOfU+S9+LHny7f/6n5ZedqEy5
YIUAn3mC7p+fX4JyeyagDnofY02WC6/O
=aDGj
-----END PGP SIGNATURE-----

Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (5.7 KiB)

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

>> === 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 21:04:18 +0000
>> @@ -111,7 +111,7 @@
>> link_bug=None,
>> dry_run=False):
>> from bzrlib.plugins.launchpad.lp_registration import (
>> - - LaunchpadService, BranchRegistrationRequest,
>> BranchBugLinkRequest,
>> + BranchRegistrationRequest, BranchBugLinkRequest,
>> DryRunLaunchpadService)
>> if public_url is None:
>> try:
>>
>> ^- This change made me wonder, but it looks like we are importing it at
>> the module level. But if we are, then why aren't we importing the rest.
>>
>> Also, it seems like it might be nicer to lazily import lp_registration.
>> I wouldn't expect a huge improvement to bzr startup time, but maybe
>> something?
>>
>> Anyway, nothing you have to tweak, just a surprise on my part.
>>
>
> I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.
>

For this specific part, I don't see how:

from bzrlib.lazy_import import lazy_import
lazy_import(globals(), """
from bzrlib.plugins.launchpad import lp_registration
""")

Is a "whole bunch of hard". However, if you are using these objects at
import time, just doing that at the top of the module seems reasonable.
(If you import LaunchpadService, you have done all the work to import
BranchRegistrationRequest, etc, you just haven't brought them into the
local namespace.)

...

>
>> Some basic comments by just looking at the code:
>>
>> +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
>>
>> Doesn't seem quite right for Windows. This does generally have a value,
>> but it isn't the place where you would put that sort of thing as a
>> Windows user.
>>
>
> I've tweaked this a bit so that it behaves like config_dir(). I've also changed the unix path to be ~/.cache/launchpadlib, which is slightly more standards compliant.
>
> No tests, sadly.

Couldn't you add a test that "lp_config_dir()" returns something that
ends in ".cache/launchpadlib" ?

As for "~/.cache/launchpadlib" I would have thought that *launchpadlib*
would have had a recommended cache location. If it doesn't, then we
shouldn't be using the name "launchpadlib", since we are *using* the
plugin, not authoring it.

Possibly "bzr_launchpad" since that is roughly the plugin's name. In
which case going to bzrlib.config.config_dir()+'/launchpad' would be a
more reasonable location. (And it gives you an APPDATA location on
windows without having to re-code all of that again.)

>
>> It also seems surprising that 'lp-login' doesn't actually use the same
>> credentials path or storage that 'lp_api.login()' ends up using.
>>
>
...

>> Which means that after running "bzr lp-login" doing "bzr lp-mirror" will
>> (unless I'm misreading something) not actually work.
>>
>
> I think you are misreading something. I can run lp-login and then lp-mirror.

You seem to be upset with the review, and I'm sorry if I've put you on
the defensive. I feel like having "bzr l...

Read more...

Revision history for this message
Jonathan Lange (jml) wrote :
Download full text (5.9 KiB)

On Thu, Dec 17, 2009 at 2:30 AM, John A Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>>> === 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 21:04:18 +0000
>>> @@ -111,7 +111,7 @@
>>>              link_bug=None,
>>>              dry_run=False):
>>>          from bzrlib.plugins.launchpad.lp_registration import (
>>> - -            LaunchpadService, BranchRegistrationRequest,
>>> BranchBugLinkRequest,
>>> +            BranchRegistrationRequest, BranchBugLinkRequest,
>>>              DryRunLaunchpadService)
>>>          if public_url is None:
>>>              try:
>>>
>>> ^- This change made me wonder, but it looks like we are importing it at
>>> the module level. But if we are, then why aren't we importing the rest.
>>>
>>> Also, it seems like it might be nicer to lazily import lp_registration.
>>> I wouldn't expect a huge improvement to bzr startup time, but maybe
>>> something?
>>>
>>> Anyway, nothing you have to tweak, just a surprise on my part.
>>>
>>
>> I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.
>>
>
> For this specific part, I don't see how:
>
> from bzrlib.lazy_import import lazy_import
> lazy_import(globals(), """
> from bzrlib.plugins.launchpad import lp_registration
> """)
>
> Is a "whole bunch of hard".

It doesn't work -- see http://paste.ubuntu.com/342421/

I tried a bunch of things based on suggestions from the #bzr channel.
Then, on poolie's recommendation, I gave up and moved everything to
scoped imports.

After I gave up, spiv made a suggestion:

<spiv> jml: if you do "from from bzrlib.plugins.launchpad import
lp_registration as _lazy_lp_registration" in the __init__.py, does
that fix it?

I haven't tried it.

...
>>> Some basic comments by just looking at the code:
>>>
>>> +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
>>>
>>> Doesn't seem quite right for Windows. This does generally have a value,
>>> but it isn't the place where you would put that sort of thing as a
>>> Windows user.
>>>
>>
>> I've tweaked this a bit so that it behaves like config_dir(). I've also changed the unix path to be ~/.cache/launchpadlib, which is slightly more standards compliant.
>>
>> No tests, sadly.
>
> Couldn't you add a test that "lp_config_dir()" returns something that
> ends in ".cache/launchpadlib" ?
>
> As for "~/.cache/launchpadlib" I would have thought that *launchpadlib*
> would have had a recommended cache location. If it doesn't, then we
> shouldn't be using the name "launchpadlib", since we are *using* the
> plugin, not authoring it.
>

I would have thought so too. launchpadlib was originally the simplest
thing that could possibly work, and is slowly improving as more people
write applications that use it.

> Possibly "bzr_launchpad" since that is roughly the plugin's name. In
> which case going to bzrlib.config.config_dir()+'/launchpad' would be a
> more reasonable location. (And it gives you an APPDATA location on
> windows without having to re-code all of that again.)
>

Done.

>>
>...

Read more...

Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (3.4 KiB)

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

...

>> For this specific part, I don't see how:
>>
>> from bzrlib.lazy_import import lazy_import
>> lazy_import(globals(), """
>> from bzrlib.plugins.launchpad import lp_registration
>> """)
>>
>> Is a "whole bunch of hard".
>
> It doesn't work -- see http://paste.ubuntu.com/342421/
>
> I tried a bunch of things based on suggestions from the #bzr channel.
> Then, on poolie's recommendation, I gave up and moved everything to
> scoped imports.
>
> After I gave up, spiv made a suggestion:
>
> <spiv> jml: if you do "from from bzrlib.plugins.launchpad import
> lp_registration as _lazy_lp_registration" in the __init__.py, does
> that fix it?
>
> I haven't tried it.
>

The problem is that you are assigning a lazily imported object to
another reference, which we can't trap, and we can't replace the new
object. As an example

foo.py:
lazy_import("bar")

bar.py:
frob = 10

quux.py:
from foo import bar

bar.frob # bad mojo

However, doing the above is bad python anyway. If you want "bar" in
"quux" then you should be importing it directly.

I'm guessing you may be importing classes rather than modules, which
also isn't a great thing to do, and lazy import makes it worse.
(isinstance(foo, lazy_class) doesn't always work.)

...

>> You seem to be upset with the review, and I'm sorry if I've put you on
>> the defensive. I feel like having "bzr lp-login" share its information
>> with "bzr lp-mirror" is a reasonable request.
>>
>
> I'm not at all upset with the review, and am sorry for seeming so.
>
> It's a reasonable request, but I think it's actually quite difficult
> to do well, and that it's not necessary for landing this patch. I'll
> happily file a bug about it once the patch lands.
>

Sure.

> ...
>>> Which is terrible.
>>>
>>> When I first wrote this branch, that was the only choice. Since then, I added features to the Launchpad API to get branches based on their URLs, so this logic is no longer needed. I've rewritten this code to use that API, which makes it much simpler and removes the need for the _service attribute.
>> So since launchpadlib has been updated, do we need to add version
>> checks/etc so that things 'break' in a clean manner. (If I have
>> launchpadlib 1.really-old installed, would I get a reasonable error
>> rather than something like AttributeError?)
>>
>
> We do. I made a note to do this, but forgot to actually do it. It's
> now done, with tests.
>
> I haven't been able to figure out how to write a test that checks to
> see what happens if launchpadlib is not available at all.
>

def test_lp_mirror_fails(self):
  if launchpadlib_feature.available():
    return # raise TestNotApplicable
  self.run_bzr_error(['you must install launchpadlib'],
            'lp-mirror')

Not that I think you have to do it.

...

> I've added some smoke tests. I'm not satisfied with them, but I think
> they're enough for landing for now. Once this lands, I'll file a bug
> about having a good testing strategy for commands that depend on
> launchpadlib.
>
> jml

You've satisfied my concerns. Now if you could just make lplib a sane
install :).

John
=:->
  review: approve
-----BEGIN PGP SIGNA...

Read more...

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

Just to add more clarity (I hope) I think John is explaining how you
_could_ use lazy_import, not actually asking you to do it.

I think we need a discussion about what lp-login and lp: are actually
meant to do long term. But not here.

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

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

John A Meinel wrote:
[...]
> The problem is that you are assigning a lazily imported object to
> another reference, which we can't trap, and we can't replace the new
> object. As an example

I don't think so, at least not exactly. My hypothesis is that actually
the problem is that normally this is a reasonable thing to do:

foo.py:
----
from x.y import z # x.y.z is a submodule that defines SomeClass, etc.
...
class Foo(z.SomeClass): ... # etc
----

That's not making a new reference to a lazy import placeholder, right?

But consider this:

x/y/__init__.py:
----
lazy_import(globals(), """
    from x.y import z
""")
----

Now foo.py will be importing the lazy 'z' reference that belongs to
x/y/__init__.py.

Hence my suggestion for that __init__.py to import and use it as _mod_z,
so that Python's import logic will find the z module rather than the
lazy import placeholder.

But it's a pretty nasty situation, because both files look utterly
reasonable and idiomatic in isolation, and the errors from lazy_import
tend to be pretty confusing.

-Andrew.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-16 20:01:49 +0000
3+++ NEWS 2009-12-17 04:37:22 +0000
4@@ -114,6 +114,12 @@
5 lengths.
6 (Vincent Ladeuil)
7
8+* The new command ``bzr lp-mirror`` will request that Launchpad update its
9+ mirror of a local branch. This command will only function if launchpadlib
10+ is installed.
11+ (Jonathan Lange)
12+
13+
14 Bug Fixes
15 *********
16
17@@ -217,6 +223,11 @@
18
19 * ``bzrlib.textui`` (vestigial module) removed. (Martin Pool)
20
21+* The Launchpad plugin now has a function ``login`` which will log in to
22+ Launchpad with launchpadlib, and ``load_branch`` which will return the
23+ Launchpad Branch object corresponding to a given Bazaar Branch object.
24+ (Jonathan Lange)
25+
26 Internals
27 *********
28
29
30=== modified file 'bzrlib/plugins/launchpad/__init__.py'
31--- bzrlib/plugins/launchpad/__init__.py 2009-12-07 15:47:05 +0000
32+++ bzrlib/plugins/launchpad/__init__.py 2009-12-17 04:37:22 +0000
33@@ -36,15 +36,12 @@
34 from bzrlib.directory_service import directories
35 from bzrlib.errors import (
36 BzrCommandError,
37+ DependencyNotPresent,
38 InvalidURL,
39 NoPublicBranch,
40 NotBranchError,
41 )
42 from bzrlib.help_topics import topic_registry
43-from bzrlib.plugins.launchpad.lp_registration import (
44- LaunchpadService,
45- NotLaunchpadBranch,
46- )
47
48
49 class cmd_register_branch(Command):
50@@ -111,8 +108,8 @@
51 link_bug=None,
52 dry_run=False):
53 from bzrlib.plugins.launchpad.lp_registration import (
54- LaunchpadService, BranchRegistrationRequest, BranchBugLinkRequest,
55- DryRunLaunchpadService)
56+ BranchRegistrationRequest, BranchBugLinkRequest,
57+ DryRunLaunchpadService, LaunchpadService)
58 if public_url is None:
59 try:
60 b = _mod_branch.Branch.open_containing('.')[0]
61@@ -147,9 +144,9 @@
62 # Run on service entirely in memory
63 service = DryRunLaunchpadService()
64 service.gather_user_credentials()
65- branch_object_url = rego.submit(service)
66+ rego.submit(service)
67 if link_bug:
68- link_bug_url = linko.submit(service)
69+ linko.submit(service)
70 print 'Branch registered.'
71
72 register_command(cmd_register_branch)
73@@ -181,6 +178,8 @@
74 yield branch_url
75
76 def _get_web_url(self, service, location):
77+ from bzrlib.plugins.launchpad.lp_registration import (
78+ NotLaunchpadBranch)
79 for branch_url in self._possible_locations(location):
80 try:
81 return service.get_web_url_from_branch_url(branch_url)
82@@ -189,6 +188,8 @@
83 raise NotLaunchpadBranch(branch_url)
84
85 def run(self, location=None, dry_run=False):
86+ from bzrlib.plugins.launchpad.lp_registration import (
87+ LaunchpadService)
88 if location is None:
89 location = u'.'
90 web_url = self._get_web_url(LaunchpadService(), location)
91@@ -226,6 +227,7 @@
92 ]
93
94 def run(self, name=None, no_check=False, verbose=False):
95+ # This is totally separate from any launchpadlib login system.
96 from bzrlib.plugins.launchpad import account
97 check_account = not no_check
98
99@@ -255,6 +257,26 @@
100 register_command(cmd_launchpad_login)
101
102
103+# XXX: cmd_launchpad_mirror is untested
104+class cmd_launchpad_mirror(Command):
105+ """Ask Launchpad to mirror a branch now."""
106+
107+ aliases = ['lp-mirror']
108+ takes_args = ['location?']
109+
110+ def run(self, location='.'):
111+ from bzrlib.plugins.launchpad import lp_api
112+ from bzrlib.plugins.launchpad.lp_registration import LaunchpadService
113+ branch = _mod_branch.Branch.open(location)
114+ service = LaunchpadService()
115+ launchpad = lp_api.login(service)
116+ lp_branch = lp_api.load_branch(launchpad, branch)
117+ lp_branch.requestMirror()
118+
119+
120+register_command(cmd_launchpad_mirror)
121+
122+
123 def _register_directory():
124 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',
125 'LaunchpadDirectory',
126@@ -266,6 +288,7 @@
127 testmod_names = [
128 'test_account',
129 'test_register',
130+ 'test_lp_api',
131 'test_lp_directory',
132 'test_lp_login',
133 'test_lp_open',
134
135=== added file 'bzrlib/plugins/launchpad/lp_api.py'
136--- bzrlib/plugins/launchpad/lp_api.py 1970-01-01 00:00:00 +0000
137+++ bzrlib/plugins/launchpad/lp_api.py 2009-12-17 04:37:22 +0000
138@@ -0,0 +1,135 @@
139+# Copyright (C) 2009 Canonical Ltd
140+#
141+# This program is free software; you can redistribute it and/or modify
142+# it under the terms of the GNU General Public License as published by
143+# the Free Software Foundation; either version 2 of the License, or
144+# (at your option) any later version.
145+#
146+# This program is distributed in the hope that it will be useful,
147+# but WITHOUT ANY WARRANTY; without even the implied warranty of
148+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
149+# GNU General Public License for more details.
150+#
151+# You should have received a copy of the GNU General Public License
152+# along with this program; if not, write to the Free Software
153+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
154+
155+"""Tools for dealing with the Launchpad API."""
156+
157+# Importing this module will be expensive, since it imports launchpadlib and
158+# its dependencies. However, our plan is to only load this module when it is
159+# needed by a command that uses it.
160+
161+
162+import os
163+
164+from bzrlib import (
165+ config,
166+ errors,
167+ osutils,
168+ )
169+from bzrlib.plugins.launchpad.lp_registration import (
170+ InvalidLaunchpadInstance,
171+ NotLaunchpadBranch,
172+ )
173+
174+try:
175+ import launchpadlib
176+except ImportError, e:
177+ raise errors.DependencyNotPresent('launchpadlib', e)
178+
179+from launchpadlib.launchpad import (
180+ EDGE_SERVICE_ROOT,
181+ STAGING_SERVICE_ROOT,
182+ Launchpad,
183+ )
184+
185+
186+# Declare the minimum version of launchpadlib that we need in order to work.
187+# 1.5.1 is the version of launchpadlib packaged in Ubuntu 9.10, the most
188+# recent Ubuntu release at the time of writing.
189+MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
190+
191+
192+def get_cache_directory():
193+ """Return the directory to cache launchpadlib objects in."""
194+ return osutils.pathjoin(config.config_dir(), 'launchpad')
195+
196+
197+def parse_launchpadlib_version(version_number):
198+ """Parse a version number of the style used by launchpadlib."""
199+ return tuple(map(int, version_number.split('.')))
200+
201+
202+def check_launchpadlib_compatibility():
203+ """Raise an error if launchpadlib has the wrong version number."""
204+ installed_version = parse_launchpadlib_version(launchpadlib.__version__)
205+ if installed_version < MINIMUM_LAUNCHPADLIB_VERSION:
206+ raise errors.IncompatibleAPI(
207+ 'launchpadlib', MINIMUM_LAUNCHPADLIB_VERSION,
208+ installed_version, installed_version)
209+
210+
211+LAUNCHPAD_API_URLS = {
212+ 'production': 'https://api.launchpad.net/beta/',
213+ 'edge': EDGE_SERVICE_ROOT,
214+ 'staging': STAGING_SERVICE_ROOT,
215+ 'dev': 'https://api.launchpad.dev/beta/',
216+ }
217+
218+
219+def _get_api_url(service):
220+ """Return the root URL of the Launchpad API.
221+
222+ e.g. For the 'edge' Launchpad service, this function returns
223+ launchpadlib.launchpad.EDGE_SERVICE_ROOT.
224+
225+ :param service: A `LaunchpadService` object.
226+ :return: A URL as a string.
227+ """
228+ if service._lp_instance is None:
229+ lp_instance = service.DEFAULT_INSTANCE
230+ else:
231+ lp_instance = service._lp_instance
232+ try:
233+ return LAUNCHPAD_API_URLS[lp_instance]
234+ except KeyError:
235+ raise InvalidLaunchpadInstance(lp_instance)
236+
237+
238+def login(service, timeout=None, proxy_info=None):
239+ """Log in to the Launchpad API.
240+
241+ :return: The root `Launchpad` object from launchpadlib.
242+ """
243+ cache_directory = get_cache_directory()
244+ launchpad = Launchpad.login_with(
245+ 'bzr', _get_api_url(service), cache_directory, timeout=timeout,
246+ proxy_info=proxy_info)
247+ # XXX: Work-around a minor security bug in launchpadlib 1.5.1, which would
248+ # create this directory with default umask.
249+ os.chmod(cache_directory, 0700)
250+ return launchpad
251+
252+
253+def load_branch(launchpad, branch):
254+ """Return the launchpadlib Branch object corresponding to 'branch'.
255+
256+ :param launchpad: The root `Launchpad` object from launchpadlib.
257+ :param branch: A `bzrlib.branch.Branch`.
258+ :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
259+ `branch`.
260+ :return: A launchpadlib Branch object.
261+ """
262+ # XXX: This duplicates the "What are possible URLs for the branch that
263+ # Launchpad might recognize" logic found in cmd_lp_open.
264+
265+ # XXX: This makes multiple roundtrips to Launchpad for what is
266+ # conceptually a single operation -- get me the branches that match these
267+ # URLs. Unfortunately, Launchpad's support for such operations is poor, so
268+ # we have to allow multiple roundtrips.
269+ for url in branch.get_public_branch(), branch.get_push_location():
270+ lp_branch = launchpad.branches.getByUrl(url=url)
271+ if lp_branch:
272+ return lp_branch
273+ raise NotLaunchpadBranch(url)
274
275=== modified file 'bzrlib/plugins/launchpad/lp_directory.py'
276--- bzrlib/plugins/launchpad/lp_directory.py 2009-03-23 14:59:43 +0000
277+++ bzrlib/plugins/launchpad/lp_directory.py 2009-12-17 04:37:22 +0000
278@@ -63,15 +63,9 @@
279 _request_factory=ResolveLaunchpadPathRequest,
280 _lp_login=None):
281 """Resolve the base URL for this transport."""
282+ service = LaunchpadService.for_url(url)
283 result = urlsplit(url)
284- # Perform an XMLRPC request to resolve the path
285- lp_instance = result[1]
286- if lp_instance == '':
287- lp_instance = None
288- elif lp_instance not in LaunchpadService.LAUNCHPAD_INSTANCE:
289- raise errors.InvalidURL(path=url)
290 resolve = _request_factory(result[2].strip('/'))
291- service = LaunchpadService(lp_instance=lp_instance)
292 try:
293 result = resolve.submit(service)
294 except xmlrpclib.Fault, fault:
295@@ -79,7 +73,7 @@
296 path=url, extra=fault.faultString)
297
298 if 'launchpad' in debug.debug_flags:
299- trace.mutter("resolve_lp_path(%r) == %r", path, result)
300+ trace.mutter("resolve_lp_path(%r) == %r", url, result)
301
302 if _lp_login is None:
303 _lp_login = get_lp_login()
304
305=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
306--- bzrlib/plugins/launchpad/lp_registration.py 2009-11-05 18:32:31 +0000
307+++ bzrlib/plugins/launchpad/lp_registration.py 2009-12-17 04:37:22 +0000
308@@ -21,18 +21,15 @@
309 import urllib
310 import xmlrpclib
311
312-from bzrlib.lazy_import import lazy_import
313-lazy_import(globals(), """
314-from bzrlib import urlutils
315-""")
316-
317 from bzrlib import (
318 config,
319 errors,
320+ urlutils,
321 __version__ as _bzrlib_version,
322 )
323 from bzrlib.transport.http import _urllib2_wrappers
324
325+
326 # for testing, do
327 '''
328 export BZR_LP_XMLRPC_URL=http://xmlrpc.staging.launchpad.net/bazaar/
329@@ -123,7 +120,6 @@
330 % (_bzrlib_version, xmlrpclib.__version__)
331 self.transport = transport
332
333-
334 @property
335 def service_url(self):
336 """Return the http or https url for the xmlrpc server.
337@@ -141,6 +137,17 @@
338 else:
339 return self.DEFAULT_SERVICE_URL
340
341+ @classmethod
342+ def for_url(cls, url, **kwargs):
343+ """Return the Launchpad service corresponding to the given URL."""
344+ result = urlsplit(url)
345+ lp_instance = result[1]
346+ if lp_instance == '':
347+ lp_instance = None
348+ elif lp_instance not in cls.LAUNCHPAD_INSTANCE:
349+ raise errors.InvalidURL(path=url)
350+ return cls(lp_instance=lp_instance, **kwargs)
351+
352 def get_proxy(self, authenticated):
353 """Return the proxy for XMLRPC requests."""
354 if authenticated:
355@@ -216,13 +223,7 @@
356 instance = self._lp_instance
357 return self.LAUNCHPAD_DOMAINS[instance]
358
359- def get_web_url_from_branch_url(self, branch_url, _request_factory=None):
360- """Get the Launchpad web URL for the given branch URL.
361-
362- :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
363- Launchpad branch URL.
364- :return: The URL of the branch on Launchpad.
365- """
366+ def _guess_branch_path(self, branch_url, _request_factory=None):
367 scheme, hostinfo, path = urlsplit(branch_url)[:3]
368 if _request_factory is None:
369 _request_factory = ResolveLaunchpadPathRequest
370@@ -240,6 +241,16 @@
371 for domain in self.LAUNCHPAD_DOMAINS.itervalues())
372 if hostinfo not in domains:
373 raise NotLaunchpadBranch(branch_url)
374+ return path.lstrip('/')
375+
376+ def get_web_url_from_branch_url(self, branch_url, _request_factory=None):
377+ """Get the Launchpad web URL for the given branch URL.
378+
379+ :raise errors.InvalidURL: if 'branch_url' cannot be identified as a
380+ Launchpad branch URL.
381+ :return: The URL of the branch on Launchpad.
382+ """
383+ path = self._guess_branch_path(branch_url, _request_factory)
384 return urlutils.join('https://code.%s' % self.domain, path)
385
386
387
388=== added file 'bzrlib/plugins/launchpad/test_lp_api.py'
389--- bzrlib/plugins/launchpad/test_lp_api.py 1970-01-01 00:00:00 +0000
390+++ bzrlib/plugins/launchpad/test_lp_api.py 2009-12-17 04:37:22 +0000
391@@ -0,0 +1,101 @@
392+# Copyright (C) 2009 Canonical Ltd
393+#
394+# This program is free software; you can redistribute it and/or modify
395+# it under the terms of the GNU General Public License as published by
396+# the Free Software Foundation; either version 2 of the License, or
397+# (at your option) any later version.
398+#
399+# This program is distributed in the hope that it will be useful,
400+# but WITHOUT ANY WARRANTY; without even the implied warranty of
401+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
402+# GNU General Public License for more details.
403+#
404+# You should have received a copy of the GNU General Public License
405+# along with this program; if not, write to the Free Software
406+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
407+
408+
409+from bzrlib import commands, config, errors, osutils
410+from bzrlib.plugins.launchpad import cmd_launchpad_mirror
411+from bzrlib.tests import (
412+ ModuleAvailableFeature,
413+ TestCase,
414+ TestCaseWithTransport,
415+ )
416+
417+
418+launchpadlib_feature = ModuleAvailableFeature('launchpadlib')
419+
420+
421+class TestDependencyManagement(TestCase):
422+ """Tests for managing the dependency on launchpadlib."""
423+
424+ _test_needs_features = [launchpadlib_feature]
425+
426+ def setUp(self):
427+ TestCase.setUp(self)
428+ from bzrlib.plugins.launchpad import lp_api
429+ self.lp_api = lp_api
430+
431+ def patch(self, obj, name, value):
432+ """Temporarily set the 'name' attribute of 'obj' to 'value'."""
433+ real_value = getattr(obj, name)
434+ setattr(obj, name, value)
435+ self.addCleanup(setattr, obj, name, real_value)
436+
437+ def test_get_launchpadlib_version(self):
438+ # parse_launchpadlib_version returns a tuple of a version number of
439+ # the style used by launchpadlib.
440+ version_info = self.lp_api.parse_launchpadlib_version('1.5.1')
441+ self.assertEqual((1, 5, 1), version_info)
442+
443+ def test_supported_launchpadlib_version(self):
444+ # If the installed version of launchpadlib is greater than the minimum
445+ # required version of launchpadlib, check_launchpadlib_compatibility
446+ # doesn't raise an error.
447+ launchpadlib = launchpadlib_feature.module
448+ self.patch(launchpadlib, '__version__', '1.5.1')
449+ self.lp_api.MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
450+ # Doesn't raise an exception.
451+ self.lp_api.check_launchpadlib_compatibility()
452+
453+ def test_unsupported_launchpadlib_version(self):
454+ # If the installed version of launchpadlib is less than the minimum
455+ # required version of launchpadlib, check_launchpadlib_compatibility
456+ # raises an IncompatibleAPI error.
457+ launchpadlib = launchpadlib_feature.module
458+ self.patch(launchpadlib, '__version__', '1.5.0')
459+ self.lp_api.MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
460+ self.assertRaises(
461+ errors.IncompatibleAPI,
462+ self.lp_api.check_launchpadlib_compatibility)
463+
464+
465+class TestCacheDirectory(TestCase):
466+ """Tests for get_cache_directory."""
467+
468+ _test_needs_features = [launchpadlib_feature]
469+
470+ def test_get_cache_directory(self):
471+ # get_cache_directory returns the path to a directory inside the
472+ # Bazaar configuration directory.
473+ from bzrlib.plugins.launchpad import lp_api
474+ expected_path = osutils.pathjoin(config.config_dir(), 'launchpad')
475+ self.assertEqual(expected_path, lp_api.get_cache_directory())
476+
477+
478+class TestLaunchpadMirror(TestCaseWithTransport):
479+ """Tests for the 'bzr lp-mirror' command."""
480+
481+ # Testing the lp-mirror command is quite hard, since it must talk to a
482+ # Launchpad server. Here, we just test that the command exists.
483+
484+ _test_needs_features = [launchpadlib_feature]
485+
486+ def test_command_exists(self):
487+ out, err = self.run_bzr(['launchpad-mirror', '--help'], retcode=0)
488+ self.assertEqual('', err)
489+
490+ def test_alias_exists(self):
491+ out, err = self.run_bzr(['lp-mirror', '--help'], retcode=0)
492+ self.assertEqual('', err)
493
494=== modified file 'bzrlib/win32utils.py'
495--- bzrlib/win32utils.py 2009-12-02 16:21:42 +0000
496+++ bzrlib/win32utils.py 2009-12-17 04:37:22 +0000
497@@ -22,9 +22,7 @@
498 import glob
499 import os
500 import re
501-import shlex
502 import struct
503-import StringIO
504 import sys
505
506
507@@ -392,7 +390,6 @@
508
509
510 def _ensure_unicode(s):
511- from bzrlib import osutils
512 if s and type(s) != unicode:
513 from bzrlib import osutils
514 s = s.decode(osutils.get_user_encoding())
515@@ -648,7 +645,7 @@
516 prototype = ctypes.WINFUNCTYPE(POINTER(LPCWSTR), LPCWSTR, POINTER(INT))
517 command_line = GetCommandLine()
518 # Skip the first argument, since we only care about parameters
519- argv = _command_line_to_argv(GetCommandLine())[1:]
520+ argv = _command_line_to_argv(command_line)[1:]
521 if getattr(sys, 'frozen', None) is None:
522 # Invoked via 'python.exe' which takes the form:
523 # python.exe [PYTHON_OPTIONS] C:\Path\bzr [BZR_OPTIONS]