run(..., pty=True) no worky when invoke invoker is not a pty #219

Closed
bitprophet opened this Issue Mar 10, 2015 · 3 comments

Projects

None yet

1 participant

@bitprophet
Member

Example case is running Invoke's own test runner, inv test (which does run('spec', pty=True)) within a Git pre-push hook. Git runs the hook script in some non-pty environment (even if one is running git from a terminal window) and so pexpect's attempt to do things like tty.tcgetattr(self.STDIN_FILENO) explodes with termios.error: (25, 'Inappropriate ioctl for device').

Sure enough, examining that fileno value when running by hand vs hook script shows that os.isatty(self.STDIN_FILENO) is False when things blow up.

I can see two ways to handle this:

  • Simply print a more useful error message (either raising a custom exception or reraising the real exception), e.g. "You seem to be running a command with pty=True when you don't actually have a PTY of your own! Please change pty=False or don't run this task headless."
    • Pretty easy to do.
    • Not very friendly; I expect most places where this error pops up would be, well, exactly what I just ran into: a command normally ran by hand which is being run by an automated system instead.
  • Fall back to pty=False behavior when os.isatty(sys.stdin.fileno()) is False, i.e. do best-effort re: running the requested command.
    • In at least some use cases, commands that normally "want" a PTY will still run without one, just with different behavior or uglier output, so we're better off than before.
    • In the other cases, there's simply no way the subcommand could ever have possibly worked (requires a real, true PTY present at top of invocation chain) so failure is still failure and we're no worse off.
      • IT could be confusing, however (since the underlying command's non-pty behavior may be subtle or unexpected, compared to how it works under a pty) so we should probably emit a warning?
@bitprophet
Member

While I have some fears about argument bloat, I could see situations where the isatty test isn't foolproof and users might want to force use of a pty when it says False. So I'm also adding a fallback kwarg which overrides the fallback behavior being added. It defaults to True so the fallback is still default behavior for most users (otherwise there'd be little point).

@bitprophet
Member

Got the basics for this working, then realized we had way too much junk stuck in Local.run that would eventually need applying to other subclasses. Currently in the middle of a big stinkin' reorg.

Once that is back to full operation I need to:

  • go and smooth out the encoding parameter added in a recent merge
  • tidy up the docstring for what is now Runner.run so it uses :param:
  • update the changelog entry to reflect the bigger changes (incl some backwards incompat for folks using the API directly - even tho invoke.run should not have changed a ton).
@bitprophet bitprophet added a commit that referenced this issue Mar 11, 2015
@bitprophet bitprophet Implement #219, pty fallback 40cb187
@bitprophet bitprophet removed the Needs patch label Mar 11, 2015
@bitprophet
Member

All done. That was a nice detour...

@bitprophet bitprophet closed this Mar 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment