Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Win32 shell interactivity apparently broke qtconsole "cd" magic #1632

Closed
jdmarch opened this issue Apr 19, 2012 · 18 comments · Fixed by enthought/ipython#1 or #1902
Closed

Win32 shell interactivity apparently broke qtconsole "cd" magic #1632

jdmarch opened this issue Apr 19, 2012 · 18 comments · Fixed by enthought/ipython#1 or #1902

Comments

@jdmarch
Copy link

jdmarch commented Apr 19, 2012

PR #1424, Win32 shell interactivity, was accepted as not enhancing qtconsole, but was not thought to break it. It seems to have. This was reproduced on several different systems. Located the breakage by bisection at 4953f2d

@mwiebe, @minrk could you have a look?

On a Win7-64 native box, with no recent history of running "ipython kernel", the following commands immediately after qtconsole startup

ls
cd ..

reliably caused the following traceback, followed by general odd behavior, apparent corruption, until restart qtconsole.

TypeError                                 Traceback (most recent call last)
c:\Users\jmarch\<ipython-input-2-56af63bcfcf6> in <module>()
----> 1 get_ipython().magic(u'cd ..')

c:\users\jmarch\ipython\IPython\core\interactiveshell.pyc in magic(self, arg_s, next_input)
   2034                 self._magic_locals = sys._getframe(1).f_locals
   2035             with self.builtin_trap:
-> 2036                 result = fn(magic_args)
   2037             # Ensure we're not keeping object references around:
   2038             self._magic_locals = {}

c:\users\jmarch\ipython\IPython\core\magic.pyc in magic_cd(self, parameter_s)
   2953             # for c:\windows\directory\names\
   2954             parameter_s = re.sub(r'\\(?! )','/', parameter_s)
-> 2955             opts,ps = self.parse_options(parameter_s,'qb',mode='string')
   2956         # jump to previous
   2957         if ps == '-':

c:\users\jmarch\ipython\IPython\core\magic.pyc in parse_options(self, arg_str, opt_str, *long_opts, **kw)
    279             # If the list of inputs only has 0 or 1 thing in it, there's no
    280             # need to look for options
--> 281             argv = arg_split(arg_str, posix, strict)
    282             # Do regular option processing
    283             try:

c:\users\jmarch\ipython\IPython\utils\_process_win32.pyc in arg_split(commandline, posix, strict)
    178         result_pointer = CommandLineToArgvW(py3compat.cast_unicode(commandline.lstrip()), ctypes.byref(argvn))
    179         result_array_type = LPCWSTR * argvn.value
--> 180         result = [arg for arg in result_array_type.from_address(result_pointer)]
    181         retval = LocalFree(result_pointer)
    182         return result

TypeError: integer expected

After running "ipython kernel" to test this on a 2-process terminal, I was unable to again reproduce the problem on this system even in a simple qtconsole. Have not yet rebooted.

Easiest short-term workaround may be to revert to previous behavior in _process_win32.system when running qtconsole?

@fperez
Copy link
Member

fperez commented Apr 19, 2012

I can't test this right now, but I'm raising it to blocker for 0.13, b/c we can't really release with the kind of breakage described here...

@takluyver
Copy link
Member

FWIW, the same error appears in a test result on Windows: https://jenkins.shiningpanda.com/ipython/job/ipython-windows-py27/lastCompletedBuild/testReport/IPython.utils.tests/test_process/test_arg_split_win32/

I think the arg_split code that calls ctypes was @jstenar's, so he might have some idea why it's failing.

@jdmarch
Copy link
Author

jdmarch commented Apr 19, 2012

Update: breakage reappears after reboot, and continues after a small amount of 2-process terminal activity.

@mwiebe
Copy link
Contributor

mwiebe commented Apr 19, 2012

I tested this on the build I had, and couldn't reproduce it. Updating to the latest master gives me that exact error, though. My previous build had the shell interactivity patch in it, so I would have expected to have the error already without updating.

@jdmarch
Copy link
Author

jdmarch commented Apr 19, 2012

The same commit (4953f2d) also broke the "ls" command from the frontend of a 2-process terminal session in Windows

$ ipython kernel
...
$ ipython console --existing
> ls

After the frontend prints the listing, it does not return to a prompt, and becomes unresponsive except to console-killing Ctrl+C. (The kernel seems to freeze).

@minrk
Copy link
Member

minrk commented Apr 19, 2012

I can reproduce this as well, only when the kernel is started with ipython kernel. I cannot reproduce it with regular ipython [qt]console, or even using --existing to connect to another kernel, if that kernel was started with an attached frontend.

I haven't the foggiest idea how or why this could be.

If we don't figure out a real solution by the time we want to release, then we can certainly just use the old version in the kernel as a band-aid.

@takluyver
Copy link
Member

Since the error is appearing in code that's used to parse a command line, I wonder if the process's own command line could be interfering with it. Then any difference between manually starting ipython kernel and the mechanism the Qt console uses to start it in a subprocess could come into play.

@minrk
Copy link
Member

minrk commented Apr 19, 2012

Good point, thanks.

@minrk
Copy link
Member

minrk commented Apr 19, 2012

Possibly related, I see the following test now hanging on Windows:

IPython.utils.tests.test_process.SubProcessTestCase.test_system

@mwiebe
Copy link
Contributor

mwiebe commented Apr 19, 2012

Here's a minimal patch to revert the functionality. I suspect the issues have to do with the fact that there isn't a good cross-platform way to asynchronously handle stdin/stdout. This particular implementation was working ok in all the testing I did, but I guess there's more depth to the issue that needs to be explored.

diff --git a/IPython/utils/_process_win32.py b/IPython/utils/_process_win32.py
index 7ee9b62..85ee0d0 100644
--- a/IPython/utils/_process_win32.py
+++ b/IPython/utils/_process_win32.py
@@ -124,8 +124,13 @@ def system(cmd):
     """
     # The controller provides interactivity with both
     # stdin and stdout
-    import _process_win32_controller
-    _process_win32_controller.system(cmd)
+    #import _process_win32_controller
+    #_process_win32_controller.system(cmd)
+
+    with AvoidUNCPath() as path:
+        if path is not None:
+            cmd = '"pushd %s &&"%s' % (path, cmd)
+        return process_handler(cmd, _system_body)

jdmarch pushed a commit to jdmarch/ipython that referenced this issue Apr 26, 2012
Win32 shell interactivity broke qtconsole cd magic
@minrk
Copy link
Member

minrk commented May 7, 2012

Can @jdmarch or @mwiebe or other Windows users confirm that PR #1709 fixes this?

@jdmarch
Copy link
Author

jdmarch commented May 8, 2012

On Win7, PR gh-1709 does fix the initially reported issue (with qtconsole).

It largely fixes the 2-process issue reported above at #1632 (comment). Specifically: after the output from ls, it is still the case that no input prompt is printed. But now, if the user presses "Enter" (which is a common response to not seeing a prompt), then the prompt will be printed. Before the break at 4953f2d, the prompt was printed immediately after the ls output, without a need to press Enter.

@jstenar
Copy link
Member

jstenar commented May 8, 2012

I believe there are two separate issues here. #1708 fixes the first issue (related to the exception caused by the ls cd .. combination). I suspect the second issue also causes the testsuite to freeze and require an explicit enter keypress on two of the tests (see relevant failure below). The freezing is also introduced by 4953f2d

======================================================================
FAIL: test_system (IPython.utils.tests.test_process.SubProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\python\ipydevel\VENV\py27\lib\site-packages\ipython-0.13.dev-py2.7.egg\IPython\utils\tests\test_process.py", line 106, in test_system
    self.assertEquals(status, 0)
AssertionError: None != 0
    'None != 0' = '%s != %s' % (safe_repr(None), safe_repr(0))
    'None != 0' = self._formatMessage('None != 0', 'None != 0')
>>  raise self.failureException('None != 0')

-------------------- >> begin captured stdout << ---------------------
on stdouton stderr

--------------------- >> end captured stdout << ----------------------

======================================================================
FAIL: test_system_quotes (IPython.utils.tests.test_process.SubProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\python\ipydevel\VENV\py27\lib\site-packages\ipython-0.13.dev-py2.7.egg\IPython\utils\tests\test_process.py", line 110, in test_system_quotes
    self.assertEquals(status, 0)
AssertionError: None != 0
    'None != 0' = '%s != %s' % (safe_repr(None), safe_repr(0))
    'None != 0' = self._formatMessage('None != 0', 'None != 0')
>>  raise self.failureException('None != 0')

-------------------- >> begin captured stdout << ---------------------


--------------------- >> end captured stdout << ----------------------

@minrk
Copy link
Member

minrk commented May 9, 2012

Thanks for the clarification. Do you have an idea for the cause of the required keypress? If that's reasonably apparent, let's add that to #1709, otherwise we can just merge that now, and leave this open pending a fix for #1632 §2.

@jstenar
Copy link
Member

jstenar commented May 9, 2012

Min RK skrev 2012-05-09 19:53:

Thanks for the clarification. Do you have an idea for the cause of the required keypress? If that's reasonably apparent, let's add that to #1709, otherwise we can just merge that now, and leave this open pending a fix for #1632 §2.

I have not looked at the code, and have no idea about what it could be.

@minrk
Copy link
Member

minrk commented May 10, 2012

Okay, thanks for clarifying. I will go ahead and merge #1709, and leave this open until the key-entry bug is fixed.

@fperez
Copy link
Member

fperez commented Jun 11, 2012

@jdmarch, any ideas on how we can make progress on this, and how serious it continues to be? It's marked as a release blocker, so I want to make sure we can really either fix it, or reassess whether to block the whole project on it.

fperez added a commit that referenced this issue Jun 11, 2012
Workaround fix for gh-1632, using a minimal revert of gh-1424.

Win32 shell interactivity broke qtconsole cd magic, this restores it.

closes #1632
@jdmarch
Copy link
Author

jdmarch commented Jun 11, 2012

Contrary to my Win7 observation above, the full original 2-process issue still exists in WinXP (no Win7 available here): freezes after ls, and a keypress no longer restores the prompt. PR #1902 (from the patch above by @mwiebe) fixes this. With this PR, I think this is not a blocker.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
Win32 shell interactivity broke qtconsole cd magic
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
Workaround fix for ipythongh-1632, using a minimal revert of ipythongh-1424.

Win32 shell interactivity broke qtconsole cd magic, this restores it.

closes ipython#1632
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants