checkpoint Blink-side work to use Apache on Windows

Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py

Index: Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py
diff --git a/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py b/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py
index e00a9a0cb449621a3de48b68f0cd8ed625da83e9..0fda84368034e45b4352955f04d59a55d5e45235 100644
--- a/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py
+++ b/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py
@@ -30,10 +30,7 @@
 
 
 import logging
-import os
-import re
 import socket
-import sys
 
 from webkitpy.layout_tests.servers import http_server_base
 
@@ -68,20 +65,24 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
         error_log = self._filesystem.join(output_dir, "error_log.txt")
         document_root = self._filesystem.join(test_dir, "http", "tests")
 
-        # FIXME: We shouldn't be calling a protected method of _port_obj!
-        executable = self._port_obj._path_to_apache()
+        self._is_win = self._port_obj.host.platform.is_win()
+
+        executable = self._port_obj.path_to_apache()
+
+        server_root = self._filesystem.dirname(self._filesystem.dirname(executable))
 
         start_cmd = [executable,
-            '-f', "\"%s\"" % self._get_apache_config_file_path(test_dir, output_dir),
-            '-C', "\'DocumentRoot \"%s\"\'" % document_root,
-            '-c', "\'Alias /js-test-resources \"%s\"'" % js_test_resources_dir,
-            '-c', "\'Alias /media-resources \"%s\"'" % media_resources_dir,
-            '-c', "\'TypesConfig \"%s\"\'" % mime_types_path,
-            '-c', "\'CustomLog \"%s\" common\'" % access_log,
-            '-c', "\'ErrorLog \"%s\"\'" % error_log,
-            '-C', "\'User \"%s\"\'" % os.environ.get("USERNAME", os.environ.get("USER", "")),
-            '-c', "\'PidFile %s'" % self._pid_file,
-            '-k', "start"]
+            '-f', "%s" % self._port_obj.path_to_apache_config_file(),
+            '-C', "ServerRoot %s" % server_root,
+            '-C', "DocumentRoot %s" % document_root,
+            '-c', "Alias /js-test-resources %s" % js_test_resources_dir,
+            '-c', "Alias /media-resources %s" % media_resources_dir,
+            '-c', "TypesConfig %s" % mime_types_path,
+            '-c', "CustomLog %s common" % access_log,
+            '-c', "ErrorLog %s" % error_log,
+            '-c', "PidFile %s" % self._pid_file,
+            ]
+
 
         enable_ipv6 = self._port_obj.http_server_supports_ipv6()
         # Perform part of the checks Apache's APR does when trying to listen to
@@ -99,7 +100,7 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
         for mapping in self._mappings:
             port = mapping['port']
 
-            start_cmd += ['-C', "\'Listen 127.0.0.1:%d\'" % port]
+            start_cmd += ['-C', "Listen 127.0.0.1:%d" % port]
 
             # We listen to both IPv4 and IPv6 loop-back addresses, but ignore
             # requests to 8000 from random users on network.
@@ -109,44 +110,22 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
 
         if additional_dirs:
             for alias, path in additional_dirs.iteritems():
-                start_cmd += ['-c', "\'Alias %s \"%s\"\'" % (alias, path),
+                start_cmd += ['-c', "'Alias %s \"%s\"\'" % (alias, path),
                         # Disable CGI handler for additional dirs.
-                        '-c', "\'<Location %s>\'" % alias,
-                        '-c', "\'RemoveHandler .cgi .pl\'",
-                        '-c', "\'</Location>\'"]
+                        '-c', "<Location %s>" % alias,
+                        '-c', "RemoveHandler .cgi .pl",
+                        '-c', "</Location>"]
 
         if self._number_of_servers:
-            start_cmd += ['-c', "\'StartServers %d\'" % self._number_of_servers,
-                          '-c', "\'MinSpareServers %d\'" % self._number_of_servers,
-                          '-c', "\'MaxSpareServers %d\'" % self._number_of_servers]
-
-        stop_cmd = [executable,
-            '-f', "\"%s\"" % self._get_apache_config_file_path(test_dir, output_dir),
-            '-c', "\'PidFile %s'" % self._pid_file,
-            '-k', "stop"]
-
-        start_cmd.extend(['-c', "\'SSLCertificateFile %s\'" % cert_file])
-        # Join the string here so that Cygwin/Windows and Mac/Linux
-        # can use the same code. Otherwise, we could remove the single
-        # quotes above and keep cmd as a sequence.
-        # FIXME: It's unclear if this is still needed.
-        self._start_cmd = " ".join(start_cmd)
-        self._stop_cmd = " ".join(stop_cmd)
-
-    def _get_apache_config_file_path(self, test_dir, output_dir):
-        """Returns the path to the apache config file to use.
-        Args:
-          test_dir: absolute path to the LayoutTests directory.
-          output_dir: absolute path to the layout test results directory.
-        """
-        httpd_config = self._port_obj._path_to_apache_config_file()
-        httpd_config_copy = os.path.join(output_dir, "httpd.conf")
-        httpd_conf = self._filesystem.read_text_file(httpd_config)
+            if self._is_win:
+                start_cmd += ['-c', "ThreadsPerChild %d" % self._number_of_servers]
+            else:
+                start_cmd += ['-c', "StartServers %d" % self._number_of_servers,
+                              '-c', "MinSpareServers %d" % self._number_of_servers,
+                              '-c', "MaxSpareServers %d" % self._number_of_servers]
 
-        # FIXME: Why do we need to copy the config file since we're not modifying it?
-        self._filesystem.write_text_file(httpd_config_copy, httpd_conf)
-
-        return httpd_config_copy
+        start_cmd.extend(['-c', "SSLCertificateFile %s" % cert_file])
+        self._start_cmd = start_cmd

[scottmg, 2014/02/04 04:58:01]

so much better!

 
     def _spawn_process(self):
         _log.debug('Starting %s server, cmd="%s"' % (self._name, str(self._start_cmd)))
@@ -161,6 +140,9 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
 
         return int(self._filesystem.read_text_file(self._pid_file))
 
+    def stop(self):
+        self._stop_running_server()
+
     def _stop_running_server(self):
         # If apache was forcefully killed, the pid file will not have been deleted, so check
         # that the process specified by the pid_file no longer exists before deleting the file.
@@ -168,25 +150,10 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
             self._filesystem.remove(self._pid_file)
             return
 
-        retval, err = self._run(self._stop_cmd)
-        if retval or len(err):
-            raise http_server_base.ServerError('Failed to stop %s: %s' % (self._name, err))
-
-        # For some reason apache isn't guaranteed to have actually stopped after
-        # the stop command returns, so we wait a little while longer for the
-        # pid file to be removed.
-        if not self._wait_for_action(lambda: not self._filesystem.exists(self._pid_file)):
-            raise http_server_base.ServerError('Failed to stop %s: pid file still exists' % self._name)
+        self._executive.kill_process(self._pid)
 
     def _run(self, cmd):
-        # Use shell=True because we join the arguments into a string for
-        # the sake of Window/Cygwin and it needs quoting that breaks
-        # shell=False.
-        # FIXME: We should not need to be joining shell arguments into strings.
-        # shell=True is a trail of tears.
-        # Note: Not thread safe: http://bugs.python.org/issue2320
-        process = self._executive.popen(cmd, shell=True, stderr=self._executive.PIPE)
-        process.wait()
-        retval = process.returncode
-        err = process.stderr.read()
-        return (retval, err)
+        self._process = self._executive.popen(cmd, stdout=self._executive.PIPE, stderr=self._executive.PIPE)
+        if self._process.returncode is not None:
+            return (self._process.returncode, self._process.stderr.read())
+        return (0, '')