dose icon indicating copy to clipboard operation
dose copied to clipboard

Fix a bug related to fcntl.ioctl in PyCharm/Linux

Open samuelgrigolato opened this issue 8 years ago • 1 comments

When running Dose inside PyCharm in a Linux environment, fnctl.ioctl can't determine the window size (see the method TerminalSize#from_io_control in terminal.py). As a result of this, the application will crash even that retrieving this size isn't required at all. In order to allow Dose to run in this environment, this fix catches the exact OSError raised internally by the affected commands and silently ignore them with a brief warning message.

Note: if you happen to know a better solution to this, please feel free to direct me into another trail. In my tests this is only affecting when running from PyCharm, as intended. The problem only arose when I setup my ArchLinux environment (in Windows, even using PyCharm, I haven't yet experienced this problem).

samuelgrigolato avatar Nov 25 '16 20:11 samuelgrigolato

These methods can only be called on Linux/OSX/Cygwin, not on Windows (there's a comment otherwhere telling so), but the terminal inside PyCharm never behaves like a Linux/OSX/Cygwin terminal.

That's not specific to PyCharm. Its terminal isn't really a terminal, as it doesn't allow any Terminal I/O Control call on its "standard" I/O streams nor on its "tty". The "standard" streams in Spyder share this behavior. Maybe there are other scenarios where that can happen.

The width = method(*args) in the TerminalSize.retrieve_width method should receive 0 or None from some from_* method when it couldn't get the desired size, you're returning None, so this part seems ok. But that can happen all the time, that's why there are several fallback strategies. When a strategy can't get the width, it shouldn't be logged, as the next strategy will be called to do so, that's what I did in the TerminalSize.from_tput_subprocess and this case you're talking about seem analogous.

Don't move other stuff around, I don't know why the fg and log functions were moved but that would make git blame no longer show the commit in history where each of their lines were last modified or added.

A comment about the comments:

    except OSError as e:
        # [Errno 6]: No such device or address: '[STRING]' (occur when
        # running from pycharm console in Linux)
        if e.errno == 6:
            [...]

The bug appears on PyCharm + Linux, but I'm pretty sure the same would happen on [several] other contexts. Also the errno appears in both the comment and the code. Simpler:

    except OSError as e:
        if e.errno == 25: # Inappropriate ioctl for device
            [...]

Or even:

    except OSError: # Inappropriate ioctl for device
        return 0

As there's no need to be so specific about the error number, and the comment is the reason for adding that line there. That's not the only OSError being caught, but we really want it to crash on any other error number? Using a simple catching block on from_tty_io_control is enough to fix the issue on it, but an if would be simpler there: you can get the file "is a tty?" flag calling the isatty method like sys.stdout.isatty() (or fobj.isatty()), it should check if the file/stream is connected to a terminal, and trying to get some terminal I/O control information from a stream that has nothing to do with a terminal really makes no sense.

There are 3 places where some checking is possible:

  1. On every strategy (TerminalSize.from_... method) when something undesired can happen
  2. On the strategy fallback loop (TerminalSize.retrieve_width)
  3. On the strategy list creation (TerminalSize.strategies)

I'm not sure if (1) is better than (2) or (3). Perhaps the best is a combined approach. The width retrieval occurs all the time due to the SIGWINCH signal handling, besides a call to the retrieve_width method on every new test job triggering event, I'm already doing (3) to avoid calling OS-specific calls where they don't belong. Maybe (2) is a better approach for catching an OSError:

for [...]
    [...]
    try:
        width = method(*args)
    except OSError:
        continue
    [...]

If we know we're in PyCharm, perhaps we could create a new PyCharm-specific strategy that gets the column width there, using the (3)rd idea. Does that terminal have a COLUMNS environment variable? How do I get the "pseudo-terminal without a tty" I/O control information in that context? Is there a way to know we're in PyCharm?

About ctermid, it always returns "/dev/tty" here, even when I get it directly from the libc in Spyder:

>>> import ctypes
>>> libc = ctypes.cdll.LoadLibrary("libc.so.6")
>>> buf = ctypes.create_string_buffer(50) # 50 >= 9 and L_ctermid = 9 in bits/stdio_lim.h
>>> number = libc.ctermid(buf)
>>> buf.value # Or buf.raw
'/dev/tty'

Opening a terminal on Spyder, I can see it already starts with a bash: cannot set terminal process group (858): Inappropriate ioctl for device warning. And trying to open /dev/tty gives a No such device or address: '/dev/tty', though os.path.exists("/dev/tty") returns True, I couldn't find a way to ask permission instead of forgiveness on this case, nor a way to get the "terminal" width.

danilobellini avatar Nov 28 '16 05:11 danilobellini