Diff
Modified: trunk/Tools/ChangeLog (159838 => 159839)
--- trunk/Tools/ChangeLog 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/ChangeLog 2013-11-28 15:14:42 UTC (rev 159839)
@@ -1,5 +1,35 @@
2013-11-28 Dániel Bátyai <[email protected]>
+ Move PrettyPatch related code to prettypatch.py
+ https://bugs.webkit.org/show_bug.cgi?id=124937
+
+ Reviewed by Ryosuke Niwa.
+
+ This code seems to have a better place here than in Port, since PrettyPatch already knows
+ pretty_patch_path, and this also unifies the usage of PrettyPatch
+
+ * Scripts/webkitpy/common/prettypatch.py:
+ (PrettyPatch.__init__):
+ (PrettyPatch.pretty_diff):
+ (PrettyPatch):
+ (PrettyPatch.pretty_patch_available):
+ (PrettyPatch.check_pretty_patch):
+ (PrettyPatch.pretty_patch_text):
+ * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
+ (TestResultWriter.create_text_diff_and_write_result):
+ * Scripts/webkitpy/layout_tests/models/test_run_results.py:
+ (summarize_results):
+ * Scripts/webkitpy/port/base.py:
+ (Port.__init__):
+ (Port.wdiff_available):
+ (Port.check_image_diff):
+ (Port.wdiff_text):
+ * Scripts/webkitpy/port/base_unittest.py:
+ (PortTest.test_pretty_patch_os_error):
+ (PortTest.test_pretty_patch_script_error):
+
+2013-11-28 Dániel Bátyai <[email protected]>
+
Checkout should own the scm object in Host
https://bugs.webkit.org/show_bug.cgi?id=124943
Modified: trunk/Tools/Scripts/webkitpy/common/prettypatch.py (159838 => 159839)
--- trunk/Tools/Scripts/webkitpy/common/prettypatch.py 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/Scripts/webkitpy/common/prettypatch.py 2013-11-28 15:14:42 UTC (rev 159839)
@@ -26,15 +26,25 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+import errno
+import logging
import os
import tempfile
+from webkitpy.common.system.executive import ScriptError
+_log = logging.getLogger(__name__)
+
class PrettyPatch(object):
# FIXME: PrettyPatch should not require checkout_root.
- def __init__(self, executive, checkout_root):
+ def __init__(self, executive, checkout_root, filesystem=None):
self._executive = executive
+ self._filesystem = filesystem
self._checkout_root = checkout_root
+ self._pretty_patch_path = os.path.join(self._checkout_root, "Websites", "bugs.webkit.org", "PrettyPatch")
+ self._prettify_path = os.path.join(self._pretty_patch_path, "prettify.rb")
+ self.pretty_patch_error_html = "Failed to run PrettyPatch, see error log."
+ self.ppatch_available = None
def pretty_diff_file(self, diff):
# Diffs can contain multiple text files of different encodings
@@ -52,16 +62,59 @@
if not diff:
return ""
- pretty_patch_path = os.path.join(self._checkout_root,
- "Websites", "bugs.webkit.org",
- "PrettyPatch")
- prettify_path = os.path.join(pretty_patch_path, "prettify.rb")
args = [
"ruby",
"-I",
- pretty_patch_path,
- prettify_path,
+ self._pretty_patch_path,
+ self._prettify_path,
]
# PrettyPatch does not modify the encoding of the diff output
# so we can't expect it to be utf-8.
return self._executive.run_command(args, input=diff, decode_output=False)
+
+ def pretty_patch_available(self):
+ if self.ppatch_available is None:
+ self.ppatch_available = self.check_pretty_patch(logging=False)
+ return self.ppatch_available
+
+ def check_pretty_patch(self, logging=True):
+ """Checks whether we can use the PrettyPatch ruby script."""
+ try:
+ _ = self._executive.run_command(['ruby', '--version'])
+ except OSError, e:
+ if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
+ if logging:
+ _log.warning("Ruby is not installed; can't generate pretty patches.")
+ _log.warning('')
+ return False
+
+ if not self._filesystem.exists(self._pretty_patch_path):
+ if logging:
+ _log.warning("Unable to find %s; can't generate pretty patches." % self._pretty_patch_path)
+ _log.warning('')
+ return False
+
+ return True
+
+ def pretty_patch_text(self, diff_path):
+ if self.ppatch_available is None:
+ self.ppatch_available = self.check_pretty_patch(logging=False)
+ if not self.ppatch_available:
+ return self.pretty_patch_error_html
+ command = ("ruby", "-I", self._filesystem.dirname(self._pretty_patch_path),
+ self._pretty_patch_path, diff_path)
+ try:
+ # Diffs are treated as binary (we pass decode_output=False) as they
+ # may contain multiple files of conflicting encodings.
+ return self._executive.run_command(command, decode_output=False)
+ except OSError, e:
+ # If the system is missing ruby log the error and stop trying.
+ self.ppatch_available = False
+ _log.error("Failed to run PrettyPatch (%s): %s" % (command, e))
+ return self.pretty_patch_error_html
+ except ScriptError, e:
+ # If ruby failed to run for some reason, log the command
+ # output and stop trying.
+ self.ppatch_available = False
+ _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
+ return self.pretty_patch_error_html
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py (159838 => 159839)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py 2013-11-28 15:14:42 UTC (rev 159839)
@@ -196,8 +196,8 @@
self._write_binary_file(wdiff_filename, wdiff)
# Use WebKit's PrettyPatch.rb to get an HTML diff.
- if self._port.pretty_patch_available():
- pretty_patch = self._port.pretty_patch_text(diff_filename)
+ if self._port.pretty_patch.pretty_patch_available():
+ pretty_patch = self._port.pretty_patch.pretty_patch_text(diff_filename)
pretty_patch_filename = self.output_filename(self.FILENAME_SUFFIX_PRETTY_PATCH)
self._write_binary_file(pretty_patch_filename, pretty_patch)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py (159838 => 159839)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py 2013-11-28 15:14:42 UTC (rev 159839)
@@ -243,7 +243,7 @@
results['interrupted'] = initial_results.interrupted # Does results.html have enough information to compute this itself? (by checking total number of results vs. total number of tests?)
results['layout_tests_dir'] = port_obj.layout_tests_dir()
results['has_wdiff'] = port_obj.wdiff_available()
- results['has_pretty_patch'] = port_obj.pretty_patch_available()
+ results['has_pretty_patch'] = port_obj.pretty_patch.pretty_patch_available()
results['pixel_tests_enabled'] = port_obj.get_option('pixel_tests')
try:
Modified: trunk/Tools/Scripts/webkitpy/port/base.py (159838 => 159839)
--- trunk/Tools/Scripts/webkitpy/port/base.py 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/Scripts/webkitpy/port/base.py 2013-11-28 15:14:42 UTC (rev 159839)
@@ -51,6 +51,7 @@
from webkitpy.common import find_files
from webkitpy.common import read_checksum_from_png
from webkitpy.common.memoized import memoized
+from webkitpy.common.prettypatch import PrettyPatch
from webkitpy.common.system import path
from webkitpy.common.system.executive import ScriptError
from webkitpy.common.system.systemhost import SystemHost
@@ -116,6 +117,7 @@
self._filesystem = host.filesystem
self._webkit_finder = WebKitFinder(host.filesystem)
self._config = port_config.Config(self._executive, self._filesystem, self.port_name)
+ self.pretty_patch = PrettyPatch(self._executive, self.path_from_webkit_base(), self._filesystem)
self._helper = None
self._http_server = None
@@ -139,10 +141,6 @@
# http://bugs.python.org/issue3210
self._wdiff_available = None
- # FIXME: prettypatch.py knows this path, why is it copied here?
- self._pretty_patch_path = self.path_from_webkit_base("Websites", "bugs.webkit.org", "PrettyPatch", "prettify.rb")
- self._pretty_patch_available = None
-
if not hasattr(options, 'configuration') or not options.configuration:
self.set_option_default('configuration', self.default_configuration())
self._test_configuration = None
@@ -178,11 +176,6 @@
self._wdiff_available = self.check_wdiff(logging=False)
return self._wdiff_available
- def pretty_patch_available(self):
- if self._pretty_patch_available is None:
- self._pretty_patch_available = self.check_pretty_patch(logging=False)
- return self._pretty_patch_available
-
def should_retry_crashes(self):
return False
@@ -281,25 +274,6 @@
return False
return True
- def check_pretty_patch(self, logging=True):
- """Checks whether we can use the PrettyPatch ruby script."""
- try:
- _ = self._executive.run_command(['ruby', '--version'])
- except OSError, e:
- if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
- if logging:
- _log.warning("Ruby is not installed; can't generate pretty patches.")
- _log.warning('')
- return False
-
- if not self._filesystem.exists(self._pretty_patch_path):
- if logging:
- _log.warning("Unable to find %s; can't generate pretty patches." % self._pretty_patch_path)
- _log.warning('')
- return False
-
- return True
-
def check_wdiff(self, logging=True):
if not self._path_to_wdiff():
# Don't need to log here since this is the port choosing not to use wdiff.
@@ -1154,32 +1128,6 @@
return ""
raise
- # This is a class variable so we can test error output easily.
- _pretty_patch_error_html = "Failed to run PrettyPatch, see error log."
-
- def pretty_patch_text(self, diff_path):
- if self._pretty_patch_available is None:
- self._pretty_patch_available = self.check_pretty_patch(logging=False)
- if not self._pretty_patch_available:
- return self._pretty_patch_error_html
- command = ("ruby", "-I", self._filesystem.dirname(self._pretty_patch_path),
- self._pretty_patch_path, diff_path)
- try:
- # Diffs are treated as binary (we pass decode_output=False) as they
- # may contain multiple files of conflicting encodings.
- return self._executive.run_command(command, decode_output=False)
- except OSError, e:
- # If the system is missing ruby log the error and stop trying.
- self._pretty_patch_available = False
- _log.error("Failed to run PrettyPatch (%s): %s" % (command, e))
- return self._pretty_patch_error_html
- except ScriptError, e:
- # If ruby failed to run for some reason, log the command
- # output and stop trying.
- self._pretty_patch_available = False
- _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
- return self._pretty_patch_error_html
-
def default_configuration(self):
return self._config.default_configuration()
Modified: trunk/Tools/Scripts/webkitpy/port/base_unittest.py (159838 => 159839)
--- trunk/Tools/Scripts/webkitpy/port/base_unittest.py 2013-11-28 13:46:26 UTC (rev 159838)
+++ trunk/Tools/Scripts/webkitpy/port/base_unittest.py 2013-11-28 15:14:42 UTC (rev 159839)
@@ -90,24 +90,24 @@
port = self.make_port(executive=executive_mock.MockExecutive2(exception=OSError))
oc = OutputCapture()
oc.capture_output()
- self.assertEqual(port.pretty_patch_text("patch.txt"),
- port._pretty_patch_error_html)
+ self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
+ port.pretty_patch.pretty_patch_error_html)
# This tests repeated calls to make sure we cache the result.
- self.assertEqual(port.pretty_patch_text("patch.txt"),
- port._pretty_patch_error_html)
+ self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
+ port.pretty_patch.pretty_patch_error_html)
oc.restore_output()
def test_pretty_patch_script_error(self):
# FIXME: This is some ugly white-box test hacking ...
port = self.make_port(executive=executive_mock.MockExecutive2(exception=ScriptError))
- port._pretty_patch_available = True
- self.assertEqual(port.pretty_patch_text("patch.txt"),
- port._pretty_patch_error_html)
+ port.pretty_patch.ppatch_available = True
+ self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
+ port.pretty_patch.pretty_patch_error_html)
# This tests repeated calls to make sure we cache the result.
- self.assertEqual(port.pretty_patch_text("patch.txt"),
- port._pretty_patch_error_html)
+ self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
+ port.pretty_patch.pretty_patch_error_html)
def integration_test_run_wdiff(self):
executive = Executive()
