Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

use os.system for shell.system in Terminal frontend #459

Closed
wants to merge 3 commits into from

3 participants

@minrk
Owner

See #457 and #297 for why using pexpect for shell escape is bad in a terminal. Those should be closed when this is merged.

This commit:
splits shell.system into shell.raw_system/piped_system, per @takluyver's recommendation.

raw_system calls os.system, piped_system uses pexpect/utils.platform magic

Defaults:
shell.system=raw_system in Terminal except on Windows and in tests
shell.system=piped_system elsewhere

There was also a small bug I found/fixed along the way involving trailing newlines and prefilter_line, where doing !(true) would raise a SyntaxError, because the transformed line would be:

get_ipython().system(u"(true)
")

The bug has been fixed, and a test included.

@takluyver
Owner

That all looks good, and it works for me. The only tiny quibble is that I think the newline should be sorted out within PrefilterManager, rather than in run_cell.

@minrk
Owner

That makes sense. However, when I made the changes, prefilter_lines always ending in a newline seems to screw with the doctesting mechanisms. That also indicates that run_cell is not the only code that calls prefilter_lines, and run_cell is the only case where a trailing newline is needed (or even desired), due to ast.parse (on 2.6 only, if I recall).

@takluyver
Owner

OK, then I'm happy with what we've got.

IPython/core/interactiveshell.py
((28 lines not shown))
raise OSError("Background processes not supported.")
-
- return system(self.var_expand(cmd, depth=2))
+
+ # don't return the result, always return None
@fperez Owner
fperez added a note

why not return the results? Is it just to avoid the Out[] prompts?

If so, it should be explained in the comment, right now the comment just repeats what the code does but doesn't give any insight as to why.

@minrk Owner
minrk added a note

Yes, that's the only reason. I'll copy the note from the subcommand here, which this is meant to match. That said, I have encountered a number of people who would prefer the exit code to be returned.

@takluyver Owner

Would it make sense to return the exit code only if it's non-zero? So in the standard "everything worked" case, there's no Out[n] prompt.

@fperez Owner
fperez added a note
@minrk Owner
minrk added a note

I think that makes sense, though it should probably be in user_ns and not builtins, as there are strong desires to move away from the singleton nature of InteractiveShell.

The bash name is $?, which obviously doesn't work. I would lean towards _exit_code.

@takluyver Owner

I'll second _exit_code as a clear way to describe it.

@fperez Owner
fperez added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/core/interactiveshell.py
((28 lines not shown))
raise OSError("Background processes not supported.")
-
- return system(self.var_expand(cmd, depth=2))
+
+ # don't return the result, always return None
+ system(self.var_expand(cmd, depth=2))
+
+ def raw_system(self, cmd):
+ """Call the given cmd in a subprocess using os.system
+
+ Parameters
+ ----------
+ cmd : str
+ Command to execute.
+ """
+ # don't return the result, always return None
@fperez Owner
fperez added a note

same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez
Owner

Other than my very minor comments above, this looks good, and does fix the behavior in the way we need it.

I suspect there are still some corner cases we may need to provide solutions for. For example right now if users type !nano in the Qt console, they get a gory mess without any easy way to unwedge themselves.

But that's a different story, not something to hold this PR on.

Once the above is handled, merge away! Thanks for the great work!

ps - don't forget to close #297 if none of the commit messages will auto-close it.

@minrk
Owner

Okay, I've added it. I'll just make sure I didn't break anything on Windows, then merge.

@fperez
Owner

Awesome. BTW, #110 should then be closed when this is merged (or at least we should indicate that for shell calls, it's done, it still leaves the question open for magics).

minrk added some commits
@minrk minrk fix SyntaxError on !(command)
prefilter_line expects that trailing newlines will be trimmed, but
run_cell passed it lines with a trailing \n.

Single-line run_cell now uses prefilter_lines again, and appends newline explicitly.

Formerly failing test included.
7462a57
@minrk minrk split shell.system into shell.system_raw/system_piped
system_raw calls os.system, system_piped uses pexpect/utils.platform magic

use system_raw in Terminal except on Windows and in tests
use system_piped elsewhere

closes gh-297
closes gh-457
bdb012c
@minrk minrk store exit code in user_ns['_exit_code']
_exit_code is an equivalent to `$?` in bash.
It always has the most recent exit value of a shell.system
subprocess.
49cc4a9
@minrk minrk referenced this pull request from a commit
@minrk minrk Merge PR #459 (issue 297)
closes gh-110
closes gh-459
6c66e2b
@minrk minrk closed this pull request from a commit
@minrk minrk Merge PR #459 (issue 297)
closes gh-110
closes gh-459
6c66e2b
@minrk minrk closed this in 6c66e2b
@ellisonbg ellisonbg referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@damianavila damianavila referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mattvonrocketstein mattvonrocketstein referenced this pull request from a commit in mattvonrocketstein/ipython
@minrk minrk Merge PR #459 (issue 297)
closes gh-110
closes gh-459
fd0222e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 27, 2011
  1. @minrk

    fix SyntaxError on !(command)

    minrk authored
    prefilter_line expects that trailing newlines will be trimmed, but
    run_cell passed it lines with a trailing \n.
    
    Single-line run_cell now uses prefilter_lines again, and appends newline explicitly.
    
    Formerly failing test included.
  2. @minrk

    split shell.system into shell.system_raw/system_piped

    minrk authored
    system_raw calls os.system, system_piped uses pexpect/utils.platform magic
    
    use system_raw in Terminal except on Windows and in tests
    use system_piped elsewhere
    
    closes gh-297
    closes gh-457
  3. @minrk

    store exit code in user_ns['_exit_code']

    minrk authored
    _exit_code is an equivalent to `$?` in bash.
    It always has the most recent exit value of a shell.system
    subprocess.
This page is out of date. Refresh to see the latest.
View
49 IPython/core/interactiveshell.py
@@ -1872,22 +1872,44 @@ def define_macro(self, name, themacro):
# Things related to the running of system commands
#-------------------------------------------------------------------------
- def system(self, cmd):
- """Call the given cmd in a subprocess.
+ def system_piped(self, cmd):
+ """Call the given cmd in a subprocess, piping stdout/err
Parameters
----------
cmd : str
- Command to execute (can not end in '&', as bacground processes are
- not supported.
+ Command to execute (can not end in '&', as background processes are
+ not supported. Should not be a command that expects input
+ other than simple text.
"""
- # We do not support backgrounding processes because we either use
- # pexpect or pipes to read from. Users can always just call
- # os.system() if they really want a background process.
- if cmd.endswith('&'):
+ if cmd.rstrip().endswith('&'):
+ # this is *far* from a rigorous test
+ # We do not support backgrounding processes because we either use
+ # pexpect or pipes to read from. Users can always just call
+ # os.system() or use ip.system=ip.system_raw
+ # if they really want a background process.
raise OSError("Background processes not supported.")
-
- return system(self.var_expand(cmd, depth=2))
+
+ # we explicitly do NOT return the subprocess status code, because
+ # a non-None value would trigger :func:`sys.displayhook` calls.
+ # Instead, we store the exit_code in user_ns.
+ self.user_ns['_exit_code'] = system(self.var_expand(cmd, depth=2))
+
+ def system_raw(self, cmd):
+ """Call the given cmd in a subprocess using os.system
+
+ Parameters
+ ----------
+ cmd : str
+ Command to execute.
+ """
+ # We explicitly do NOT return the subprocess status code, because
+ # a non-None value would trigger :func:`sys.displayhook` calls.
+ # Instead, we store the exit_code in user_ns.
+ self.user_ns['_exit_code'] = os.system(self.var_expand(cmd, depth=2))
+
+ # use piped system by default, because it is better behaved
+ system = system_piped
def getoutput(self, cmd, split=True):
"""Get output (possibly including stderr) from a subprocess.
@@ -1905,7 +1927,8 @@ def getoutput(self, cmd, split=True):
manipulation of line-based output. You can use '?' on them for
details.
"""
- if cmd.endswith('&'):
+ if cmd.rstrip().endswith('&'):
+ # this is *far* from a rigorous test
raise OSError("Background processes not supported.")
out = getoutput(self.var_expand(cmd, depth=2))
if split:
@@ -2172,7 +2195,9 @@ def run_cell(self, raw_cell, store_history=True):
prefilter_failed = False
if len(cell.splitlines()) == 1:
try:
- cell = self.prefilter_manager.prefilter_line(cell)
+ # use prefilter_lines to handle trailing newlines
+ # restore trailing newline for ast.parse
+ cell = self.prefilter_manager.prefilter_lines(cell) + '\n'
except AliasError as e:
error(e)
prefilter_failed=True
View
7 IPython/core/tests/test_interactiveshell.py
@@ -106,4 +106,11 @@ def test_alias_crash(self):
err = io.stderr.getvalue()
io.stderr = save_err
self.assertEquals(err.split(':')[0], 'ERROR')
+
+ def test_trailing_newline(self):
+ """test that running !(command) does not raise a SyntaxError"""
+ ip = get_ipython()
+ ip.run_cell('!(true)\n', False)
+ ip.run_cell('!(true)\n\n\n', False)
+
View
6 IPython/frontend/terminal/interactiveshell.py
@@ -86,6 +86,12 @@ def __init__(self, config=None, ipython_dir=None, user_ns=None,
config=config, ipython_dir=ipython_dir, user_ns=user_ns,
user_global_ns=user_global_ns, custom_exceptions=custom_exceptions
)
+ # use os.system instead of utils.process.system by default, except on Windows
+ if os.name == 'nt':
+ self.system = self.system_piped
+ else:
+ self.system = self.system_raw
+
self.init_term_title()
self.init_usage(usage)
self.init_banner(banner1, banner2, display_banner)
View
5 IPython/utils/_process_posix.py
@@ -130,9 +130,7 @@ def system(self, cmd):
Returns
-------
- None : we explicitly do NOT return the subprocess status code, as this
- utility is meant to be used extensively in IPython, where any return
- value would trigger :func:`sys.displayhook` calls.
+ int : child's exitstatus
"""
pcmd = self._make_cmd(cmd)
# Patterns to match on the output, for pexpect. We read input and
@@ -181,6 +179,7 @@ def system(self, cmd):
finally:
# Ensure the subprocess really is terminated
child.terminate(force=True)
+ return child.exitstatus
def _make_cmd(self, cmd):
return '%s -c "%s"' % (self.sh, cmd)
View
5 IPython/utils/_process_win32.py
@@ -96,6 +96,9 @@ def _system_body(p):
line = line.decode(enc, 'replace')
print(line, file=sys.stderr)
+ # Wait to finish for returncode
+ return p.wait()
+
def system(cmd):
"""Win32 version of os.system() that works with network shares.
@@ -116,7 +119,7 @@ def system(cmd):
with AvoidUNCPath() as path:
if path is not None:
cmd = '"pushd %s &&"%s' % (path, cmd)
- process_handler(cmd, _system_body)
+ return process_handler(cmd, _system_body)
def getoutput(cmd):
Something went wrong with that request. Please try again.