Bug 58708 – new-run-webkit-tests: read stderr from chromium DRT separately

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: read stderr from chromium DRT separately

[Dirk Pranke]

Created attachment 89875 [details]
Patch

[Dirk Pranke]

easily the least portable python I've ever written, but it seems to work ...

[Dirk Pranke]

on unix, it might be better to just mark the socket as non-blocking and do a select(), but the patch is posted for the record :).

[Dirk Pranke]

Created attachment 89880 [details]
use non-blocking sockets on unix instead

[Ojan Vafai]

Comment on attachment 89880 [details]
use non-blocking sockets on unix instead

View in context: https://bugs.webkit.org/attachment.cgi?id=89880&action=review

Looks fine to me. Can't say I really understand what _read_stderr is doing. Maybe tony or someone with my python experience can comment. I don't think we need to block getting this on on that though.

The only issue, I'm pretty sure some tests currently have stderr output in their expected results, so you'll probably need to rebaseline them with this patch.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:578
> +    def _read_stderr(self):
> +        if sys.platform in ('win32', 'cygwin'):
> +            import msvcrt
> +            import win32pipe
> +
> +            fd = self._proc.stderr.fileno()
> +            osf = msvcrt.get_osfhandle(fd)
> +            _, avail, _ = win32pipe.PeekNamedPipe(osf, 0)
> +            if avail:
> +                return self._proc.stderr.read(avail)
> +            return ''
> +        else:
> +            if not self._stderr_is_nonblocking:
> +                import fcntl
> +                import os
> +                fd = self._proc.stderr.fileno()
> +                fl = fcntl.fcntl(fd, fcntl.F_GETFL)
> +                fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NONBLOCK)
> +                self._stderr_is_nonblocking = True
> +            return self._proc.stderr.read()

this doesn't seem chromium specific. don't other ports need this code as well?

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 89880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89880&action=review
> 
> Looks fine to me. Can't say I really understand what _read_stderr is doing. Maybe tony or someone with my python experience can comment. I don't think we need to block getting this on on that though.
> 
> The only issue, I'm pretty sure some tests currently have stderr output in their expected results, so you'll probably need to rebaseline them with this patch.
> 

Yeah, that wouldn't surprise me.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:578
> > +    def _read_stderr(self):
> > +        if sys.platform in ('win32', 'cygwin'):
> > +            import msvcrt
> > +            import win32pipe
> > +
> > +            fd = self._proc.stderr.fileno()
> > +            osf = msvcrt.get_osfhandle(fd)
> > +            _, avail, _ = win32pipe.PeekNamedPipe(osf, 0)
> > +            if avail:
> > +                return self._proc.stderr.read(avail)
> > +            return ''
> > +        else:
> > +            if not self._stderr_is_nonblocking:
> > +                import fcntl
> > +                import os
> > +                fd = self._proc.stderr.fileno()
> > +                fl = fcntl.fcntl(fd, fcntl.F_GETFL)
> > +                fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NONBLOCK)
> > +                self._stderr_is_nonblocking = True
> > +            return self._proc.stderr.read()
> 
> this doesn't seem chromium specific. don't other ports need this code as well?

All of the other ports use the server_process.py code (via port/webkit.py) which uses select and non-blocking reads. As a result we don't have any other ports that will actually work correctly on windows (see bug 38756). As a part of that, we'll likely need to re-use this code there as well in some form.

[Dirk Pranke]

This will probably allow us to figure out whether or not to fix bug 37426, as well.

[Dirk Pranke]

Committed r84111: <http://trac.webkit.org/changeset/84111>

[Tony Chang]

Comment on attachment 89880 [details]
use non-blocking sockets on unix instead

View in context: https://bugs.webkit.org/attachment.cgi?id=89880&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:562
> +            import msvcrt
> +            import win32pipe

Do both of these exist in cygwin python?  I thought cygwin python could import fcntl so maybe it can go through the other code path?

[James Robinson]

This got reverted in http://trac.webkit.org/changeset/84128 but the bug wasn't reopened.  I think this is (at least part of) the problem with https://bugs.webkit.org/show_bug.cgi?id=66806

[James Robinson]

Reopening, patch was reverted

[Dirk Pranke]

closing, this is fixed w/ r115490 and r115903.