[webkit-reviews] review granted: [Bug 48883] executive_unittest relies on echo and cat utilities from coreutils, which are not present on Windows : [Attachment 72942] Incorporates Eric's comments

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Roben (aroben)
<aroben at apple.com>'s request for review:
Bug 48883: executive_unittest relies on echo and cat utilities from coreutils,
which are not present on Windows
https://bugs.webkit.org/show_bug.cgi?id=48883

Attachment 72942: Incorporates Eric's comments
https://bugs.webkit.org/attachment.cgi?id=72942&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72942&action=review

Looks good.  You'll want to fix the style before landing.

> WebKitTools/Scripts/webkitpy/common/system/fileutils.py:33
> +def make_stdout_binary():
> +    """Puts sys.stdout into binary mode (on platforms that have a
distinction
> +    between text and binary mode)."""
> +    if sys.platform != 'win32' or not hasattr(sys.stdout, 'fileno'):
> +	   return
> +    import msvcrt
> +    import os
> +    msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY)

Do we need a way to reset out of binary mode after unit tests?	Is it a concern
that binary mode may bleed between unit tests?	I guess if we're using
OutputCapture it's not?

> WebKitTools/Scripts/webkitpy/test/cat.py:34
> +    if sys.platform == 'win32' and hasattr(sys.stdout, 'fileno'):
> +	   import msvcrt
> +	   import os
> +	   msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY)

So you have a make_stdout_binary call but you're not using it here. :)

> WebKitTools/Scripts/webkitpy/test/echo.py:27
> +# Add WebKitTools/Scripts to the path to ensure we can find webkitpy.
> +sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))


Why does echo need this and webkitpy does not?