Merge lp:~jspashett/bzr/587868_args_handling_cant_debug into lp:bzr

[Jason Spashett]

Change to enable debugging of bzr under windows, pdb, eclipse et al.
* Compare the number of arguments from GetCommandLineW with sys.argv and remove any 'extra' arguments present obtained from GetCommandLineW therby eliminating any unwanted args if run under a debugger.

e.g. python -m pdb bzr status

currently yeilds -m pdb bzr status, and bzr complains about arguments.

Rationale: In all cases, using sys.argv or GetCommandLineW command arguments are seperated by spaces. Hence the number of arguments should agree. We do not use sys.argv becuase the encoding can be an unknown multi-byte. The assumption is that arguments will always be seperated by a space (as Splitter class assumes, and presumably python itself, as well as the command interpreter) thereore we can use this counting method to determine the proper argument start.

[Alexander Belchenko (bialix)]

1) Your changes are conflicting with current state of bzr.dev. Please, merge lp:bzr into your branch and resolve conflicts. Then commit and push new state to lp.

2) I think you don't need additional function process_metachars_in_commandline, do you?

3) Why you have changed name of the function to _win32syscall_command_line_to_argv? What it should say to other people?

4) Don't use `raise AssertionError()`, use instead BzrInternalError.

review: Needs Fixing

[Alexander Belchenko (bialix)]

5) You don't need to use such complex code as

arguments = []
arguments.extend(s)

There is enough to use just:

arguments = list(s)

6) len (sys.argv) <-- don't put a space between len and parenthesis.

review: Needs Fixing

[Jason Spashett (jspashett)]

1) Have merged and resolved, removing your fix for 588277 as this fix fixes that bug too.
2) Yes. Have refactored into call site
3) Thought it would be more informative, but have changed the name back
4) Ok, changed.
5) Yes, my bad python
6) Changed.

I have not updated the news file to say I've removed the code fix for 588277.

[Alexander Belchenko (bialix)]

I don't think that removing my fix for bug 588277 is the right thing. And btw, have you tested your branch manually with --profile-imports in the real command line?

[Jason Spashett (jspashett)]

Alexander Belchenko wrote:
> I don't think that removing my fix for bug 588277 is the right thing. And btw, have you tested your branch manually with --profile-imports in the real command line?
>
Yes I did test it with "python bzr --profile-imports status". Perhaps
it's not right as you say but It does get rid of the message "bzr:
ERROR: no such option: --profile-imports" though, and it does print out
the profile to stdout.

Like so:

C:\bzr\bazaar\587868_args_handling_cant_debug>python bzr.py
--profile-imports status 2>&1 | more
unknown:
  .project
  .pydevproject
  bzr.py
  cum inline name @ file:line
 92.2 1.5 bzrlib.commands @ __main__:133
 80.8 1.0 + [disable_plugins, load_plugins]bzrlib.plugin @
bzrlib.commands:52
 79.8 61.1 ++ [osutils]bzrlib @ bzrlib.plugin:36
  ...etc

It works because --profile-imports is removed from sys.argv, and that is
the length used in my fix, so it is also removed from the string
returned by GetCommandLineW
However this isn't correct because if you run python bzr status
--profile-imports 2>&1 | more with --profile-imports last, it does not
work. So your code is required. I could uncomit my change and do it
again. But since sys.argv is modified I don't think my fix will work
properly with as it assumes the arguments to the front should be removed
not arbitrary ones in the middle.

bzr, line 55: sys.argv.remove('--profile-imports')

I think therefore I will scrap this branch as I don't think there is a
way around this.

[Alexander Belchenko (bialix)]

Jason Spashett пишет:
> I think therefore I will scrap this branch as I don't think there is a
> way around this.

Not so fast, please.

I think you need to add the code which removes --profile-imports
before you start comparing with len(sys.argv).
It's a bit tricky because arguments is a list of 2-tuples.

So you need something like this

if '--profile-imports' not in sys.argv:
    if (False, '--profile-imports') in arguments:
        arguments.remove((False, '--profile-imports'))
    elif (True, '--profile-imports') in arguments:
        arguments.remove((True, '--profile-imports'))

Which is a bit verbose, but it should handle both cases (plain and
quoted --profile-imports).

[Alexander Belchenko (bialix)]

does PEB means Process Enviroment Block? http://undocumented.ntinternals.net/UserMode/Undocumented%20Functions/NT%20Objects/Process/PEB.html

[Alexander Belchenko (bialix)]

2 more fixes needed:

1) you can't raise InternalBzrError without importing this name first, so please add this line before raise:

    from bzrlib.errors import InternalBzrError

and sorry for my typo in error class name, it's actually InternalBzrError not BzrInternalError.

2) Bazaar code guideline requires only 2 blank lines between high-level functions, so please remove extra blank line after `return argv`

And just a note: I'm usually trying to create minimal possible patch and avoid changing names of variables unless it's strictly necessary, because it's just creates a noise which makes review harder. Your change of args to argv is not strictly necessary here, although I don't mind with such change. It won't block your patch. But such change don't add any real value to the patch.

[Jason Spashett (jspashett)]

Alexander Belchenko wrote:
> does PEB means Process Enviroment Block? http://undocumented.ntinternals.net/UserMode/Undocumented%20Functions/NT%20Objects/Process/PEB.html
>
Yes, the RTL_USER_PROCESS_PARAMETERS is in there. All I meant was that
sys.argv in python and GetCommandLineW should look at the same data,
except sys.argv is locale encoded. At least this is what I believe.

[Jason Spashett (jspashett)]

Alexander Belchenko wrote:
> 2 more fixes needed:
>
> 1) you can't raise InternalBzrError without importing this name first, so please add this line before raise:
>
> from bzrlib.errors import InternalBzrError
>
> and sorry for my typo in error class name, it's actually InternalBzrError not BzrInternalError.
>
> 2) Bazaar code guideline requires only 2 blank lines between high-level functions, so please remove extra blank line after `return argv`
>
> And just a note: I'm usually trying to create minimal possible patch and avoid changing names of variables unless it's strictly necessary, because it's just creates a noise which makes review harder. Your change of args to argv is not strictly necessary here, although I don't mind with such change. It won't block your patch. But such change don't add any real value to the patch.
>
Alexander,

I put the code you suggested in, but have trouble with the import and
can't tell why at the moment:

    module = __import__(module_python_path, scope, scope, [member])
  File
"C:\bzr\bazaar\587868_args_handling_cant_debug\bzrlib\win32utils.py",
line 28, in <module>
    from bzrlib.errors import InternalBzrError
  File "C:\bzr\bazaar\587868_args_handling_cant_debug\bzrlib\errors.py",
line 20, in <module>
    from bzrlib import (
ImportError: cannot import name osutils

[Alexander Belchenko (bialix)]

Jason Spashett пишет:
> I put the code you suggested in, but have trouble with the import and
> can't tell why at the moment:
>
> module = __import__(module_python_path, scope, scope, [member])
> File
> "C:\bzr\bazaar\587868_args_handling_cant_debug\bzrlib\win32utils.py",
> line 28, in <module>
> from bzrlib.errors import InternalBzrError

You have this error because osutils module tries to import win32utils
module. But win32utils tries to import bzrlib which in fact imports
osutils.

Don't put this import at the beginning of the file. Put it inside the
"if" block, right before raise statement, e.g.:

    if len(arguments) < len(sys.argv):
        from bzrlib.errors import InternalBzrError
        raise InternalBzrError("len(GetCommandLineW(...)) <
len(sys.argv) should not be possible")

That will work.

[Alexander Belchenko (bialix)]

I think, now it's ready. Except... you need corresponding NEWS entry! Please, add one in the bugfix section.

Also we need a second approver to land this patch. Maybe Martin gz? ;-)

review: Approve

[Martin Packman (gz)]

This basically looks fine to me, but could do with some tests. It's hard to tell just from the diff that, for instance, the py2exe case is still correct. Perhaps Alexander could help you out with that?

Some follow on comments about specific parts of change already mentioned in review:

+ # Bug #588277 remove the --profile-imports from the command line arguments

I realise the below code is just trying to match what the code in the bzr script is doing, but a better option would be to change that to not do `sys.argv.remove('--profile-imports')` at all, and add it to the global args list.

+ from bzrlib.errors import InternalBzrError
+ raise InternalBzrError("len(GetCommandLineW(...)) < len(sys.argv) should not be possible")

Just AssertionError would be fine here.

I'd have no problem with this landing as-is though.

review: Approve

[Jason Spashett (jspashett)]

Martin [gz] wrote:
> Review: Approve
> This basically looks fine to me, but could do with some tests. It's hard to tell just from the diff that, for instance, the py2exe case is still correct. Perhaps Alexander could help you out with that?
>
> Some follow on comments about specific parts of change already mentioned in review:
>
> + # Bug #588277 remove the --profile-imports from the command line arguments
>
> I realise the below code is just trying to match what the code in the bzr script is doing, but a better option would be to change that to not do `sys.argv.remove('--profile-imports')` at all, and add it to the global args list.
>
>
Yes I agree with that. I am not familiar with the global args list
though. My thought was that sys.argv shouldn't be modified, and that
there should be some args list that is passed around instead of
referencing sys.args from within the lower code. That way we can do what
ever is necessary to prepare the arguments list. Unfortunately that's
too many changes as sys.argv is used in many places as you would expect.

> + from bzrlib.errors import InternalBzrError
> + raise InternalBzrError("len(GetCommandLineW(...)) < len(sys.argv) should not be possible")
>
> Just AssertionError would be fine here.
>
> I'd have no problem with this landing as-is though.
>

If you want I can start up another branch try and do what you say Martin
and if Alexander thinks it's a good idea.

[Jason Spashett (jspashett)]

Jason Spashett wrote:
> Martin [gz] wrote:
>
> ...
>> Just AssertionError would be fine here.
>>
>> I'd have no problem with this landing as-is though.
>>
>>
>
> If you want I can start up another branch try and do what you say Martin
> and if Alexander thinks it's a good idea.
>
I've just 'discovered' that the unit tests don't pass, and they don't
because of course sys.argv isn't set. And so an exception is raised, namely:
raise InternalBzrError("len(GetCommandLineW(...)) < len(sys.argv) should
not be possible")
It's all rather messy now somehow.

I suppose that could be transformed into a > if statement instead.

Is it possible to call near the start of execution the GetCommandLineW
function, process it for 'extraneous' args and then set sys.argv to the
result? would this work? I just say that because it may get around these
fiddly special cases that are now cropping up -- like the
--profile-imports fix. Instead, fix it up once and assign it to
sys.argv, and the rest of the code can carry on as it does removing args
or whatever it wants to do.

[Martin Packman (gz)]

Hm, yes. Means move some code around, because the order must be:
1) Split line
2) Strip args from front
3) Glob unquoted args

The unit tests for _command_line_to_argv aren't expecting step 2 so don't think you'll need to be looking at sys.argv there. You could change the tests, but it's good to have them reasonably isolated. Might be better to split the three steps into separate private functions and call 1 followed by 3 for those tests.

I don't think assigning to sys.argv will really help anything, there's only one entry point for this code already, it's just been poked too many times and become rather gnarly.

On the --profile-imports front, how about just removing the removal from from the bzr script and adding to bzrlib.commands.run_bzr something like:
    elif a == '--profile-imports':
        pass # already handled in startup script

[Jason Spashett (jspashett)]

Martin [gz] wrote:
> Hm, yes. Means move some code around, because the order must be:
> 1) Split line
> 2) Strip args from front
> 3) Glob unquoted args
>
> The unit tests for _command_line_to_argv aren't expecting step 2 so don't think you'll need to be looking at sys.argv there. You could change the tests, but it's good to have them reasonably isolated. Might be better to split the three steps into separate private functions and call 1 followed by 3 for those tests.
>
> I don't think assigning to sys.argv will really help anything, there's only one entry point for this code already, it's just been poked too many times and become rather gnarly.
>
>
Yes, well I have more questions than answers now. Presumably we have to
call GetCommandLineW on windows. So what happens on linux if the locale
is ascii but a file we try and open is utf with 'unicode' characters in
it? Does it fail? Sorry to be a pain but I am just trying to understand
why things are they way they are.

I was thinking of this sort of thing (not exactly this), so that the
command line is 'fixed' and out of the way. Of course there would need
to be changes in win32utils.py too, refactorings, which I've hacked in
but not shown.
C:\bzr\bazaar\587868_args_handling_cant_debug>bzr diff
=== modified file 'bzr'
--- bzr 2010-05-27 17:59:18 +0000
+++ bzr 2010-06-11 00:42:26 +0000
@@ -49,6 +49,16 @@
 if hasattr(os, "unsetenv"):
     os.unsetenv(REINVOKE)

+if sys.platform.startswith('win'):
+ from bzrlib.win32utils import get_unicode_argv
+ new_args = [u'bzr']
+ new_args.extend(get_unicode_argv())
+ if len(new_args) < len(sys.argv):
+ raise AssertError("len(GetCommandLineW(...)) < len(sys.argv)
should not be possible")
+ new_args = new_args[len(new_args) - len(sys.argv):]
+ sys.argv = new_args
+
+

In other words the work done to get the correct command line is called
once from bzr. Things appear to then work, including the unit tests.

> On the --profile-imports front, how about just removing the removal from from the bzr script and adding to bzrlib.commands.run_bzr something like:
> elif a == '--profile-imports':
> pass # already handled in startup script
>

Should probably do this anyway/aswell.

[Vincent Ladeuil (vila)]

@Jason, Thanks a lot, I think the only missing bit now is the contributor agreement: http://www.canonical.com/contributors

[Jason Spashett (jspashett)]

Vincent Ladeuil wrote:
> @Jason, Thanks a lot, I think the only missing bit now is the contributor agreement: http://www.canonical.com/contributors
>
I can do that Vincent, but my attempted patch isn't good enough yet, and
I would be interested to see what people thing of my last suggestion. If
that's of no help then I'll do what Matrin [gz] suggests.

[Vincent Ladeuil (vila)]

@Jason: Ok, so, I'll put this proposal status to 'Work In progress' then.
From there, you can push new changes to the same branch and set the status back to 'Needs Review', people
that have already review the previous version should be notified and asked to review again.

If you have doubts about how your changes will behave outside of windows, feel free to add more tests and ping me
on IRC #bzr or here, I can help you run these tests on http://babune.ladeuil.net:24842.

[Jason Spashett (jspashett)]

I have now re-done this fix, hopefully it's reasonable and should avoide the cases were we had to adjust the command line in win32utils.py becuase of things like --profile-imports

What I've done is to:

set sys.argv to a unicode copy of the command line, for all patforms, by calling bzrlib.osutils.get_unicode_argv

I've then gone and changed get_uncode_argv (both of them) to return the full command line complete with program name, because it's needed in te bzr script (as above)

So then I've changed _specified_or_unicode_argv to return sys.argv instead of call get_unicode_argv

You'll see from the diffs, but the upshot is, hopefully that sys.argv is unicode from the start. It's a bit 'odd' changing sys.argv I know, but perhaps it's not too hacky and it's done in one place as startup now.

[Gordon Tyler (doxxx)]

The first hunk of changes in win32utils seem superfluous.

[Alexander Belchenko (bialix)]

Jason Spashett пишет:
> I have now re-done this fix, hopefully it's reasonable and should avoide the cases were we had to adjust the command line in win32utils.py becuase of things like --profile-imports
>
> What I've done is to:
>
> set sys.argv to a unicode copy of the command line, for all patforms, by calling bzrlib.osutils.get_unicode_argv
>
> I've then gone and changed get_uncode_argv (both of them) to return the full command line complete with program name, because it's needed in te bzr script (as above)
>
> So then I've changed _specified_or_unicode_argv to return sys.argv instead of call get_unicode_argv
>
> You'll see from the diffs, but the upshot is, hopefully that sys.argv is unicode from the start. It's a bit 'odd' changing sys.argv I know, but perhaps it's not too hacky and it's done in one place as startup now.

You can't import anything from bzrlib before --profile-imports will be
processed. At all. Otherwise the output of --profile-imports will be
incorrect. So this is wrong path.

--
All the dude wanted was his rug back

[Alexander Belchenko (bialix)]

You can't import anything before --profile-imports option will be processed, otherwise its result will be wrong. This is wrong path.

Martin gz explained you earlier what need to be done to avoid sys.argv manipulations when --profile-imports is processed: add --profile-imports to the global bzr options.

review: Disapprove

[Jason Spashett (jspashett)]

I have done what Matring gz explained. Removed the modification of sys.argv from the bzr script
Ignore --profile-imports option in run_bzr

=== modified file 'NEWS'
--- NEWS	2010-06-18 10:40:31 +0000
+++ NEWS	2010-06-18 11:09:29 +0000
@@ -80,6 +80,11 @@
 * ``walkdirs`` now raises a useful message when the filenames are not using
   the filesystem encoding. (Eric Moritz, #488519)
 
+* Enable debugging of bzr on windows with pdb and other tools. This was 
+  broken because we call GetCommandLineW on windows. The fix adjusts the 
+  command line we get to be the same length as sys.argv.
+  (Jason Spashett, Alexander Belchenko, #587868)
+
 Improvements
 ************
 

=== modified file 'bzr'
--- bzr	2010-06-16 12:47:51 +0000
+++ bzr	2010-06-18 11:09:29 +0000
@@ -52,7 +52,6 @@
 
 profiling = False
 if '--profile-imports' in sys.argv:
-    sys.argv.remove('--profile-imports')
     import profile_imports
     profile_imports.install()
     profiling = True

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2010-05-27 21:16:48 +0000
+++ bzrlib/commands.py	2010-06-18 11:09:29 +0000
@@ -1054,6 +1054,8 @@
         elif a == '--coverage':
             opt_coverage_dir = argv[i + 1]
             i += 1
+        elif a == '--profile-imports':
+            pass # already handled in startup script Bug #588277
         elif a.startswith('-D'):
             debug.debug_flags.add(a[2:])
         else:

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2010-06-01 14:24:01 +0000
+++ bzrlib/win32utils.py	2010-06-18 11:09:29 +0000
@@ -535,17 +535,33 @@
                                   default.
     :return: A list of unicode strings.
     """
+    # First, spit the command line
     s = cmdline.Splitter(command_line, single_quotes_allowed=single_quotes_allowed)
-    # Now that we've split the content, expand globs if necessary
+    
+    # Bug #587868 Now make sure that the length of s agrees with sys.argv 
+    # we do this by simply counting the number of arguments in each. The counts should 
+    # agree no matter what encoding sys.argv is in (AFAIK) 
+    # len(arguments) < len(sys.argv) should be an impossibility since python gets 
+    # args from the very same PEB as does GetCommandLineW
+    arguments = list(s)
+    
+    # Now shorten the command line we get from GetCommandLineW to match sys.argv
+    if len(arguments) < len(sys.argv):
+        from bzrlib.errors import InternalBzrError
+        raise InternalBzrError("len(GetCommandLineW(...)) < len(sys.argv) should not be possible")
+    arguments = arguments[len(arguments) - len(sys.argv):]
+    
+    # Carry on to process globs (metachars) in the command line
+    # expand globs if necessary
     # TODO: Use 'globbing' instead of 'glob.glob', this gives us stuff like
     #       '**/' style globs
-    args = []
-    for is_quoted, arg in s:
+    argv = []
+    for is_quoted, arg in arguments:
         if is_quoted or not glob.has_magic(arg):
-            args.append(arg)
+            argv.append(arg)
         else:
-            args.extend(glob_one(arg))
-    return args
+            argv.extend(glob_one(arg))
+    return argv
 
 
 if has_ctypes and winver != 'Windows 98':
@@ -558,20 +574,6 @@
             raise ctypes.WinError()
         # Skip the first argument, since we only care about parameters
         argv = _command_line_to_argv(command_line)[1:]
-        if getattr(sys, 'frozen', None) is None:
-            # Invoked via 'python.exe' which takes the form:
-            #   python.exe [PYTHON_OPTIONS] C:\Path\bzr [BZR_OPTIONS]
-            # we need to get only BZR_OPTIONS part,
-            # We already removed 'python.exe' so we remove everything up to and
-            # including the first non-option ('-') argument.
-            for idx in xrange(len(argv)):
-                if argv[idx][:1] != '-':
-                    break
-            argv = argv[idx+1:]
-        # we should remove '--profile-imports' option as well (bug #588277)
-        # see bzr script ~ line 54
-        if '--profile-imports' in argv and '--profile-imports' not in sys.argv:
-            argv.remove('--profile-imports')
         return argv
 else:
     get_unicode_argv = None