Bug 183605 – [WinCairo] Fix incompatibility of run-webkit-httpd on native Windows.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Basuke Suzuki]

Fix for launch HTTPD from command line on Windows.

[Basuke Suzuki]

Created attachment 335711 [details]
PATCH

[Basuke Suzuki]

Created attachment 335712 [details]
PATCH

[Basuke Suzuki]

Comment on attachment 335712 [details]
PATCH

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

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:53
> +    option_group = optparse.OptionGroup(parser, "Platform options")

This are to accept platform specific options like --wincairo

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:-63
> -    options.platform = None

See below for this change.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:-95
> -            sys.stdout.write(tail.stdout.readline())

Switch to universal way.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:116
> +    """

As noted on this document, Windows cannot handle opened temp file as Unix do. The workaround is to close it immediately it created and use it's filename. At the end, this is almost identical with tempfile.mkstemp.

[Daniel Bates]

Comment on attachment 335712 [details]
PATCH

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

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:67
> +    log_file = temp_log_file(host.platform)

On Windows, we never remove this temporary file.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:102
> +        print("Stopping servers")

Please remove this. I do not see the value in having the tool print what it is doing or going to do. Tools should follow the general Unix tool philosophy of being silent on success and noisy on failiure.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:123
> +def tail(path):

The name of this function is not very descriptive. Maybe last_line_of_file_so_far().

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:124
> +    """Python equivalent of tail"""

If you take my suggestion above then we can remove this comment.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:125
> +    with open(path) as fp:

What does fp stand for? file pointer? Maybe a better name for this local variable would be file.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:128
> +

Please remove this empty line as I do not feel it improves the readability of the code.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:132
> +

Ditto.

[Daniel Bates]

Comment on attachment 335712 [details]
PATCH

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

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:116
>> +    """
> 
> As noted on this document, Windows cannot handle opened temp file as Unix do. The workaround is to close it immediately it created and use it's filename. At the end, this is almost identical with tempfile.mkstemp.

How does this work? I mean, Apache and webkitpy open this file to write logging and read the logged messages, respectively.

[Basuke Suzuki]

Comment on attachment 335712 [details]
PATCH

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

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:67
>> +    log_file = temp_log_file(host.platform)
> 
> On Windows, we never remove this temporary file.

Right. Need to add that.

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:102
>> +        print("Stopping servers")
> 
> Please remove this. I do not see the value in having the tool print what it is doing or going to do. Tools should follow the general Unix tool philosophy of being silent on success and noisy on failiure.

Agreed. The reason I put this is that a user does not notice about their ctrl-c accepted in the command line. This prevent them to wait next ctrl-c which prevent proper closing sequence. But I will send another patch to make that not happen.

>>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:116
>>> +    """
>> 
>> As noted on this document, Windows cannot handle opened temp file as Unix do. The workaround is to close it immediately it created and use it's filename. At the end, this is almost identical with tempfile.mkstemp.
> 
> How does this work? I mean, Apache and webkitpy open this file to write logging and read the logged messages, respectively.

My patch just opens tmp file readonly. No write access. It works, but original doesn't work well.

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:123
>> +def tail(path):
> 
> The name of this function is not very descriptive. Maybe last_line_of_file_so_far().

I think tail is already self descriptive. One clue is original code has used the term tail (of course its a command name). But if I agree with your suggestion, I don't have any strong alternative to yours. I am not good at English at all.

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:125
>> +    with open(path) as fp:
> 
> What does fp stand for? file pointer? Maybe a better name for this local variable would be file.

Ok.

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:128
>> +
> 
> Please remove this empty line as I do not feel it improves the readability of the code.

Right.

[Stephan Szabo]

For the function name, maybe follow_file or something more like that? last_line_of_file_so_far doesn't seem like a name for a function that would wait if there's not another new line, it seems like it'd return the last line it had read whether or not it's returned it before.

[Basuke Suzuki]

>        for line in follow_file(log_file):
>            stdout.write(line)

It looks meaningful on actual code. I like this.

[Basuke Suzuki]

Created attachment 335786 [details]
FIX

[Basuke Suzuki]

Created attachment 335787 [details]
PATCH

[Basuke Suzuki]

Comment on attachment 335787 [details]
PATCH

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

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:69
> +        run_server_with_log_file(host, options, stdout, stderr, log_file)

This ensures deletion of log_file. I created another function 'run_server_with_log_file' not to indent entire function body only for this purpose.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:130
> +    with open(path) as file_handle:

file is python's built-in function name. I don't like conflicting with that.

[EWS Watchlist]

Comment on attachment 335787 [details]
PATCH

Attachment 335787 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6954471

New failing tests:
http/tests/preload/download_resources.html

[EWS Watchlist]

Created attachment 335799 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Created attachment 335893 [details]
PATCH

[Daniel Bates]

Comment on attachment 335893 [details]
PATCH

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

OK.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:126
> +def temp_log_file(platform):
> +    """Workaround for Windows.
> +    From the document: https://docs.python.org/2/library/tempfile.html#module-tempfile
> +    | Whether the name can be used to open the file a second time, while the named temporary
> +    | file is still open, varies across platforms (it can be so used on Unix; it cannot on
> +    | Windows NT or later).
> +    """
> +    temp_file = tempfile.NamedTemporaryFile()
> +    if platform.is_win():
> +        temp_file.close()
> +    yield temp_file.name
> +

I take it the issue is that Windows only allows one process to have write access to file? Without your patch, Python has write access to the temp file and Apache wants to have write access to the same file. With your patch, Python only has read access to the temp file (since the Python builtin open() opens the specified file for read access when the mode argument is omitted) and hence Apache can acquire write access to the temp file.

[Basuke Suzuki]

a = tempfile.NamedTemporaryFile(delete=False)
file(a.name, 'r')
#>>> <open file 'c:\\users\\basuke\\appdata\\local\\temp\\tmpdclqbx', mode 'r' at 0x0000000009290DB0>

a = tempfile.NamedTemporaryFile(delete=True) # default
file(a.name, 'r')
#---------------------------------------------------------------------------
#IOError                                   Traceback (most recent call last)
#<ipython-input-35-1cb82d16d8f2> in <module>()
#----> 1 file(a.name, 'r')
#IOError: [Errno 13] Permission denied: 'c:\\users\\basuke\\appdata\\local\\temp\\tmpssixrx'

It figured out that when Windows open named temp file, python library attache the flag "O_TEMPORARY", which means "delete on close". With this flag, we cannot open same file again.

[Basuke Suzuki]

See python's implementation here:
https://github.com/python/cpython/blob/2.7/Lib/tempfile.py#L473

Also it is documented at here:
https://docs.python.org/2/library/tempfile.html#tempfile.TemporaryFile

[Daniel Bates]

Comment on attachment 335893 [details]
PATCH

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

>> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:126
>> +
> 
> I take it the issue is that Windows only allows one process to have write access to file? Without your patch, Python has write access to the temp file and Apache wants to have write access to the same file. With your patch, Python only has read access to the temp file (since the Python builtin open() opens the specified file for read access when the mode argument is omitted) and hence Apache can acquire write access to the temp file.

Disregard this remark.

[Daniel Bates]

Comment on attachment 335893 [details]
PATCH

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

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:124
> +        temp_file.close()

We are underutilizing the purpose of tempfile.NamedTemporaryFile() by closing it. In particular, the only benefit we are gaining here is that tempfile.NamedTemporaryFile() picks a path inside the system's temporary directory. However, this path could be reused by the OS between the time when we delete the file (implicitly when we call close()) and when Apache tries to open the file for write. Moreover, we never unlink the file after Apache is done. I suggest that we make use of FileSystem to create a temporary directory then create a normal file inside that directory and give the path to that file to Apache and tail.

[Basuke Suzuki]

Created attachment 336083 [details]
PATCH

Stop using python's native tempfile.NamedTemporaryFile because of the difference of Windows behavior. Use filesystem.mkdtemp() instead.

[Basuke Suzuki]

Comment on attachment 336083 [details]
PATCH

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:-194
> -                    os.rmdir(self._directory_path)

This fails when directory is not empty. That's not the purpose of this code.

[Daniel Bates]

Comment on attachment 336083 [details]
PATCH

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

> Tools/ChangeLog:8
> +        To run Apapche HTTPD from the native Windows command line.

Apapche => Apache

This description and the bug title are insufficient. Please explain what the incompatibility is and what is the fix. Even better, it should explain the difference between native Windows and Cygwin that necessitated making this change.

>> Tools/Scripts/webkitpy/common/system/filesystem.py:-194
>> -                    os.rmdir(self._directory_path)
> 
> This fails when directory is not empty. That's not the purpose of this code.

That is the purpose of this code: to fail when the directory is not empty. This purpose was questioned in the FIXME above it. Your proposed change signifies that we are answering the question posed in the FIXME to the effect of "Yes, we should delete non-empty directories." Therefore we should remove the FIXME comment.

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:114
> +    """Python equivalent of tail"""

tail => tail(1)

OR to be formal TAIL(1). Either notation helps convey that we are referring to the tail command line tool whose man page is in section 1.

[Daniel Bates]

Comment on attachment 336083 [details]
PATCH

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

> Tools/Scripts/webkitpy/layout_tests/servers/run_webkit_httpd.py:115
> +    with open(path) as fp:

fp => file

As per the WebKit Code Style Guidelines please do not use abbreviations unless they are more canonical. I do not find it canonical to think of this a file pointer/handle. I find it canonical to think of this a file.

[Basuke Suzuki]

Created attachment 336144 [details]
FIX

Thanks for r+. > Daniel

I chose `file_handle` instead of your suggestion `file` because it conflict with python's native `file` function. Although we can do it, Python linter keeps warning me and it is uncomfortable. I think `file_handle` is long, but it makes sense.

[Basuke Suzuki]

Created attachment 336168 [details]
FIX ChangeLog with better description

[WebKit Commit Bot]

Comment on attachment 336168 [details]
FIX ChangeLog with better description

Clearing flags on attachment: 336168

Committed r229781: <https://trac.webkit.org/changeset/229781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38688723>