Summary: | [WinCairo] Fix incompatibility of run-webkit-httpd on native Windows. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aakash_jain, Basuke.Suzuki, commit-queue, dbates, ddavidso, ews-watchlist, glenn, jbedard, lforschler, ross.kirsling, stephan.szabo, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Basuke Suzuki
2018-03-13 12:21:41 PDT
Created attachment 335711 [details]
PATCH
Created attachment 335712 [details]
PATCH
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. 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. 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. 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. 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. > for line in follow_file(log_file):
> stdout.write(line)
It looks meaningful on actual code. I like this.
Created attachment 335786 [details]
FIX
Created attachment 335787 [details]
PATCH
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. 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 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
Created attachment 335893 [details]
PATCH
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. 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. 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 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. 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. Created attachment 336083 [details]
PATCH
Stop using python's native tempfile.NamedTemporaryFile because of the difference of Windows behavior. Use filesystem.mkdtemp() instead.
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. 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. 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. 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.
Created attachment 336168 [details]
FIX ChangeLog with better description
Comment on attachment 336168 [details] FIX ChangeLog with better description Clearing flags on attachment: 336168 Committed r229781: <https://trac.webkit.org/changeset/229781> All reviewed patches have been landed. Closing bug. |