Bug 183265

Created attachment 334865 [details] PATCH

Apache is required to run http tests. Use xampp distribution. https://www.apachefriends.org/index.html Or install via chocolatey: > C:\ choco install bitnami-xampp

Created attachment 334869 [details] FIX for webkitpy tests

port_testcase.py is weird. It makes platform object useless because this test depends on the fact code depends on sys.platform. It should be refactored for the future anyway.

Comment on attachment 334869 [details] FIX for webkitpy tests View in context: https://bugs.webkit.org/attachment.cgi?id=334869&action=review I'm just wondering if it would be better to have like an XAMP_ROOT or something like that defined that we could then use. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:1 > +ServerRoot "C:/xampp/apache" Is there any way to avoid hard coding a path? It appears that apache config files can take environment variables from http://httpd.apache.org/docs/current/configuring.html#syntax > Tools/Scripts/webkitpy/port/win.py:468 > + httpdPath = os.path.join('C:\\', 'xampp', 'apache', 'bin', 'httpd.exe') Same

Those are better improvements. I wanna do that modification after this patch landed to probe this working.

Comment on attachment 334869 [details] FIX for webkitpy tests View in context: https://bugs.webkit.org/attachment.cgi?id=334869&action=review > LayoutTests/http/conf/win-httpd-2.4-php7.conf:12 > +# LoadModule unixd_module libexec/apache/mod_unixd.so We shouldn't commit commented out code. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:25 > +#LoadModule imagemap_module modules/mod_imagemap.so Ditto. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:172 > + return [(alias, make_path(path)) for (alias, path) in json.loads(json_data)] This function use to return a list of lists. Now its a list of tuples. Is this OK with respect to the calling code?

Run with Administrator privileges. > $ python .\Tools\Scripts\run-webkit-tests --dump-render-tree --wincairo

Comment on attachment 334869 [details] FIX for webkitpy tests View in context: https://bugs.webkit.org/attachment.cgi?id=334869&action=review >> LayoutTests/http/conf/win-httpd-2.4-php7.conf:12 >> +# LoadModule unixd_module libexec/apache/mod_unixd.so > > We shouldn't commit commented out code. Got it. >> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:172 >> + return [(alias, make_path(path)) for (alias, path) in json.loads(json_data)] > > This function use to return a list of lists. Now its a list of tuples. Is this OK with respect to the calling code? I double check it, but the result was used only to generate a string using format function. Of course it can be list as it was, tuple is better representation for this result and I think this is improvement.

Comment on attachment 334869 [details] FIX for webkitpy tests View in context: https://bugs.webkit.org/attachment.cgi?id=334869&action=review >>> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:172 >>> + return [(alias, make_path(path)) for (alias, path) in json.loads(json_data)] >> >> This function use to return a list of lists. Now its a list of tuples. Is this OK with respect to the calling code? > > I double check it, but the result was used only to generate a string using format function. Of course it can be list as it was, tuple is better representation for this result and I think this is improvement. I've confirmed this was only used in apache_http_server.py and http_server.py and usage is to get pair of string to generate a config string.

Created attachment 335058 [details] fix

Comment on attachment 335058 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=335058&action=review > LayoutTests/ChangeLog:8 > + * http/conf/win-httpd-2.4-php7.conf: Added. Is this basically a copy of the .conf file used by AppleWin, where only the file paths are changed?

(In reply to Per Arne Vollan from comment #12) > Comment on attachment 335058 [details] > fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335058&action=review > > > LayoutTests/ChangeLog:8 > > + * http/conf/win-httpd-2.4-php7.conf: Added. > > Is this basically a copy of the .conf file used by AppleWin, where only the > file paths are changed? The main change is PHP version, from 5 to 7. PHP 5 is almost out of date and support is going to be ended soon. http://php.net/supported-versions.php

Comment on attachment 335058 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=335058&action=review >>> LayoutTests/ChangeLog:8 >>> + * http/conf/win-httpd-2.4-php7.conf: Added. >> >> Is this basically a copy of the .conf file used by AppleWin, where only the file paths are changed? > > The main change is PHP version, from 5 to 7. PHP 5 is almost out of date and support is going to be ended soon. http://php.net/supported-versions.php Not related to your patch. We should look to update the PHP requirement for the Apple Windows port. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:1 > +ServerRoot "C:/xampp/apache" The direction of the slashes in this line do not match the direction of the slashes we used on line 54. I take it we do not need to worry about the direction of the slashes. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:3 > +PidFile "c:/xampp/apache/logs/httpd.pid" Does this assume a case-insensitive file system? I mean, we use a lowercase drive letter in this line (c:) but use an uppercase drive letter everywhere else in this configuration file (e.g. on line 1). The direction of the slashes in this line do not match the direction of the slashes we used on line 54. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:27 > +LoadFile "C:/xampp/php/php7ts.dll" > +LoadFile "C:/xampp/php/libpq.dll" > +LoadModule php7_module "C:/xampp/php/php7apache2_4.dll" The direction of the slashes in this line do not match the direction of the slashes we used on line 54. > LayoutTests/http/conf/win-httpd-2.4-php7.conf:64 > +<IfModule mod_alias.c> > +</IfModule> I know that you copied this over from cygwin-httpd.conf. I suggest we remove this empty directive. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:34 > +import os.path It is unnecessary to import this module. We should use FileSystem.normpath() below. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:170 > + make_path = lambda path: self._filesystem.join(self.tests_dir, os.path.normpath(path)) Notice that the FileSystem module has a normpath function that turns around and calls os.path.normpath(). We should make use of it instead of directly invoking os.path.normpath() so as to support unit testing this code ... > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:172 > def aliases(self): > json_data = self._filesystem.read_text_file(self._port_obj.path_from_webkit_base("Tools", "Scripts", "webkitpy", "layout_tests", "servers", "aliases.json")) > - results = [] > - for item in json.loads(json_data): > - results.append([item[0], self._port_obj._filesystem.join(self.tests_dir, item[1])]) > - return results > + > + make_path = lambda path: self._filesystem.join(self.tests_dir, os.path.normpath(path)) > + > + return [(alias, make_path(path)) for (alias, path) in json.loads(json_data)] Please add unit tests for this function. > Tools/Scripts/webkitpy/port/win.py:468 > + httpdPath = os.path.join('C:\\', 'xampp', 'apache', 'bin', 'httpd.exe') This code is almost identical to the code in WinPort._path_to_apache() except for the drive-prefix portion of the path. Would the change made to this function (to pass os.path.join() "C:\\" as the first argument) break the Apple Windows port? If not, we should share this code. On another note, we should be using FileSystem.join() instead of os.path.join() directly. > Tools/Scripts/webkitpy/port/win.py:471 > + _log.error("Could not find apache. Not installed or unknown path.") " (double quote) => ' (single quote) > Tools/Scripts/webkitpy/port/win.py:480 > + # to launch apache as a daemon, service installation is required. We follow the WebKit Code Style guidelines more or less for Python code and hence comments should be complete sentence that start with a capital letter and end with a period. Please capitalize the 't' in "to". > Tools/Scripts/webkitpy/port/win.py:481 > + code = self._executive.run_command([httpd_path, "-k", "install"], return_exit_code=True) I suggest that we rename "code" to "exit_code" as it better describes the purpose of this value. " (double quote) => ' (single quote) > Tools/Scripts/webkitpy/port/win.py:482 > + # 0 : success, 2 : already installed, 720005 : permission error Are these the only possible exit codes? I mean, the code below assumes that if we do not get 0 or 2 then it has to be a permission error. It simplifies the code to make this assumption as you did. Having said that, if there is a possibility of other exit code values then we may want to consider emitting the exit code value with the logged error below for debugging purposes. > Tools/Scripts/webkitpy/port/win.py:486 > + _log.error("Httpd cannot run as service. perhaps you forgot to log in as Adminstrator?") "as service" => "as a service" "perhaps" => "Perhaps" "as Administrator?" => "as an administrator user?"

Created attachment 335222 [details] PATCH Added unittest, cleanup apache config, refactoring

Comment on attachment 335222 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=335222&action=review > Tools/ChangeLog:7 > + Please describe this change as the title of the bug is not descriptive enough. The purpose of this change is to support running HTTP layout tests on Windows without Cygwin. Currently we only support running HTTP layout tests on Windows in Cygwin. > Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:178 > + @property > + def platform(self): > + return self._port_obj.host.platform Nice. > Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:227 > + def _server_error(self, message, err, exit_code): I know that you named the second argument, err, based on the existing code. It neither conforms to the naming conventions in the WebKit Code Style guidelines nor is is very descriptive. I suggest we rename the argument err to stderr_output (or can we come up with a better name) to describe what it is: the standard error output. > Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:229 > + err = 'Access is denied. Do you have an administrator privilegde?' "an administrator privilegde?" => "administrator privilege?" > Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:230 > + Please remove this empty line as I do not feel it improves the readability of this function. > Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:231 > + return http_server_base.ServerError('%s: %s (exit code=%s)' % (message, err, exit_code)) We prefer using String.format() in new code as this was initially suggested as the analog in Python 3. I am unclear when we will move to Python 3 (if ever) or if String.format() is still preferred in Python 3. If you chose to use the % operator then we should format the exit code as decimal (%d). > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:168 > + Please remove this empty line as I do not feel it improves the readability of this function. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:171 > + def _prepare_aliases(self, data): Maybe a more descriptive name for this would be _build_alias_path_pairs? > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:174 > + Please remove this empty line as I do not feel it improves the readability of this function. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:60 > + def test_aliases(self): I suggest we name this function test_prepare_aliases to coincide with the name of the function it is testing _prepare_aliases. If you take my suggestion above then we should rename this function to test_build_alias_path_pairs. > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:68 > + ["/media-resources", "media"], > + ["/modern-media-controls", "../Source/WebCore/Modules/modern-media-controls"], > + ["/resources/testharness.css", "resources/testharness.css"], " (double quote) => ' (single quote) > Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:74 > + ('/media-resources', '/test.checkout/LayoutTests/media'), > + ('/modern-media-controls', '/test.checkout/LayoutTests/../Source/WebCore/Modules/modern-media-controls'), > + ('/resources/testharness.css', '/test.checkout/LayoutTests/resources/testharness.css'), This output matches the output on Windows? I would have expected the slashes to be different. > Tools/Scripts/webkitpy/port/win.py:187 > + def _expected_path_to_apache(self): > + return self._filesystem.join('C:\\', 'xampp', 'apache', 'bin', 'httpd.exe') I do not see the need for this function because it is referenced exactly once from _path_to_apache() and its name raises more questions than answers ("expected"?). I suggest we inline the implementation of this function into _path_to_apache() and remove this function. > Tools/Scripts/webkitpy/port/win.py:193 > + Please remove this empty line as I do not feel it improves the readability of this function. > Tools/Scripts/webkitpy/port/win.py:479 > + # To launch apache as a daemon, service installation is required. apache => Apache (it's a proper noun) > Tools/Scripts/webkitpy/port/win.py:481 > + # 0=success, 2=already installed, 720005=permission error, etc etc => "etc." (notice the period at the end since etc. is an abbreviation of et cetera).

Created attachment 335390 [details] FIX > Reviewed by Daniel Bates. Thanks.

Comment on attachment 335222 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=335222&action=review >> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:231 >> + return http_server_base.ServerError('%s: %s (exit code=%s)' % (message, err, exit_code)) > > We prefer using String.format() in new code as this was initially suggested as the analog in Python 3. I am unclear when we will move to Python 3 (if ever) or if String.format() is still preferred in Python 3. If you chose to use the % operator then we should format the exit code as decimal (%d). That's wonderful. I prefer format(). >> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:74 >> + ('/resources/testharness.css', '/test.checkout/LayoutTests/resources/testharness.css'), > > This output matches the output on Windows? I would have expected the slashes to be different. Filesystem_mock object prepared for mock port always uses '/' as a separator. This is the case I discussed at IRC. I think the validity of Windows style path style is okay not checking at this test, but it should be covered by filesystem_mock_unitcase. I've started working on that test.

Comment on attachment 335390 [details] FIX Clearing flags on attachment: 335390 Committed r229469: <https://trac.webkit.org/changeset/229469>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38308428>