Skip to content
This repository

#1148 (win32 arg_split) #1149

Closed
wants to merge 1 commit into from

4 participants

Min RK Thomas Kluyver Fernando Perez Jörgen Stenarson
Min RK
Owner
  • adds missing strict kwarg to Windows arg_split, which is ignored
  • marks the new test as a known failure on Windows, because is is not a regression, this is just a failure case nobody has reported. win32 argsplit is new since 0.11, so bug is also new.
Thomas Kluyver
Collaborator

The ctypes arg_split was added by PR #1064 to solve #592 (another problem in parsing arguments for magic commands).

Min RK
Owner

My mistake, it looks like the win32 arg_split is new, but doesn't actually behave as expected with respect to quotation marks.

Options:

  1. set win32 arg_split to fallback on _process_common.arg_split() when strict=False
  2. always use _process_common.arg_split() in magics.

Both are ugly, and indicative of the fact that magics should not be treated like regular platform-sensitive command-line args.

Min RK
Owner

Okay, based on readling #592, and #1064, I think we should go with case 1.

Thomas Kluyver
Collaborator

I wonder if it's worth differentiating %run, where the arguments are much more like system command line arguments.

Min RK
Owner

This PR now reflects approach 1. above

Min RK add strict kwarg to win32 arg_split
if strict=False, falls back on Python shlex split
8db0d93
Min RK
Owner

With this commit the new test passes (as does the entire test suite) on my Windows VM. We should be very careful about this, and we should look again at how we parse args for magics so that we aren't running the native command-line parser on arbitrary Python code, trusting it won't be transformed.

Fernando Perez
Owner

@minrk, do you think of this one for merge now or do you want to wait til 0.13?

Thomas Kluyver
Collaborator

I think it'll need to be merged for 0.12. I suspect otherwise several magic commands will be broken on Windows.

Min RK
Owner

definitely required for 0.12. The test suite fails without it.

The implementation as it is, while ugly, does only affect %timeit, as that is the only strict=False case. So if @jstenar's ctypes arg_split works fine in other magics, then this should be good enough for 0.12.

We should definitely look into a better approach to parsing magic arguments for 0.13 that alleviates the convolution of parsing magic flags, filenames, subprocess args, and Python code that we have right now.

Min RK minrk referenced this pull request December 13, 2011
Closed

A lot of testerrors #1148

Jörgen Stenarson
Collaborator

The tests run fine for me with this PR.

Fernando Perez
Owner

Ok, all the tests pass OK on my box, but I'm only checking on Ubuntu. I also checked that the most important case, %run, doesn't change how it handles cmd-line arguments by comparing it to plain python:

In [1]: !cat argv.py
#!/usr/bin/env python
import os, sys

print 'argv:', sys.argv
print 'My PID is :', os.getpid()
print 'My name is:', __name__
#print 'My file is:', __file__

In [2]: %run argv.py foo --bar "ab" 'ab' "ab cd" 'ab cd'
argv: ['argv.py', 'foo', '--bar', 'ab', 'ab', 'ab cd', 'ab cd']
My PID is : 10679
My name is: __main__

In [3]: !python argv.py foo --bar "ab" 'ab' "ab cd" 'ab cd'
argv: ['argv.py', 'foo', '--bar', 'ab', 'ab', 'ab cd', 'ab cd']
My PID is : 10686
My name is: __main__

Given the severity of the iproblem, I'd suggest merging this now then. Since it's just one commit but non-fast forward, I suggest a quick rebase before pushing to avoid an unnecessary merge commit.

Min RK minrk closed this pull request from a commit December 13, 2011
Min RK add strict kwarg to win32 arg_split
if strict=False, falls back on shlex-based split from _process_common

closes #1148
closes #1149
26662f4
Min RK minrk closed this in 26662f4 December 13, 2011
Min RK
Owner

pushed (single commit, no merge)

Brian E. Granger ellisonbg referenced this pull request from a commit January 10, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Dec 13, 2011
Min RK add strict kwarg to win32 arg_split
if strict=False, falls back on Python shlex split
8db0d93
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 8 additions and 3 deletions. Show diff stats Hide diff stats

  1. 11  IPython/utils/_process_win32.py
11  IPython/utils/_process_win32.py
@@ -25,7 +25,7 @@
25 25
 from subprocess import STDOUT
26 26
 
27 27
 # our own imports
28  
-from ._process_common import read_no_interrupt, process_handler
  28
+from ._process_common import read_no_interrupt, process_handler, arg_split as py_arg_split
29 29
 from . import py3compat
30 30
 from . import text
31 31
 
@@ -159,15 +159,20 @@ def getoutput(cmd):
159 159
     LocalFree.res_type = HLOCAL
160 160
     LocalFree.arg_types = [HLOCAL]
161 161
     
162  
-    def arg_split(commandline, posix=False):
  162
+    def arg_split(commandline, posix=False, strict=True):
163 163
         """Split a command line's arguments in a shell-like manner.
164 164
 
165 165
         This is a special version for windows that use a ctypes call to CommandLineToArgvW
166 166
         to do the argv splitting. The posix paramter is ignored.
  167
+        
  168
+        If strict=False, process_common.arg_split(...strict=False) is used instead.
167 169
         """
168 170
         #CommandLineToArgvW returns path to executable if called with empty string.
169 171
         if commandline.strip() == "":
170 172
             return []
  173
+        if not strict:
  174
+            # not really a cl-arg, fallback on _process_common
  175
+            return py_arg_split(commandline, posix=posix, strict=strict)
171 176
         argvn = c_int()
172 177
         result_pointer = CommandLineToArgvW(py3compat.cast_unicode(commandline.lstrip()), ctypes.byref(argvn))
173 178
         result_array_type = LPCWSTR * argvn.value
@@ -175,4 +180,4 @@ def arg_split(commandline, posix=False):
175 180
         retval = LocalFree(result_pointer)
176 181
         return result
177 182
 except AttributeError:
178  
-    from ._process_common import arg_split
  183
+    arg_split = py_arg_split
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.