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

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
=== modified file 'NEWS'
--- NEWS 2009-12-16 20:01:49 +0000
+++ NEWS 2009-12-17 04:37:22 +0000
@@ -114,6 +114,12 @@
114 lengths. 114 lengths.
115 (Vincent Ladeuil)115 (Vincent Ladeuil)
116116
117* The new command ``bzr lp-mirror`` will request that Launchpad update its
118 mirror of a local branch. This command will only function if launchpadlib
119 is installed.
120 (Jonathan Lange)
121
122
117Bug Fixes123Bug Fixes
118*********124*********
119125
@@ -217,6 +223,11 @@
217223
218* ``bzrlib.textui`` (vestigial module) removed. (Martin Pool)224* ``bzrlib.textui`` (vestigial module) removed. (Martin Pool)
219225
226* The Launchpad plugin now has a function ``login`` which will log in to
227 Launchpad with launchpadlib, and ``load_branch`` which will return the
228 Launchpad Branch object corresponding to a given Bazaar Branch object.
229 (Jonathan Lange)
230
220Internals231Internals
221*********232*********
222233
223234
=== 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-17 04:37:22 +0000
@@ -36,15 +36,12 @@
36from bzrlib.directory_service import directories36from bzrlib.directory_service import directories
37from bzrlib.errors import (37from bzrlib.errors import (
38 BzrCommandError,38 BzrCommandError,
39 DependencyNotPresent,
39 InvalidURL,40 InvalidURL,
40 NoPublicBranch,41 NoPublicBranch,
41 NotBranchError,42 NotBranchError,
42 )43 )
43from bzrlib.help_topics import topic_registry44from bzrlib.help_topics import topic_registry
44from bzrlib.plugins.launchpad.lp_registration import (
45 LaunchpadService,
46 NotLaunchpadBranch,
47 )
4845
4946
50class cmd_register_branch(Command):47class cmd_register_branch(Command):
@@ -111,8 +108,8 @@
111 link_bug=None,108 link_bug=None,
112 dry_run=False):109 dry_run=False):
113 from bzrlib.plugins.launchpad.lp_registration import (110 from bzrlib.plugins.launchpad.lp_registration import (
114 LaunchpadService, BranchRegistrationRequest, BranchBugLinkRequest,111 BranchRegistrationRequest, BranchBugLinkRequest,
115 DryRunLaunchpadService)112 DryRunLaunchpadService, LaunchpadService)
116 if public_url is None:113 if public_url is None:
117 try:114 try:
118 b = _mod_branch.Branch.open_containing('.')[0]115 b = _mod_branch.Branch.open_containing('.')[0]
@@ -147,9 +144,9 @@
147 # Run on service entirely in memory144 # Run on service entirely in memory
148 service = DryRunLaunchpadService()145 service = DryRunLaunchpadService()
149 service.gather_user_credentials()146 service.gather_user_credentials()
150 branch_object_url = rego.submit(service)147 rego.submit(service)
151 if link_bug:148 if link_bug:
152 link_bug_url = linko.submit(service)149 linko.submit(service)
153 print 'Branch registered.'150 print 'Branch registered.'
154151
155register_command(cmd_register_branch)152register_command(cmd_register_branch)
@@ -181,6 +178,8 @@
181 yield branch_url178 yield branch_url
182179
183 def _get_web_url(self, service, location):180 def _get_web_url(self, service, location):
181 from bzrlib.plugins.launchpad.lp_registration import (
182 NotLaunchpadBranch)
184 for branch_url in self._possible_locations(location):183 for branch_url in self._possible_locations(location):
185 try:184 try:
186 return service.get_web_url_from_branch_url(branch_url)185 return service.get_web_url_from_branch_url(branch_url)
@@ -189,6 +188,8 @@
189 raise NotLaunchpadBranch(branch_url)188 raise NotLaunchpadBranch(branch_url)
190189
191 def run(self, location=None, dry_run=False):190 def run(self, location=None, dry_run=False):
191 from bzrlib.plugins.launchpad.lp_registration import (
192 LaunchpadService)
192 if location is None:193 if location is None:
193 location = u'.'194 location = u'.'
194 web_url = self._get_web_url(LaunchpadService(), location)195 web_url = self._get_web_url(LaunchpadService(), location)
@@ -226,6 +227,7 @@
226 ]227 ]
227228
228 def run(self, name=None, no_check=False, verbose=False):229 def run(self, name=None, no_check=False, verbose=False):
230 # This is totally separate from any launchpadlib login system.
229 from bzrlib.plugins.launchpad import account231 from bzrlib.plugins.launchpad import account
230 check_account = not no_check232 check_account = not no_check
231233
@@ -255,6 +257,26 @@
255register_command(cmd_launchpad_login)257register_command(cmd_launchpad_login)
256258
257259
260# XXX: cmd_launchpad_mirror is untested
261class cmd_launchpad_mirror(Command):
262 """Ask Launchpad to mirror a branch now."""
263
264 aliases = ['lp-mirror']
265 takes_args = ['location?']
266
267 def run(self, location='.'):
268 from bzrlib.plugins.launchpad import lp_api
269 from bzrlib.plugins.launchpad.lp_registration import LaunchpadService
270 branch = _mod_branch.Branch.open(location)
271 service = LaunchpadService()
272 launchpad = lp_api.login(service)
273 lp_branch = lp_api.load_branch(launchpad, branch)
274 lp_branch.requestMirror()
275
276
277register_command(cmd_launchpad_mirror)
278
279
258def _register_directory():280def _register_directory():
259 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',281 directories.register_lazy('lp:', 'bzrlib.plugins.launchpad.lp_directory',
260 'LaunchpadDirectory',282 'LaunchpadDirectory',
@@ -266,6 +288,7 @@
266 testmod_names = [288 testmod_names = [
267 'test_account',289 'test_account',
268 'test_register',290 'test_register',
291 'test_lp_api',
269 'test_lp_directory',292 'test_lp_directory',
270 'test_lp_login',293 'test_lp_login',
271 'test_lp_open',294 'test_lp_open',
272295
=== 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-17 04:37:22 +0000
@@ -0,0 +1,135 @@
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
19# Importing this module will be expensive, since it imports launchpadlib and
20# its dependencies. However, our plan is to only load this module when it is
21# needed by a command that uses it.
22
23
24import os
25
26from bzrlib import (
27 config,
28 errors,
29 osutils,
30 )
31from bzrlib.plugins.launchpad.lp_registration import (
32 InvalidLaunchpadInstance,
33 NotLaunchpadBranch,
34 )
35
36try:
37 import launchpadlib
38except ImportError, e:
39 raise errors.DependencyNotPresent('launchpadlib', e)
40
41from launchpadlib.launchpad import (
42 EDGE_SERVICE_ROOT,
43 STAGING_SERVICE_ROOT,
44 Launchpad,
45 )
46
47
48# Declare the minimum version of launchpadlib that we need in order to work.
49# 1.5.1 is the version of launchpadlib packaged in Ubuntu 9.10, the most
50# recent Ubuntu release at the time of writing.
51MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
52
53
54def get_cache_directory():
55 """Return the directory to cache launchpadlib objects in."""
56 return osutils.pathjoin(config.config_dir(), 'launchpad')
57
58
59def parse_launchpadlib_version(version_number):
60 """Parse a version number of the style used by launchpadlib."""
61 return tuple(map(int, version_number.split('.')))
62
63
64def check_launchpadlib_compatibility():
65 """Raise an error if launchpadlib has the wrong version number."""
66 installed_version = parse_launchpadlib_version(launchpadlib.__version__)
67 if installed_version < MINIMUM_LAUNCHPADLIB_VERSION:
68 raise errors.IncompatibleAPI(
69 'launchpadlib', MINIMUM_LAUNCHPADLIB_VERSION,
70 installed_version, installed_version)
71
72
73LAUNCHPAD_API_URLS = {
74 'production': 'https://api.launchpad.net/beta/',
75 'edge': EDGE_SERVICE_ROOT,
76 'staging': STAGING_SERVICE_ROOT,
77 'dev': 'https://api.launchpad.dev/beta/',
78 }
79
80
81def _get_api_url(service):
82 """Return the root URL of the Launchpad API.
83
84 e.g. For the 'edge' Launchpad service, this function returns
85 launchpadlib.launchpad.EDGE_SERVICE_ROOT.
86
87 :param service: A `LaunchpadService` object.
88 :return: A URL as a string.
89 """
90 if service._lp_instance is None:
91 lp_instance = service.DEFAULT_INSTANCE
92 else:
93 lp_instance = service._lp_instance
94 try:
95 return LAUNCHPAD_API_URLS[lp_instance]
96 except KeyError:
97 raise InvalidLaunchpadInstance(lp_instance)
98
99
100def login(service, timeout=None, proxy_info=None):
101 """Log in to the Launchpad API.
102
103 :return: The root `Launchpad` object from launchpadlib.
104 """
105 cache_directory = get_cache_directory()
106 launchpad = Launchpad.login_with(
107 'bzr', _get_api_url(service), cache_directory, timeout=timeout,
108 proxy_info=proxy_info)
109 # XXX: Work-around a minor security bug in launchpadlib 1.5.1, which would
110 # create this directory with default umask.
111 os.chmod(cache_directory, 0700)
112 return launchpad
113
114
115def load_branch(launchpad, branch):
116 """Return the launchpadlib Branch object corresponding to 'branch'.
117
118 :param launchpad: The root `Launchpad` object from launchpadlib.
119 :param branch: A `bzrlib.branch.Branch`.
120 :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
121 `branch`.
122 :return: A launchpadlib Branch object.
123 """
124 # XXX: This duplicates the "What are possible URLs for the branch that
125 # Launchpad might recognize" logic found in cmd_lp_open.
126
127 # XXX: This makes multiple roundtrips to Launchpad for what is
128 # conceptually a single operation -- get me the branches that match these
129 # URLs. Unfortunately, Launchpad's support for such operations is poor, so
130 # we have to allow multiple roundtrips.
131 for url in branch.get_public_branch(), branch.get_push_location():
132 lp_branch = launchpad.branches.getByUrl(url=url)
133 if lp_branch:
134 return lp_branch
135 raise NotLaunchpadBranch(url)
0136
=== 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-17 04:37:22 +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-17 04:37:22 +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
246257
=== added file 'bzrlib/plugins/launchpad/test_lp_api.py'
--- bzrlib/plugins/launchpad/test_lp_api.py 1970-01-01 00:00:00 +0000
+++ bzrlib/plugins/launchpad/test_lp_api.py 2009-12-17 04:37:22 +0000
@@ -0,0 +1,101 @@
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
18from bzrlib import commands, config, errors, osutils
19from bzrlib.plugins.launchpad import cmd_launchpad_mirror
20from bzrlib.tests import (
21 ModuleAvailableFeature,
22 TestCase,
23 TestCaseWithTransport,
24 )
25
26
27launchpadlib_feature = ModuleAvailableFeature('launchpadlib')
28
29
30class TestDependencyManagement(TestCase):
31 """Tests for managing the dependency on launchpadlib."""
32
33 _test_needs_features = [launchpadlib_feature]
34
35 def setUp(self):
36 TestCase.setUp(self)
37 from bzrlib.plugins.launchpad import lp_api
38 self.lp_api = lp_api
39
40 def patch(self, obj, name, value):
41 """Temporarily set the 'name' attribute of 'obj' to 'value'."""
42 real_value = getattr(obj, name)
43 setattr(obj, name, value)
44 self.addCleanup(setattr, obj, name, real_value)
45
46 def test_get_launchpadlib_version(self):
47 # parse_launchpadlib_version returns a tuple of a version number of
48 # the style used by launchpadlib.
49 version_info = self.lp_api.parse_launchpadlib_version('1.5.1')
50 self.assertEqual((1, 5, 1), version_info)
51
52 def test_supported_launchpadlib_version(self):
53 # If the installed version of launchpadlib is greater than the minimum
54 # required version of launchpadlib, check_launchpadlib_compatibility
55 # doesn't raise an error.
56 launchpadlib = launchpadlib_feature.module
57 self.patch(launchpadlib, '__version__', '1.5.1')
58 self.lp_api.MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
59 # Doesn't raise an exception.
60 self.lp_api.check_launchpadlib_compatibility()
61
62 def test_unsupported_launchpadlib_version(self):
63 # If the installed version of launchpadlib is less than the minimum
64 # required version of launchpadlib, check_launchpadlib_compatibility
65 # raises an IncompatibleAPI error.
66 launchpadlib = launchpadlib_feature.module
67 self.patch(launchpadlib, '__version__', '1.5.0')
68 self.lp_api.MINIMUM_LAUNCHPADLIB_VERSION = (1, 5, 1)
69 self.assertRaises(
70 errors.IncompatibleAPI,
71 self.lp_api.check_launchpadlib_compatibility)
72
73
74class TestCacheDirectory(TestCase):
75 """Tests for get_cache_directory."""
76
77 _test_needs_features = [launchpadlib_feature]
78
79 def test_get_cache_directory(self):
80 # get_cache_directory returns the path to a directory inside the
81 # Bazaar configuration directory.
82 from bzrlib.plugins.launchpad import lp_api
83 expected_path = osutils.pathjoin(config.config_dir(), 'launchpad')
84 self.assertEqual(expected_path, lp_api.get_cache_directory())
85
86
87class TestLaunchpadMirror(TestCaseWithTransport):
88 """Tests for the 'bzr lp-mirror' command."""
89
90 # Testing the lp-mirror command is quite hard, since it must talk to a
91 # Launchpad server. Here, we just test that the command exists.
92
93 _test_needs_features = [launchpadlib_feature]
94
95 def test_command_exists(self):
96 out, err = self.run_bzr(['launchpad-mirror', '--help'], retcode=0)
97 self.assertEqual('', err)
98
99 def test_alias_exists(self):
100 out, err = self.run_bzr(['lp-mirror', '--help'], retcode=0)
101 self.assertEqual('', err)
0102
=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py 2009-12-02 16:21:42 +0000
+++ bzrlib/win32utils.py 2009-12-17 04:37:22 +0000
@@ -22,9 +22,7 @@
22import glob22import glob
23import os23import os
24import re24import re
25import shlex
26import struct25import struct
27import StringIO
28import sys26import sys
2927
3028
@@ -392,7 +390,6 @@
392390
393391
394def _ensure_unicode(s):392def _ensure_unicode(s):
395 from bzrlib import osutils
396 if s and type(s) != unicode:393 if s and type(s) != unicode:
397 from bzrlib import osutils394 from bzrlib import osutils
398 s = s.decode(osutils.get_user_encoding())395 s = s.decode(osutils.get_user_encoding())
@@ -648,7 +645,7 @@
648 prototype = ctypes.WINFUNCTYPE(POINTER(LPCWSTR), LPCWSTR, POINTER(INT))645 prototype = ctypes.WINFUNCTYPE(POINTER(LPCWSTR), LPCWSTR, POINTER(INT))
649 command_line = GetCommandLine()646 command_line = GetCommandLine()
650 # Skip the first argument, since we only care about parameters647 # Skip the first argument, since we only care about parameters
651 argv = _command_line_to_argv(GetCommandLine())[1:]648 argv = _command_line_to_argv(command_line)[1:]
652 if getattr(sys, 'frozen', None) is None:649 if getattr(sys, 'frozen', None) is None:
653 # Invoked via 'python.exe' which takes the form:650 # Invoked via 'python.exe' which takes the form:
654 # python.exe [PYTHON_OPTIONS] C:\Path\bzr [BZR_OPTIONS]651 # python.exe [PYTHON_OPTIONS] C:\Path\bzr [BZR_OPTIONS]