[webkit-reviews] review denied: [Bug 183265] [webkitpy, WinCairo] Launch Apache HTTPD for HTTP Tests. : [Attachment 335058] fix

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 183265: [webkitpy, WinCairo] Launch Apache HTTPD for HTTP Tests.
https://bugs.webkit.org/show_bug.cgi?id=183265

Attachment 335058: fix

https://bugs.webkit.org/attachment.cgi?id=335058&action=review




--- Comment #14 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 335058
  --> https://bugs.webkit.org/attachment.cgi?id=335058
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?"