Command output not displayed in Windows

[syu-id]

In Windows, command output is not displayed in terminal, even with the simple task below.

@task
def test():
    """output test"""
    run('echo hello invoke')

The --echo command line option also doesn't work in Windows.
Things are fine in Linux though.

Windows 7 32bit, Python 2.7.9, Invoke 0.9.0

[pfmoore]

This is because the monkeypatched subprocess.Popen doesn't support the hide argument on Windows. Making it do so probably requires extra lower-level code, similar to that already implemented for Unix.

[bitprophet]

Yup, what @pfmoore said. I am open to patches, I just can't dev them myself due to Windows aversion :)

[syu-id]

Thank you guys.
It's a pity because echoing the output is such basic feature.
Hope someone would solve this.

[pfmoore]

Honestly, monkeypatching subprocess.Popen seems like a horrible approach. @bitprophet what exactly is that monkeypatching intended to achieve? It may be that there's a 3rd party module that does what you need in a cross-platform way. I'm certainly not willing to dive into the guts of Popen myself - there be dragons :-)

[pfmoore]

@bitprophet AFAICT, the monkeypatching is solely so that runner.run() can support the hide parameter. Would it not be less fragile to just do the select loop directly in runner.run rather than using communicate? That way you wouldn't need the monkeypatching (which, BTW, is duplicating code that has changed between Python 2.7 and 3.4 :-() It'd need to be Unix only, as select doesn't work on stdout and stderr under Windows, but that's fine, just use the communicate approach for now on Windows, it's no worse than currently.

If you did it that way, I could write a PR that used threads to support displaying the output and the hide option for Windows.

Let me know if you want to go down that route. I'm not particularly keen on writing the PR if it's not going to be accepted...

(It might make sense to factor the code out to a function in platform.py, that took 2 file objects and multiplexed the output to stdout/stderr, capturing it as it went past. That would keep the ugly platform specific stuff in one place.)

[pfmoore]

Sigh. I really can't work out what the monkeypatching is doing, it seems to be also making communicate return strings even though universal_newlines isn't set.

I'd love to fix this, but it feels like the existing code is so Windows-hostile that even trying is going to end up in a world of pain :-( So I'm going to leave it, I'm afraid. Ping me if you ever decide to remove the monkeypatching, and I'll take another look.

[pfmoore]

@bitprophet The following implementation of Local.run will work for Windows as well as Unix, and means that the subprocess monkeypatching can be removed. If you want to avoid using threads on Unix, you could either code a select-based approach for Unix and use this for Windows, or just use this for Windows and leave the existing monkeypatching in place for Unix.

Hope this is of use.

from subprocess import Popen, PIPE
import os
import sys
import threading

def run(self, command, warn, hide):
    process = Popen(
        command,
        shell=True,
        stdout=PIPE,
        stderr=PIPE,
    )

    def display(src, dst, cap):
        while True:
            data = os.read(src.fileno(), 1000)
            if not data: break
            os.write(dst.fileno(), data)
            cap.append(data)

    stdout = []
    stderr = []

    threads = []
    if 'out' not in hide:
        t = threading.Thread(target=display, args=(process.stdout, sys.stdout, stdout))
        threads.append(t)
        t.start()
    if 'err' not in hide:
        t = threading.Thread(target=display, args=(process.stderr, sys.stderr, stderr))
        threads.append(t)
        t.start()

    process.wait()
    for t in threads:
        t.join()

    stdout = b''.join(stdout).decode('utf-8', 'replace')
    stderr = b''.join(stderr).decode('utf-8', 'replace')
    return stdout, stderr, process.returncode, None

I've decoded the bytes objects to strings using UTF-8 in the same way the monkeypatching does. I'm not sure that using UTF-8 is right, though - what if the user has a different locale set? It should probably use sys.getfilesystemencoding(). UTF-8 is typically wrong for Windows.

Sorry I've not raised a PR for this, but I don't want to guess which approach you would prefer.

[bitprophet]

Thanks a lot for digging, @pfmoore - I don't remember offhand how necessary the monkeypatching truly is, just that at the time I started poking at things it seemed like the least awful route (which, given how terrible monkeypatching is, implies it seemed like the ONLY route).

I'll definitely look at your suggestions soon to see if past-me was being a dummy, and I also agree that shuffling things around for xplatform compatibility is a good idea, orthogonal to the monkeypatch issue.

[bitprophet]

I poked around and can't find a record of why the subclassing-Popen route seemed superior to doing things manually. Could've just been the first thing I tried that worked :( "Get it working basically, then iterate" has been my overall MO here - which is also why things such as naive approaches to text encoding, are present.

If I slot in your above snippet, many of the tests relating to output hiding & returning fail. I'll dig into it and try to figure out why - it might highlight why the monkeypatch approach was a thing.

[pfmoore]

@bitprophet It's quite possible I got something wrong around the hide options. I didn't really put much effort into that side of it, mostly just made sure that output was displayed and captured at the same time. That's the difference between dumping some sample code here and putting together a proper PR :-)

If I get some spare time (no promises!) I might play with it a bit more. Don't hold your breath, though - I might need to build a Unix box/VM first, though, I don't know if the tests even work on Windows, and I don't want to end up getting distracted trying to make them :-)

[bitprophet]

Oh sure, wasn't blaming you, the tl;dr here is the high level "why did we monkeypatch, is it possible a less dirty approach still works on Unix (and then also makes it easier to work on Windows)" stuff. At the very least I want to come out of this knowing the reasons for the current impl - that'll still be an improvement.

Anyway I'm in the middle of digging so I'll record my findings here & let you know when I have something I want your feedback on :)

Re: tests, until Travis-CI supports Windows (sounds like 'eventually'?) there's no real point mucking with them sadly. Maybe run them to see what that reveals at a high level, but I wouldn't waste time making them all pass if there's nontrivial wacky stuff going on.

[bitprophet]

@pfmoore I've found one wrinkle (beyond functionality updates to fix hide+capture) - was there a particular reason you used os.write(file_obj.fileno(), ...) instead of file_obj.write(...)? This breaks on file-like-but-not-really-file objects, including the ones used for testing purposes.

On my end, when I simply swap those calls with their method counterparts, things still appear to work fine (and the tests then pass). Obviously my worry is that there's a Windows-level distinction I'm not seeing.

Here's a link to highlight the two basic changes I needed to make for things to work on my end: 7f030f5...54a66af

If you grab HEAD of that branch (https://github.com/pyinvoke/invoke/tree/xplatform-friendly-run-201) I'm curious to see if it works for you.

[pfmoore]

The reason I used os.write was simply to match os.read, which was needed because IIRC anything higher level on the read side doesn't grab what's available without blocking. And that's important if you want the output to appear on screen immediately it's available. But if you're happy to read using the low-level function and write using a higher-level one, that should be fine.

One concern I'd have with mixing the levels is that os.read always returns bytes, whereas what file_obj.write() takes depends on whether file_obj was opened in binary mode or not. I've seen a lot of code that seems to work on Python 2, but breaks on Python 3, to like taking that risk :-) I don't know if your primary platform is Python 2 or Python 3 - I use Python 3 almost exclusively (Windows + Python 3, how minority can I get? :-)) so bytes/string confusion is something I'm very conscious of.

Re tests, are you aware of Appveyor? It's similar to Travis, but with Windows builders.

I'll try to get some time to try out that branch over the weekend.