diff options
| author | Ben Murdoch <benm@google.com> | 2010-05-11 18:35:50 +0100 |
|---|---|---|
| committer | Ben Murdoch <benm@google.com> | 2010-05-14 10:23:05 +0100 |
| commit | 21939df44de1705786c545cd1bf519d47250322d (patch) | |
| tree | ef56c310f5c0cdc379c2abb2e212308a3281ce20 /WebKitTools/Scripts/webkitpy/common | |
| parent | 4ff1d8891d520763f17675827154340c7c740f90 (diff) | |
| download | external_webkit-21939df44de1705786c545cd1bf519d47250322d.zip external_webkit-21939df44de1705786c545cd1bf519d47250322d.tar.gz external_webkit-21939df44de1705786c545cd1bf519d47250322d.tar.bz2 | |
Merge Webkit at r58956: Initial merge by Git.
Change-Id: I1d9fb60ea2c3f2ddc04c17a871acdb39353be228
Diffstat (limited to 'WebKitTools/Scripts/webkitpy/common')
23 files changed, 998 insertions, 243 deletions
diff --git a/WebKitTools/Scripts/webkitpy/common/array_stream.py b/WebKitTools/Scripts/webkitpy/common/array_stream.py new file mode 100644 index 0000000..e425d02 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/common/array_stream.py @@ -0,0 +1,66 @@ +#!/usr/bin/python +# Copyright (C) 2010 Google Inc. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +"""Package that private an array-based implementation of a stream.""" + + +class ArrayStream(object): + """Simple class that implmements a stream interface on top of an array. + + This is used primarily by unit test classes to mock output streams. It + performs a similar function to StringIO, but (a) it is write-only, and + (b) it can be used to retrieve each individual write(); StringIO + concatenates all of the writes together. + """ + + def __init__(self): + self._contents = [] + + def write(self, msg): + """Implement stream.write() by appending to the stream's contents.""" + self._contents.append(msg) + + def get(self): + """Return the contents of a stream (as an array).""" + return self._contents + + def reset(self): + """Empty the stream.""" + self._contents = [] + + def empty(self): + """Return whether the stream is empty.""" + return (len(self._contents) == 0) + + def flush(self): + """Flush the stream (a no-op implemented for compatibility).""" + pass + + def __repr__(self): + return '<ArrayStream: ' + str(self._contents) + '>' diff --git a/WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py b/WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py new file mode 100644 index 0000000..1a9b34a --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py @@ -0,0 +1,78 @@ +#!/usr/bin/python +# Copyright (C) 2010 Google Inc. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +"""Unit tests for array_stream.py.""" + +import pdb +import unittest + +from webkitpy.common.array_stream import ArrayStream + + +class ArrayStreamTest(unittest.TestCase): + def assertEmpty(self, a_stream): + self.assertTrue(a_stream.empty()) + + def assertNotEmpty(self, a_stream): + self.assertFalse(a_stream.empty()) + + def assertContentsMatch(self, a_stream, contents): + self.assertEquals(a_stream.get(), contents) + + def test_basics(self): + a = ArrayStream() + self.assertEmpty(a) + self.assertContentsMatch(a, []) + + a.flush() + self.assertEmpty(a) + self.assertContentsMatch(a, []) + + a.write("foo") + a.write("bar") + self.assertNotEmpty(a) + self.assertContentsMatch(a, ["foo", "bar"]) + + a.flush() + self.assertNotEmpty(a) + self.assertContentsMatch(a, ["foo", "bar"]) + + a.reset() + self.assertEmpty(a) + self.assertContentsMatch(a, []) + + self.assertEquals(str(a), "<ArrayStream: []>") + + a.write("foo") + self.assertNotEmpty(a) + self.assertContentsMatch(a, ["foo"]) + self.assertEquals(str(a), "<ArrayStream: ['foo']>") + +if __name__ == '__main__': + unittest.main() diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api.py b/WebKitTools/Scripts/webkitpy/common/checkout/api.py index c4e2b69..a5ac939 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api.py @@ -27,7 +27,6 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import os -import subprocess import StringIO from webkitpy.common.checkout.changelog import ChangeLog @@ -50,7 +49,11 @@ class Checkout(object): def _latest_entry_for_changelog_at_revision(self, changelog_path, revision): changelog_contents = self._scm.contents_at_revision(changelog_path, revision) - return ChangeLog.parse_latest_entry_from_file(StringIO.StringIO(changelog_contents)) + # contents_at_revision returns a byte array (str()), but we know + # that ChangeLog files are utf-8. parse_latest_entry_from_file + # expects a file-like object which vends unicode(), so we decode here. + changelog_file = StringIO.StringIO(changelog_contents.decode("utf-8")) + return ChangeLog.parse_latest_entry_from_file(changelog_file) def changelog_entries_for_revision(self, revision): changed_files = self._scm.changed_files_for_revision(revision) @@ -80,16 +83,16 @@ class Checkout(object): def bug_id_for_revision(self, revision): return self.commit_info_for_revision(revision).bug_id() - def modified_changelogs(self): + def modified_changelogs(self, git_commit, squash): # SCM returns paths relative to scm.checkout_root # Callers (especially those using the ChangeLog class) may # expect absolute paths, so this method returns absolute paths. - changed_files = self._scm.changed_files() + changed_files = self._scm.changed_files(git_commit, squash) absolute_paths = [os.path.join(self._scm.checkout_root, path) for path in changed_files] return [path for path in absolute_paths if self._is_path_to_changelog(path)] - def commit_message_for_this_commit(self): - changelog_paths = self.modified_changelogs() + def commit_message_for_this_commit(self, git_commit, squash): + changelog_paths = self.modified_changelogs(git_commit, squash) if not len(changelog_paths): raise ScriptError(message="Found no modified ChangeLogs, cannot create a commit message.\n" "All changes require a ChangeLog. See:\n" @@ -106,32 +109,29 @@ class Checkout(object): # FIXME: We should sort and label the ChangeLog messages like commit-log-editor does. return CommitMessage("".join(changelog_messages).splitlines()) - def bug_id_for_this_commit(self): + def bug_id_for_this_commit(self, git_commit, squash): try: - return parse_bug_id(self.commit_message_for_this_commit().message()) + return parse_bug_id(self.commit_message_for_this_commit(git_commit, squash).message()) except ScriptError, e: pass # We might not have ChangeLogs. def apply_patch(self, patch, force=False): # It's possible that the patch was not made from the root directory. # We should detect and handle that case. - # FIXME: Use Executive instead of subprocess here. - curl_process = subprocess.Popen(['curl', '--location', '--silent', '--show-error', patch.url()], stdout=subprocess.PIPE) # FIXME: Move _scm.script_path here once we get rid of all the dependencies. args = [self._scm.script_path('svn-apply')] if patch.reviewer(): args += ['--reviewer', patch.reviewer().full_name] if force: args.append('--force') - - run_command(args, input=curl_process.stdout) + run_command(args, input=patch.contents()) def apply_reverse_diff(self, revision): self._scm.apply_reverse_diff(revision) # We revert the ChangeLogs because removing lines from a ChangeLog # doesn't make sense. ChangeLogs are append only. - changelog_paths = self.modified_changelogs() + changelog_paths = self.modified_changelogs(git_commit=None, squash=False) if len(changelog_paths): self._scm.revert_files(changelog_paths) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py index e99caee..1436379 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/api_unittest.py @@ -26,6 +26,9 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from __future__ import with_statement + +import codecs import os import shutil import tempfile @@ -37,14 +40,14 @@ from webkitpy.common.checkout.scm import detect_scm_system, CommitMessage from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.thirdparty.mock import Mock + # FIXME: Copied from scm_unittest.py -def write_into_file_at_path(file_path, contents): - new_file = open(file_path, 'w') - new_file.write(contents) - new_file.close() +def write_into_file_at_path(file_path, contents, encoding="utf-8"): + with codecs.open(file_path, "w", encoding) as file: + file.write(contents) -_changelog1entry1 = """2010-03-25 Eric Seidel <eric@webkit.org> +_changelog1entry1 = u"""2010-03-25 Tor Arne Vestb\u00f8 <vestbo@webkit.org> Unreviewed build fix to un-break webkit-patch land. @@ -53,7 +56,7 @@ _changelog1entry1 = """2010-03-25 Eric Seidel <eric@webkit.org> * Scripts/webkitpy/common/checkout/api.py: import scm.CommitMessage """ -_changelog1entry2 = """2010-03-25 Adam Barth <abarth@webkit.org> +_changelog1entry2 = u"""2010-03-25 Adam Barth <abarth@webkit.org> Reviewed by Eric Seidel. @@ -62,8 +65,8 @@ _changelog1entry2 = """2010-03-25 Adam Barth <abarth@webkit.org> * Scripts/webkitpy/common/checkout/api.py: """ -_changelog1 = "\n".join([_changelog1entry1, _changelog1entry2]) -_changelog2 = """2010-03-25 Eric Seidel <eric@webkit.org> +_changelog1 = u"\n".join([_changelog1entry1, _changelog1entry2]) +_changelog2 = u"""2010-03-25 Tor Arne Vestb\u00f8 <vestbo@webkit.org> Unreviewed build fix to un-break webkit-patch land. @@ -79,7 +82,7 @@ _changelog2 = """2010-03-25 Eric Seidel <eric@webkit.org> """ class CommitMessageForThisCommitTest(unittest.TestCase): - expected_commit_message = """2010-03-25 Eric Seidel <eric@webkit.org> + expected_commit_message = u"""2010-03-25 Tor Arne Vestb\u00f8 <vestbo@webkit.org> Unreviewed build fix to un-break webkit-patch land. @@ -87,7 +90,7 @@ class CommitMessageForThisCommitTest(unittest.TestCase): https://bugs.webkit.org/show_bug.cgi?id=36629 * Scripts/webkitpy/common/checkout/api.py: import scm.CommitMessage -2010-03-25 Eric Seidel <eric@webkit.org> +2010-03-25 Tor Arne Vestb\u00f8 <vestbo@webkit.org> Unreviewed build fix to un-break webkit-patch land. @@ -111,10 +114,11 @@ class CommitMessageForThisCommitTest(unittest.TestCase): # ChangeLog is difficult to mock at current. def test_commit_message_for_this_commit(self): checkout = Checkout(None) - checkout.modified_changelogs = lambda: ["ChangeLog1", "ChangeLog2"] + checkout.modified_changelogs = lambda git_commit, squash: ["ChangeLog1", "ChangeLog2"] output = OutputCapture() expected_stderr = "Parsing ChangeLog: ChangeLog1\nParsing ChangeLog: ChangeLog2\n" - commit_message = output.assert_outputs(self, checkout.commit_message_for_this_commit, expected_stderr=expected_stderr) + commit_message = output.assert_outputs(self, checkout.commit_message_for_this_commit, + kwargs={"git_commit": None, "squash": False}, expected_stderr=expected_stderr) self.assertEqual(commit_message.message(), self.expected_commit_message) @@ -124,7 +128,9 @@ class CheckoutTest(unittest.TestCase): def mock_contents_at_revision(changelog_path, revision): self.assertEqual(changelog_path, "foo") self.assertEqual(revision, "bar") - return _changelog1 + # contents_at_revision is expected to return a byte array (str) + # so we encode our unicode ChangeLog down to a utf-8 stream. + return _changelog1.encode("utf-8") scm.contents_at_revision = mock_contents_at_revision checkout = Checkout(scm) entry = checkout._latest_entry_for_changelog_at_revision("foo", "bar") @@ -137,8 +143,8 @@ class CheckoutTest(unittest.TestCase): checkout.changelog_entries_for_revision = lambda revision: [ChangeLogEntry(_changelog1entry1)] commitinfo = checkout.commit_info_for_revision(4) self.assertEqual(commitinfo.bug_id(), 36629) - self.assertEqual(commitinfo.author_name(), "Eric Seidel") - self.assertEqual(commitinfo.author_email(), "eric@webkit.org") + self.assertEqual(commitinfo.author_name(), u"Tor Arne Vestb\u00f8") + self.assertEqual(commitinfo.author_email(), "vestbo@webkit.org") self.assertEqual(commitinfo.reviewer_text(), None) self.assertEqual(commitinfo.reviewer(), None) self.assertEqual(commitinfo.committer_email(), "committer@example.com") @@ -157,13 +163,13 @@ class CheckoutTest(unittest.TestCase): def test_bug_id_for_this_commit(self): scm = Mock() checkout = Checkout(scm) - checkout.commit_message_for_this_commit = lambda: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) - self.assertEqual(checkout.bug_id_for_this_commit(), 36629) + checkout.commit_message_for_this_commit = lambda git_commit, squash: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines()) + self.assertEqual(checkout.bug_id_for_this_commit(git_commit=None, squash=False), 36629) def test_modified_changelogs(self): scm = Mock() scm.checkout_root = "/foo/bar" - scm.changed_files = lambda:["file1", "ChangeLog", "relative/path/ChangeLog"] + scm.changed_files = lambda git_commit, squash: ["file1", "ChangeLog", "relative/path/ChangeLog"] checkout = Checkout(scm) expected_changlogs = ["/foo/bar/ChangeLog", "/foo/bar/relative/path/ChangeLog"] - self.assertEqual(checkout.modified_changelogs(), expected_changlogs) + self.assertEqual(checkout.modified_changelogs(git_commit=None, squash=False), expected_changlogs) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py b/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py index e93896f..6220fbd 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py @@ -99,10 +99,14 @@ class ChangeLog(object): @staticmethod def parse_latest_entry_from_file(changelog_file): + """changelog_file must be a file-like object which returns + unicode strings. Use codecs.open or StringIO(unicode()) + to pass file objects to this class.""" date_line_regexp = re.compile(ChangeLogEntry.date_line_regexp) entry_lines = [] # The first line should be a date line. first_line = changelog_file.readline() + assert(isinstance(first_line, unicode)) if not date_line_regexp.match(first_line): return None entry_lines.append(first_line) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/changelog_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/changelog_unittest.py index 9210c9c..864428a 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/changelog_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/changelog_unittest.py @@ -26,9 +26,12 @@ # (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 unittest +from __future__ import with_statement + +import codecs import os import tempfile +import unittest from StringIO import StringIO @@ -52,7 +55,7 @@ class ChangeLogsTest(unittest.TestCase): ''' # More example text than we need. Eventually we need to support parsing this all and write tests for the parsing. - _example_changelog = '''2009-08-17 David Kilzer <ddkilzer@apple.com> + _example_changelog = u"""2009-08-17 Tor Arne Vestb\xf8 <vestbo@webkit.org> <http://webkit.org/b/28393> check-webkit-style: add check for use of std::max()/std::min() instead of MAX()/MIN() @@ -84,10 +87,10 @@ class ChangeLogsTest(unittest.TestCase): so we can't assert here. == Rolled over to ChangeLog-2009-06-16 == -''' +""" def test_latest_entry_parse(self): - changelog_contents = "%s\n%s" % (self._example_entry, self._example_changelog) + changelog_contents = u"%s\n%s" % (self._example_entry, self._example_changelog) changelog_file = StringIO(changelog_contents) latest_entry = ChangeLog.parse_latest_entry_from_file(changelog_file) self.assertEquals(latest_entry.contents(), self._example_entry) @@ -97,19 +100,17 @@ class ChangeLogsTest(unittest.TestCase): self.assertTrue(latest_entry.reviewer()) # Make sure that our UTF8-based lookup of Tor works. @staticmethod - def _write_tmp_file_with_contents(contents): + def _write_tmp_file_with_contents(byte_array): + assert(isinstance(byte_array, str)) (file_descriptor, file_path) = tempfile.mkstemp() # NamedTemporaryFile always deletes the file on close in python < 2.6 - file = os.fdopen(file_descriptor, 'w') - file.write(contents) - file.close() + with os.fdopen(file_descriptor, "w") as file: + file.write(byte_array) return file_path @staticmethod - def _read_file_contents(file_path): - file = open(file_path) - contents = file.read() - file.close() - return contents + def _read_file_contents(file_path, encoding): + with codecs.open(file_path, "r", encoding) as file: + return file.read() _new_entry_boilerplate = '''2009-08-19 Eric Seidel <eric@webkit.org> @@ -121,11 +122,11 @@ class ChangeLogsTest(unittest.TestCase): ''' def test_set_reviewer(self): - changelog_contents = "%s\n%s" % (self._new_entry_boilerplate, self._example_changelog) - changelog_path = self._write_tmp_file_with_contents(changelog_contents) + changelog_contents = u"%s\n%s" % (self._new_entry_boilerplate, self._example_changelog) + changelog_path = self._write_tmp_file_with_contents(changelog_contents.encode("utf-8")) reviewer_name = 'Test Reviewer' ChangeLog(changelog_path).set_reviewer(reviewer_name) - actual_contents = self._read_file_contents(changelog_path) + actual_contents = self._read_file_contents(changelog_path, "utf-8") expected_contents = changelog_contents.replace('NOBODY (OOPS!)', reviewer_name) os.remove(changelog_path) self.assertEquals(actual_contents, expected_contents) @@ -169,8 +170,8 @@ class ChangeLogsTest(unittest.TestCase): ''' def _assert_update_for_revert_output(self, args, expected_entry): - changelog_contents = "%s\n%s" % (self._new_entry_boilerplate, self._example_changelog) - changelog_path = self._write_tmp_file_with_contents(changelog_contents) + changelog_contents = u"%s\n%s" % (self._new_entry_boilerplate, self._example_changelog) + changelog_path = self._write_tmp_file_with_contents(changelog_contents.encode("utf-8")) changelog = ChangeLog(changelog_path) changelog.update_for_revert(*args) actual_entry = changelog.latest_entry() diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/commitinfo.py b/WebKitTools/Scripts/webkitpy/common/checkout/commitinfo.py index 7c3315f..448d530 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/commitinfo.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/commitinfo.py @@ -28,8 +28,6 @@ # # WebKit's python module for holding information on a commit -import StringIO - from webkitpy.common.checkout.changelog import view_source_url from webkitpy.common.config.committers import CommitterList diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py index 2704f07..02e114a 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py @@ -41,11 +41,13 @@ from webkitpy.common.system.deprecated_logging import error, log def detect_scm_system(path): - if SVN.in_working_directory(path): - return SVN(cwd=path) + absolute_path = os.path.abspath(path) + + if SVN.in_working_directory(absolute_path): + return SVN(cwd=absolute_path) - if Git.in_working_directory(path): - return Git(cwd=path) + if Git.in_working_directory(absolute_path): + return Git(cwd=absolute_path) return None @@ -145,7 +147,7 @@ class SCM: return filenames def strip_r_from_svn_revision(self, svn_revision): - match = re.match("^r(?P<svn_revision>\d+)", svn_revision) + match = re.match("^r(?P<svn_revision>\d+)", unicode(svn_revision)) if (match): return match.group('svn_revision') return svn_revision @@ -178,7 +180,7 @@ class SCM: def add(self, path): raise NotImplementedError, "subclasses must implement" - def changed_files(self): + def changed_files(self, git_commit=None, squash=None): raise NotImplementedError, "subclasses must implement" def changed_files_for_revision(self): @@ -193,7 +195,7 @@ class SCM: def display_name(self): raise NotImplementedError, "subclasses must implement" - def create_patch(self): + def create_patch(self, git_commit=None, squash=None): raise NotImplementedError, "subclasses must implement" def committer_email_for_revision(self, revision): @@ -211,7 +213,10 @@ class SCM: def revert_files(self, file_paths): raise NotImplementedError, "subclasses must implement" - def commit_with_message(self, message, username=None): + def should_squash(self, squash): + raise NotImplementedError, "subclasses must implement" + + def commit_with_message(self, message, username=None, git_commit=None, squash=None): raise NotImplementedError, "subclasses must implement" def svn_commit_log(self, svn_revision): @@ -229,12 +234,6 @@ class SCM: def svn_merge_base(): raise NotImplementedError, "subclasses must implement" - def create_patch_from_local_commit(self, commit_id): - error("Your source control manager does not support creating a patch from a local commit.") - - def create_patch_since_local_commit(self, commit_id): - error("Your source control manager does not support creating a patch from a local commit.") - def commit_locally_with_message(self, message): error("Your source control manager does not support local commits.") @@ -308,7 +307,7 @@ class SVN(SCM): return self.cached_version def working_directory_is_clean(self): - return run_command(["svn", "diff"], cwd=self.checkout_root) == "" + return run_command(["svn", "diff"], cwd=self.checkout_root, decode_output=False) == "" def clean_working_directory(self): # svn revert -R is not as awesome as git reset --hard. @@ -339,12 +338,13 @@ class SVN(SCM): # path is assumed to be cwd relative? run_command(["svn", "add", path]) - def changed_files(self): + def changed_files(self, git_commit=None, squash=None): return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("ACDMR")) def changed_files_for_revision(self, revision): # As far as I can tell svn diff --summarize output looks just like svn status output. - status_command = ["svn", "diff", "--summarize", "-c", str(revision)] + # No file contents printed, thus utf-8 auto-decoding in run_command is fine. + status_command = ["svn", "diff", "--summarize", "-c", revision] return self.run_status_and_extract_filenames(status_command, self._status_regexp("ACDMR")) def conflicted_files(self): @@ -360,19 +360,26 @@ class SVN(SCM): def display_name(self): return "svn" - def create_patch(self): - return run_command(self.script_path("svn-create-patch"), cwd=self.checkout_root, return_stderr=False) + def create_patch(self, git_commit=None, squash=None): + """Returns a byte array (str()) representing the patch file. + Patch files are effectively binary since they may contain + files of multiple different encodings.""" + return run_command([self.script_path("svn-create-patch")], + cwd=self.checkout_root, return_stderr=False, + decode_output=False) def committer_email_for_revision(self, revision): - return run_command(["svn", "propget", "svn:author", "--revprop", "-r", str(revision)]).rstrip() + return run_command(["svn", "propget", "svn:author", "--revprop", "-r", revision]).rstrip() def contents_at_revision(self, path, revision): + """Returns a byte array (str()) containing the contents + of path @ revision in the repository.""" remote_path = "%s/%s" % (self._repository_url(), path) - return run_command(["svn", "cat", "-r", str(revision), remote_path]) + return run_command(["svn", "cat", "-r", revision, remote_path], decode_output=False) def diff_for_revision(self, revision): # FIXME: This should probably use cwd=self.checkout_root - return run_command(['svn', 'diff', '-c', str(revision)]) + return run_command(['svn', 'diff', '-c', revision]) def _repository_url(self): return self.value_from_svn_info(self.checkout_root, 'URL') @@ -389,7 +396,12 @@ class SVN(SCM): # FIXME: This should probably use cwd=self.checkout_root. run_command(['svn', 'revert'] + file_paths) - def commit_with_message(self, message, username=None): + def should_squash(self, squash): + # SVN doesn't support the concept of squashing. + return False + + def commit_with_message(self, message, username=None, git_commit=None, squash=None): + # squash and git-commit are not used by SVN. if self.dryrun: # Return a string which looks like a commit so that things which parse this output will succeed. return "Dry run, no commit.\nCommitted revision 0." @@ -405,7 +417,7 @@ class SVN(SCM): return run_command(svn_commit_args, error_handler=commit_error_handler) def svn_commit_log(self, svn_revision): - svn_revision = self.strip_r_from_svn_revision(str(svn_revision)) + svn_revision = self.strip_r_from_svn_revision(svn_revision) return run_command(['svn', 'log', '--non-interactive', '--revision', svn_revision]); def last_svn_commit_log(self): @@ -466,6 +478,7 @@ class Git(SCM): def status_command(self): # git status returns non-zero when there are changes, so we use git diff name --name-status HEAD instead. + # No file contents printed, thus utf-8 autodecoding in run_command is fine. return ["git", "diff", "--name-status", "HEAD"] def _status_regexp(self, expected_types): @@ -475,8 +488,24 @@ class Git(SCM): # path is assumed to be cwd relative? run_command(["git", "add", path]) - def changed_files(self): - status_command = ['git', 'diff', '-r', '--name-status', '-C', '-M', 'HEAD'] + def _merge_base(self, git_commit, squash): + if git_commit: + # FIXME: Calling code should turn commit ranges into a list of commit IDs + # and then treat each commit separately. + if '..' not in git_commit: + git_commit = git_commit + "^.." + git_commit + return git_commit + + if self.should_squash(squash): + return self.svn_merge_base() + + # FIXME: Non-squash behavior should match commit_with_message. It raises an error + # if there are working copy changes and --squash or --no-squash wasn't passed in. + # If --no-squash, then it should proceed with each local commit as a separate patch. + return 'HEAD' + + def changed_files(self, git_commit=None, squash=None): + status_command = ['git', 'diff', '-r', '--name-status', '-C', '-M', "--no-ext-diff", "--full-index", self._merge_base(git_commit, squash)] return self.run_status_and_extract_filenames(status_command, self._status_regexp("ADM")) def _changes_files_for_commit(self, git_commit): @@ -490,6 +519,8 @@ class Git(SCM): return self._changes_files_for_commit(commit_id) def conflicted_files(self): + # We do not need to pass decode_output for this diff command + # as we're passing --name-status which does not output any data. status_command = ['git', 'diff', '--name-status', '-C', '-M', '--diff-filter=U'] return self.run_status_and_extract_filenames(status_command, self._status_regexp("U")) @@ -503,9 +534,12 @@ class Git(SCM): def display_name(self): return "git" - def create_patch(self): + def create_patch(self, git_commit=None, squash=None): + """Returns a byte array (str()) representing the patch file. + Patch files are effectively binary since they may contain + files of multiple different encodings.""" # FIXME: This should probably use cwd=self.checkout_root - return run_command(['git', 'diff', '--binary', 'HEAD']) + return run_command(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self._merge_base(git_commit, squash)], decode_output=False) @classmethod def git_commit_from_svn_revision(cls, revision): @@ -517,11 +551,13 @@ class Git(SCM): return git_commit def contents_at_revision(self, path, revision): - return run_command(["git", "show", "%s:%s" % (self.git_commit_from_svn_revision(revision), path)]) + """Returns a byte array (str()) containing the contents + of path @ revision in the repository.""" + return run_command(["git", "show", "%s:%s" % (self.git_commit_from_svn_revision(revision), path)], decode_output=False) def diff_for_revision(self, revision): git_commit = self.git_commit_from_svn_revision(revision) - return self.create_patch_from_local_commit(git_commit) + return self.create_patch(git_commit) def committer_email_for_revision(self, revision): git_commit = self.git_commit_from_svn_revision(revision) @@ -538,11 +574,100 @@ class Git(SCM): def revert_files(self, file_paths): run_command(['git', 'checkout', 'HEAD'] + file_paths) - def commit_with_message(self, message, username=None): + def should_squash(self, squash): + if squash is not None: + # Squash is specified on the command-line. + return squash + + config_squash = Git.read_git_config('webkit-patch.squash') + if (config_squash and config_squash is not ""): + return config_squash.lower() == "true" + + # Only raise an error if there are actually multiple commits to squash. + num_local_commits = len(self.local_commits()) + if num_local_commits > 1 or num_local_commits > 0 and not self.working_directory_is_clean(): + working_directory_message = "" if self.working_directory_is_clean() else " and working copy changes" + raise ScriptError(message="""There are %s local commits%s. Do one of the following: +1) Use --squash or --no-squash +2) git config webkit-patch.squash true/false +""" % (num_local_commits, working_directory_message)) + + return None + + def commit_with_message(self, message, username=None, git_commit=None, squash=None): # Username is ignored during Git commits. - self.commit_locally_with_message(message) + if git_commit: + # Need working directory changes to be committed so we can checkout the merge branch. + if not self.working_directory_is_clean(): + # FIXME: webkit-patch land will modify the ChangeLogs to correct the reviewer. + # That will modify the working-copy and cause us to hit this error. + # The ChangeLog modification could be made to modify the existing local commit? + raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.") + return self._commit_on_branch(message, git_commit) + + squash = self.should_squash(squash) + if squash: + run_command(['git', 'reset', '--soft', self.svn_branch_name()]) + self.commit_locally_with_message(message) + elif not self.working_directory_is_clean(): + if not len(self.local_commits()): + # There are only working copy changes. Assume they should be committed. + self.commit_locally_with_message(message) + elif squash is None: + # The user didn't explicitly say to squash or not squash. There are local commits + # and working copy changes. Not clear what the user wants. + raise ScriptError(message="""There are local commits and working copy changes. Do one of the following: +1) Commit/revert working copy changes. +2) Use --squash or --no-squash +3) git config webkit-patch.squash true/false +""") + + # FIXME: This will commit all local commits, each with it's own message. We should restructure + # so that each local commit has the appropriate commit message based off it's ChangeLogs. return self.push_local_commits_to_server() + def _commit_on_branch(self, message, git_commit): + branch_ref = run_command(['git', 'symbolic-ref', 'HEAD']).strip() + branch_name = branch_ref.replace('refs/heads/', '') + commit_ids = self.commit_ids_from_commitish_arguments([git_commit]) + + # We want to squash all this branch's commits into one commit with the proper description. + # We do this by doing a "merge --squash" into a new commit branch, then dcommitting that. + MERGE_BRANCH = 'webkit-patch-land' + self.delete_branch(MERGE_BRANCH) + + # We might be in a directory that's present in this branch but not in the + # trunk. Move up to the top of the tree so that git commands that expect a + # valid CWD won't fail after we check out the merge branch. + os.chdir(self.checkout_root) + + # Stuff our change into the merge branch. + # We wrap in a try...finally block so if anything goes wrong, we clean up the branches. + commit_succeeded = True + try: + run_command(['git', 'checkout', '-q', '-b', MERGE_BRANCH, self.svn_branch_name()]) + + for commit in commit_ids: + # We're on a different branch now, so convert "head" to the branch name. + commit = re.sub(r'(?i)head', branch_name, commit) + # FIXME: Once changed_files and create_patch are modified to separately handle each + # commit in a commit range, commit each cherry pick so they'll get dcommitted separately. + run_command(['git', 'cherry-pick', '--no-commit', commit]) + + run_command(['git', 'commit', '-m', message]) + output = self.push_local_commits_to_server() + except Exception, e: + log("COMMIT FAILED: " + str(e)) + output = "Commit failed." + commit_succeeded = False + finally: + # And then swap back to the original branch and clean up. + self.clean_working_directory() + run_command(['git', 'checkout', '-q', branch_name]) + self.delete_branch(MERGE_BRANCH) + + return output + def svn_commit_log(self, svn_revision): svn_revision = self.strip_r_from_svn_revision(svn_revision) return run_command(['git', 'svn', 'log', '-r', svn_revision]) @@ -560,13 +685,9 @@ class Git(SCM): return run_command(['git', 'merge-base', self.svn_branch_name(), 'HEAD']).strip() def svn_branch_name(self): - return Git.read_git_config('svn-remote.svn.fetch').split(':')[1] - - def create_patch_from_local_commit(self, commit_id): - return run_command(['git', 'diff', '--binary', commit_id + "^.." + commit_id]) - - def create_patch_since_local_commit(self, commit_id): - return run_command(['git', 'diff', '--binary', commit_id]) + # FIXME: This should so something like: Git.read_git_config('svn-remote.svn.fetch').split(':')[1] + # but that doesn't work if the git repo is tracking multiple svn branches. + return 'trunk' def commit_locally_with_message(self, message): run_command(['git', 'commit', '--all', '-F', '-'], input=message) diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py index c0a64d4..5a2c094 100644 --- a/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py @@ -27,7 +27,10 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from __future__ import with_statement + import base64 +import codecs import getpass import os import os.path @@ -49,23 +52,39 @@ from webkitpy.common.system.executive import Executive, run_command, ScriptError # Perhaps through some SCMTest base-class which both SVNTest and GitTest inherit from. # FIXME: This should be unified into one of the executive.py commands! +# Callers could use run_and_throw_if_fail(args, cwd=cwd, quiet=True) def run_silent(args, cwd=None): + # Note: Not thread safe: http://bugs.python.org/issue2320 process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) process.communicate() # ignore output exit_code = process.wait() if exit_code: raise ScriptError('Failed to run "%s" exit_code: %d cwd: %s' % (args, exit_code, cwd)) -def write_into_file_at_path(file_path, contents): - file = open(file_path, 'w') - file.write(contents) - file.close() -def read_from_path(file_path): - file = open(file_path, 'r') - contents = file.read() - file.close() - return contents +def write_into_file_at_path(file_path, contents, encoding="utf-8"): + with codecs.open(file_path, "w", encoding) as file: + file.write(contents) + + +def read_from_path(file_path, encoding="utf-8"): + with codecs.open(file_path, "r", encoding) as file: + return file.read() + + +def _make_diff(command, *args): + # We use this wrapper to disable output decoding. diffs should be treated as + # binary files since they may include text files of multiple differnet encodings. + return run_command([command, "diff"] + list(args), decode_output=False) + + +def _svn_diff(*args): + return _make_diff("svn", *args) + + +def _git_diff(*args): + return _make_diff("git", *args) + # Exists to share svn repository creation code between the git and svn tests class SVNTestRepository: @@ -103,7 +122,11 @@ class SVNTestRepository: cls._svn_add("test_file2") cls._svn_commit("third commit") - write_into_file_at_path("test_file", "test1test2test3\ntest4\n") + # This 4th commit is used to make sure that our patch file handling + # code correctly treats patches as binary and does not attempt to + # decode them assuming they're utf-8. + write_into_file_at_path("test_file", u"latin1 test: \u00A0\n", "latin1") + write_into_file_at_path("test_file2", u"utf-8 test: \u00A0\n", "utf-8") cls._svn_commit("fourth commit") # svn does not seem to update after commit as I would expect. @@ -122,6 +145,19 @@ class SVNTestRepository: test_object.svn_checkout_path = tempfile.mkdtemp(suffix="svn_test_checkout") run_command(['svn', 'checkout', '--quiet', test_object.svn_repo_url, test_object.svn_checkout_path]) + # Create and checkout a trunk dir to match the standard svn configuration to match git-svn's expectations + os.chdir(test_object.svn_checkout_path) + os.mkdir('trunk') + cls._svn_add('trunk') + # We can add tags and branches as well if we ever need to test those. + cls._svn_commit('add trunk') + + # Change directory out of the svn checkout so we can delete the checkout directory. + # _setup_test_commits will CD back to the svn checkout directory. + os.chdir('/') + run_command(['rm', '-rf', test_object.svn_checkout_path]) + run_command(['svn', 'checkout', '--quiet', test_object.svn_repo_url + '/trunk', test_object.svn_checkout_path]) + cls._setup_test_commits(test_object) @classmethod @@ -181,15 +217,12 @@ svn: resource out of date; try updating # GitTest and SVNTest inherit from this so any test_ methods here will be run once for this class and then once for each subclass. class SCMTest(unittest.TestCase): def _create_patch(self, patch_contents): - patch_path = os.path.join(self.svn_checkout_path, 'patch.diff') - write_into_file_at_path(patch_path, patch_contents) - patch = {} - patch['bug_id'] = '12345' - patch['url'] = 'file://%s' % urllib.pathname2url(patch_path) + # FIXME: This code is brittle if the Attachment API changes. + attachment = Attachment({"bug_id": 12345}, None) + attachment.contents = lambda: patch_contents - attachment = Attachment(patch, None) # FIXME: This is a hack, scm.py shouldn't be fetching attachment data. joe_cool = Committer(name="Joe Cool", email_or_emails=None) - attachment._reviewer = joe_cool + attachment.reviewer = lambda: joe_cool return attachment @@ -202,15 +235,6 @@ class SCMTest(unittest.TestCase): # Tests which both GitTest and SVNTest should run. # FIXME: There must be a simpler way to add these w/o adding a wrapper method to both subclasses - def _shared_test_commit_with_message(self, username="dbates@webkit.org"): - write_into_file_at_path('test_file', 'more test content') - commit_text = self.scm.commit_with_message("another test commit", username) - self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '5') - - self.scm.dryrun = True - write_into_file_at_path('test_file', 'still more test content') - commit_text = self.scm.commit_with_message("yet another test commit", username) - self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '0') def _shared_test_changed_files(self): write_into_file_at_path("test_file", "changed content") @@ -248,19 +272,22 @@ class SCMTest(unittest.TestCase): def _shared_test_changed_files_for_revision(self): # SVN reports directory changes, Git does not. - changed_files = self.scm.changed_files_for_revision(2) + changed_files = self.scm.changed_files_for_revision(3) if "test_dir" in changed_files: changed_files.remove("test_dir") self.assertEqual(changed_files, ["test_dir/test_file3", "test_file"]) - self.assertEqual(sorted(self.scm.changed_files_for_revision(3)), sorted(["test_file", "test_file2"])) # Git and SVN return different orders. - self.assertEqual(self.scm.changed_files_for_revision(4), ["test_file"]) + self.assertEqual(sorted(self.scm.changed_files_for_revision(4)), sorted(["test_file", "test_file2"])) # Git and SVN return different orders. + self.assertEqual(self.scm.changed_files_for_revision(2), ["test_file"]) def _shared_test_contents_at_revision(self): - self.assertEqual(self.scm.contents_at_revision("test_file", 2), "test1test2") - self.assertEqual(self.scm.contents_at_revision("test_file", 3), "test1test2test3\n") - self.assertEqual(self.scm.contents_at_revision("test_file", 4), "test1test2test3\ntest4\n") + self.assertEqual(self.scm.contents_at_revision("test_file", 3), "test1test2") + self.assertEqual(self.scm.contents_at_revision("test_file", 4), "test1test2test3\n") + + # Verify that contents_at_revision returns a byte array, aka str(): + self.assertEqual(self.scm.contents_at_revision("test_file", 5), u"latin1 test: \u00A0\n".encode("latin1")) + self.assertEqual(self.scm.contents_at_revision("test_file2", 5), u"utf-8 test: \u00A0\n".encode("utf-8")) - self.assertEqual(self.scm.contents_at_revision("test_file2", 3), "second file") + self.assertEqual(self.scm.contents_at_revision("test_file2", 4), "second file") # Files which don't exist: # Currently we raise instead of returning None because detecting the difference between # "file not found" and any other error seems impossible with svn (git seems to expose such through the return code). @@ -268,21 +295,21 @@ class SCMTest(unittest.TestCase): self.assertRaises(ScriptError, self.scm.contents_at_revision, "does_not_exist", 2) def _shared_test_committer_email_for_revision(self): - self.assertEqual(self.scm.committer_email_for_revision(2), getpass.getuser()) # Committer "email" will be the current user + self.assertEqual(self.scm.committer_email_for_revision(3), getpass.getuser()) # Committer "email" will be the current user def _shared_test_reverse_diff(self): self._setup_webkittools_scripts_symlink(self.scm) # Git's apply_reverse_diff uses resolve-ChangeLogs # Only test the simple case, as any other will end up with conflict markers. - self.scm.apply_reverse_diff('4') + self.scm.apply_reverse_diff('5') self.assertEqual(read_from_path('test_file'), "test1test2test3\n") def _shared_test_diff_for_revision(self): # Patch formats are slightly different between svn and git, so just regexp for things we know should be there. - r3_patch = self.scm.diff_for_revision(3) + r3_patch = self.scm.diff_for_revision(4) self.assertTrue(re.search('test3', r3_patch)) self.assertFalse(re.search('test4', r3_patch)) self.assertTrue(re.search('test2', r3_patch)) - self.assertTrue(re.search('test2', self.scm.diff_for_revision(2))) + self.assertTrue(re.search('test2', self.scm.diff_for_revision(3))) def _shared_test_svn_apply_git_patch(self): self._setup_webkittools_scripts_symlink(self.scm) @@ -308,7 +335,7 @@ HcmV?d00001 """ self.checkout.apply_patch(self._create_patch(git_binary_addition)) - added = read_from_path('fizzbuzz7.gif') + added = read_from_path('fizzbuzz7.gif', encoding=None) self.assertEqual(512, len(added)) self.assertTrue(added.startswith('GIF89a')) self.assertTrue('fizzbuzz7.gif' in self.scm.changed_files()) @@ -336,7 +363,7 @@ ptUl-ZG<%a~#LwkIWv&q!KSCH7tQ8cJDiw+|GV?MN)RjY50RTb-xvT&H """ self.checkout.apply_patch(self._create_patch(git_binary_modification)) - modified = read_from_path('fizzbuzz7.gif') + modified = read_from_path('fizzbuzz7.gif', encoding=None) self.assertEqual('foobar\n', modified) self.assertTrue('fizzbuzz7.gif' in self.scm.changed_files()) @@ -473,6 +500,12 @@ class SVNTest(SCMTest): def tearDown(self): SVNTestRepository.tear_down(self) + def test_detect_scm_system_relative_url(self): + scm = detect_scm_system(".") + # I wanted to assert that we got the right path, but there was some + # crazy magic with temp folder names that I couldn't figure out. + self.assertTrue(scm.checkout_root) + def test_create_patch_is_full_patch(self): test_dir_path = os.path.join(self.svn_checkout_path, "test_dir2") os.mkdir(test_dir_path) @@ -518,25 +551,35 @@ Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA== self._setup_webkittools_scripts_symlink(self.scm) patch_file = self._create_patch(patch_contents) self.checkout.apply_patch(patch_file) - actual_contents = read_from_path("test_file.swf") + actual_contents = read_from_path("test_file.swf", encoding=None) self.assertEqual(actual_contents, expected_contents) def test_apply_svn_patch(self): scm = detect_scm_system(self.svn_checkout_path) - patch = self._create_patch(run_command(['svn', 'diff', '-r4:3'])) + patch = self._create_patch(_svn_diff("-r5:4")) self._setup_webkittools_scripts_symlink(scm) Checkout(scm).apply_patch(patch) def test_apply_svn_patch_force(self): scm = detect_scm_system(self.svn_checkout_path) - patch = self._create_patch(run_command(['svn', 'diff', '-r2:4'])) + patch = self._create_patch(_svn_diff("-r3:5")) self._setup_webkittools_scripts_symlink(scm) self.assertRaises(ScriptError, Checkout(scm).apply_patch, patch, force=True) def test_commit_logs(self): # Commits have dates and usernames in them, so we can't just direct compare. self.assertTrue(re.search('fourth commit', self.scm.last_svn_commit_log())) - self.assertTrue(re.search('second commit', self.scm.svn_commit_log(2))) + self.assertTrue(re.search('second commit', self.scm.svn_commit_log(3))) + + def _shared_test_commit_with_message(self, username=None): + write_into_file_at_path('test_file', 'more test content') + commit_text = self.scm.commit_with_message("another test commit", username) + self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '6') + + self.scm.dryrun = True + write_into_file_at_path('test_file', 'still more test content') + commit_text = self.scm.commit_with_message("yet another test commit", username) + self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '0') def test_commit_text_parsing(self): self._shared_test_commit_with_message() @@ -595,7 +638,7 @@ class GitTest(SCMTest): def _setup_git_clone_of_svn_repository(self): self.git_checkout_path = tempfile.mkdtemp(suffix="git_test_checkout") # --quiet doesn't make git svn silent, so we use run_silent to redirect output - run_silent(['git', 'svn', '--quiet', 'clone', self.svn_repo_url, self.git_checkout_path]) + run_silent(['git', 'svn', 'clone', '-T', 'trunk', self.svn_repo_url, self.git_checkout_path]) def _tear_down_git_clone_of_svn_repository(self): run_command(['rm', '-rf', self.git_checkout_path]) @@ -657,8 +700,8 @@ class GitTest(SCMTest): test_file = os.path.join(self.git_checkout_path, 'test_file') write_into_file_at_path(test_file, 'foo') - diff_to_common_base = run_command(['git', 'diff', self.scm.svn_branch_name() + '..']) - diff_to_merge_base = run_command(['git', 'diff', self.scm.svn_merge_base()]) + diff_to_common_base = _git_diff(self.scm.svn_branch_name() + '..') + diff_to_merge_base = _git_diff(self.scm.svn_merge_base()) self.assertFalse(re.search(r'foo', diff_to_common_base)) self.assertTrue(re.search(r'foo', diff_to_merge_base)) @@ -711,18 +754,126 @@ class GitTest(SCMTest): # We carefullly pick a diff which does not have a directory addition # as currently svn-apply will error out when trying to remove directories # in Git: https://bugs.webkit.org/show_bug.cgi?id=34871 - patch = self._create_patch(run_command(['git', 'diff', 'HEAD..HEAD^'])) + patch = self._create_patch(_git_diff('HEAD..HEAD^')) self._setup_webkittools_scripts_symlink(scm) Checkout(scm).apply_patch(patch) def test_apply_git_patch_force(self): scm = detect_scm_system(self.git_checkout_path) - patch = self._create_patch(run_command(['git', 'diff', 'HEAD~2..HEAD'])) + patch = self._create_patch(_git_diff('HEAD~2..HEAD')) self._setup_webkittools_scripts_symlink(scm) self.assertRaises(ScriptError, Checkout(scm).apply_patch, patch, force=True) def test_commit_text_parsing(self): - self._shared_test_commit_with_message() + write_into_file_at_path('test_file', 'more test content') + self.scm.commit_locally_with_message("another test commit") + commit_text = self.scm.commit_with_message("another test commit") + self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '6') + + self.scm.dryrun = True + write_into_file_at_path('test_file', 'still more test content') + self.scm.commit_locally_with_message("yet another test commit") + commit_text = self.scm.commit_with_message("yet another test commit") + self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '0') + + def _one_local_commit_plus_working_copy_changes(self): + write_into_file_at_path('test_file_commit1', 'more test content') + run_command(['git', 'add', 'test_file_commit1']) + self.scm.commit_locally_with_message("another test commit") + + write_into_file_at_path('test_file_commit2', 'still more test content') + run_command(['git', 'add', 'test_file_commit2']) + + def test_commit_with_message_working_copy_only(self): + write_into_file_at_path('test_file_commit1', 'more test content') + run_command(['git', 'add', 'test_file_commit1']) + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("yet another test commit") + + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + + def test_commit_with_message_squashed(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("yet another test commit", squash=True) + + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + + def _two_local_commits(self): + write_into_file_at_path('test_file_commit1', 'more test content') + run_command(['git', 'add', 'test_file_commit1']) + self.scm.commit_locally_with_message("another test commit") + + write_into_file_at_path('test_file_commit2', 'still more test content') + run_command(['git', 'add', 'test_file_commit2']) + self.scm.commit_locally_with_message("yet another test commit") + + def test_commit_with_message_git_commit(self): + self._two_local_commits() + + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("another test commit", git_commit="HEAD^") + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + self.assertFalse(re.search(r'test_file_commit2', svn_log)) + + def test_commit_with_message_git_commit_range(self): + self._two_local_commits() + + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("another test commit", git_commit="HEAD~2..HEAD") + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + + def test_commit_with_message_multiple_local_commits(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit"]) + + def test_commit_with_message_multiple_local_commits_and_working_copy(self): + self._two_local_commits() + write_into_file_at_path('test_file_commit1', 'working copy change') + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit"]) + + def test_commit_with_message_git_commit_and_working_copy(self): + self._two_local_commits() + write_into_file_at_path('test_file_commit1', 'working copy change') + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.commit_with_message, ["another test commit", 'git_commit="HEAD^"']) + + def test_commit_with_message_multiple_local_commits_no_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("yet another test commit", squash=False) + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + self.assertFalse(re.search(r'test_file_commit1', svn_log)) + + svn_log = run_command(['git', 'svn', 'log', '--limit=2', '--verbose']) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) + + def test_commit_with_message_multiple_local_commits_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + commit_text = scm.commit_with_message("yet another test commit", squash=True) + self.assertEqual(scm.svn_revision_from_commit_text(commit_text), '6') + + svn_log = run_command(['git', 'svn', 'log', '--limit=1', '--verbose']) + self.assertTrue(re.search(r'test_file_commit2', svn_log)) + self.assertTrue(re.search(r'test_file_commit1', svn_log)) def test_reverse_diff(self): self._shared_test_reverse_diff() @@ -733,13 +884,66 @@ class GitTest(SCMTest): def test_svn_apply_git_patch(self): self._shared_test_svn_apply_git_patch() + def test_create_patch_local_plus_working_copy(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.create_patch) + + def test_create_patch_multiple_local_commits(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.create_patch) + + def test_create_patch_squashed(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(squash=True) + self.assertTrue(re.search(r'test_file_commit2', patch)) + self.assertTrue(re.search(r'test_file_commit1', patch)) + + def test_create_patch_not_squashed(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(squash=False) + self.assertTrue(re.search(r'test_file_commit2', patch)) + self.assertFalse(re.search(r'test_file_commit1', patch)) + + def test_create_patch_git_commit(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(git_commit="HEAD^") + self.assertTrue(re.search(r'test_file_commit1', patch)) + self.assertFalse(re.search(r'test_file_commit2', patch)) + + def test_create_patch_git_commit_range(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(git_commit="HEAD~2..HEAD") + self.assertTrue(re.search(r'test_file_commit2', patch)) + self.assertTrue(re.search(r'test_file_commit1', patch)) + + def test_create_patch_multiple_local_commits_no_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(squash=False) + # FIXME: It's weird that with squash=False, create_patch/changed_files ignores local commits, + # but commit_with_message commits them. + self.assertTrue(patch == "") + + def test_create_patch_multiple_local_commits_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + patch = scm.create_patch(squash=True) + self.assertTrue(re.search(r'test_file_commit2', patch)) + self.assertTrue(re.search(r'test_file_commit1', patch)) + def test_create_binary_patch(self): # Create a git binary patch and check the contents. scm = detect_scm_system(self.git_checkout_path) test_file_name = 'binary_file' test_file_path = os.path.join(self.git_checkout_path, test_file_name) file_contents = ''.join(map(chr, range(256))) - write_into_file_at_path(test_file_path, file_contents) + write_into_file_at_path(test_file_path, file_contents, encoding=None) run_command(['git', 'add', test_file_name]) patch = scm.create_patch() self.assertTrue(re.search(r'\nliteral 0\n', patch)) @@ -749,19 +953,68 @@ class GitTest(SCMTest): run_command(['git', 'rm', '-f', test_file_name]) self._setup_webkittools_scripts_symlink(scm) self.checkout.apply_patch(self._create_patch(patch)) - self.assertEqual(file_contents, read_from_path(test_file_path)) + self.assertEqual(file_contents, read_from_path(test_file_path, encoding=None)) # Check if we can create a patch from a local commit. - write_into_file_at_path(test_file_path, file_contents) + write_into_file_at_path(test_file_path, file_contents, encoding=None) run_command(['git', 'add', test_file_name]) run_command(['git', 'commit', '-m', 'binary diff']) - patch_from_local_commit = scm.create_patch_from_local_commit('HEAD') + patch_from_local_commit = scm.create_patch('HEAD') self.assertTrue(re.search(r'\nliteral 0\n', patch_from_local_commit)) self.assertTrue(re.search(r'\nliteral 256\n', patch_from_local_commit)) - patch_since_local_commit = scm.create_patch_since_local_commit('HEAD^1') - self.assertTrue(re.search(r'\nliteral 0\n', patch_since_local_commit)) - self.assertTrue(re.search(r'\nliteral 256\n', patch_since_local_commit)) - self.assertEqual(patch_from_local_commit, patch_since_local_commit) + + def test_changed_files_local_plus_working_copy(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.changed_files) + + def test_changed_files_multiple_local_commits(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + self.assertRaises(ScriptError, scm.changed_files) + + def test_changed_files_squashed(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(squash=True) + self.assertTrue('test_file_commit2' in files) + self.assertTrue('test_file_commit1' in files) + + def test_changed_files_not_squashed(self): + self._one_local_commit_plus_working_copy_changes() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(squash=False) + self.assertTrue('test_file_commit2' in files) + self.assertFalse('test_file_commit1' in files) + + def test_changed_files_git_commit(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(git_commit="HEAD^") + self.assertTrue('test_file_commit1' in files) + self.assertFalse('test_file_commit2' in files) + + def test_changed_files_git_commit_range(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(git_commit="HEAD~2..HEAD") + self.assertTrue('test_file_commit1' in files) + self.assertTrue('test_file_commit2' in files) + + def test_changed_files_multiple_local_commits_no_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(squash=False) + # FIXME: It's weird that with squash=False, create_patch/changed_files ignores local commits, + # but commit_with_message commits them. + self.assertTrue(len(files) == 0) + + def test_changed_files_multiple_local_commits_squash(self): + self._two_local_commits() + scm = detect_scm_system(self.git_checkout_path) + files = scm.changed_files(squash=True) + self.assertTrue('test_file_commit2' in files) + self.assertTrue('test_file_commit1' in files) def test_changed_files(self): self._shared_test_changed_files() diff --git a/WebKitTools/Scripts/webkitpy/common/config/committers.py b/WebKitTools/Scripts/webkitpy/common/config/committers.py index a92dbd3..56887ab 100644 --- a/WebKitTools/Scripts/webkitpy/common/config/committers.py +++ b/WebKitTools/Scripts/webkitpy/common/config/committers.py @@ -86,7 +86,6 @@ committers_unable_to_review = [ Committer("Carol Szabo", "carol.szabo@nokia.com"), Committer("Chang Shu", "Chang.Shu@nokia.com"), Committer("Chris Fleizach", "cfleizach@apple.com"), - Committer("Chris Jerdonek", "cjerdonek@webkit.org", "cjerdonek"), Committer("Chris Marrin", "cmarrin@apple.com", "cmarrin"), Committer("Chris Petersen", "cpetersen@apple.com", "cpetersen"), Committer("Christian Dywan", ["christian@twotoasts.de", "christian@webkit.org"]), @@ -94,6 +93,7 @@ committers_unable_to_review = [ Committer("Csaba Osztrogonac", "ossy@webkit.org", "ossy"), Committer("David Smith", ["catfish.man@gmail.com", "dsmith@webkit.org"], "catfishman"), Committer("Dean Jackson", "dino@apple.com", "dino"), + Committer("Diego Gonzalez", ["diegohcg@webkit.org", "diego.gonzalez@openbossa.org"], "diegohcg"), Committer("Dirk Pranke", "dpranke@chromium.org"), Committer("Drew Wilson", "atwilson@chromium.org", "atwilson"), Committer("Dumitru Daniliuc", "dumi@chromium.org", "dumi"), @@ -101,6 +101,8 @@ committers_unable_to_review = [ Committer("Enrica Casucci", "enrica@apple.com"), Committer("Erik Arvidsson", "arv@chromium.org", "arv"), Committer("Eric Roman", "eroman@chromium.org", "eroman"), + Committer("Evan Martin", "evan@chromium.org", "evmar"), + Committer("Evan Stade", "estade@chromium.org", "estade"), Committer("Feng Qian", "feng@chromium.org"), Committer("Fumitoshi Ukai", "ukai@chromium.org", "ukai"), Committer("Gabor Loki", "loki@webkit.org", "loki04"), @@ -153,13 +155,13 @@ committers_unable_to_review = [ Committer("Ryosuke Niwa", "rniwa@webkit.org", "rniwa"), Committer("Scott Violet", "sky@chromium.org", "sky"), Committer("Stephen White", "senorblanco@chromium.org", "senorblanco"), - Committer("Steve Block", "steveblock@google.com"), Committer("Tony Chang", "tony@chromium.org", "tony^work"), Committer("Trey Matteson", "trey@usa.net", "trey"), Committer("Tristan O'Tierney", ["tristan@otierney.net", "tristan@apple.com"]), Committer("Victor Wang", "victorw@chromium.org"), Committer("Vitaly Repeshko", "vitalyr@chromium.org"), Committer("William Siegrist", "wsiegrist@apple.com", "wms"), + Committer("Xiaomei Ji", "xji@chromium.org", "xji"), Committer("Yael Aharon", "yael.aharon@nokia.com"), Committer("Yaar Schnitman", ["yaar@chromium.org", "yaar@google.com"]), Committer("Yong Li", ["yong.li@torchmobile.com", "yong.li.webkit@gmail.com"], "yong"), @@ -191,6 +193,7 @@ reviewers_list = [ Reviewer("Brady Eidson", "beidson@apple.com", "bradee-oh"), Reviewer("Cameron Zwarich", ["zwarich@apple.com", "cwzwarich@apple.com", "cwzwarich@webkit.org"]), Reviewer("Chris Blumenberg", "cblu@apple.com", "cblu"), + Reviewer("Chris Jerdonek", "cjerdonek@webkit.org", "cjerdonek"), Reviewer("Dan Bernstein", ["mitz@webkit.org", "mitz@apple.com"], "mitzpettel"), Reviewer("Daniel Bates", "dbates@webkit.org", "dydz"), Reviewer("Darin Adler", "darin@apple.com", "darin"), @@ -237,10 +240,11 @@ reviewers_list = [ Reviewer("Simon Fraser", "simon.fraser@apple.com", "smfr"), Reviewer("Simon Hausmann", ["hausmann@webkit.org", "hausmann@kde.org", "simon.hausmann@nokia.com"], "tronical"), Reviewer("Stephanie Lewis", "slewis@apple.com", "sundiamonde"), + Reviewer("Steve Block", "steveblock@google.com", "steveblock"), Reviewer("Steve Falkenburg", "sfalken@apple.com", "sfalken"), Reviewer("Tim Omernick", "timo@apple.com"), Reviewer("Timothy Hatcher", ["timothy@hatcher.name", "timothy@apple.com"], "xenon"), - Reviewer(u'Tor Arne Vestb\xf8', "vestbo@webkit.org", "torarne"), + Reviewer(u"Tor Arne Vestb\u00f8", "vestbo@webkit.org", "torarne"), Reviewer("Vicki Murley", "vicki@apple.com"), Reviewer("Xan Lopez", ["xan.lopez@gmail.com", "xan@gnome.org", "xan@webkit.org"], "xan"), Reviewer("Yury Semikhatsky", "yurys@chromium.org", "yurys"), diff --git a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py index 6920d67..4311a00 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py +++ b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py @@ -32,7 +32,7 @@ import os.path import re -import subprocess +import StringIO from datetime import datetime # used in timestamp() @@ -116,6 +116,10 @@ class Attachment(object): # depends on the current behavior. return self._attachment_dictionary.get("url") + def contents(self): + # FIXME: We shouldn't be grabbing at _bugzilla. + return self._bug._bugzilla.fetch_attachment_contents(self.id()) + def _validate_flag_value(self, flag): email = self._attachment_dictionary.get("%s_email" % flag) if not email: @@ -427,7 +431,16 @@ class Bugzilla(object): if flag['status'] == '+': attachment[result_key] = flag['setter'] + def _string_contents(self, soup): + # WebKit's bugzilla instance uses UTF-8. + # BeautifulSoup always returns Unicode strings, however + # the .string method returns a (unicode) NavigableString. + # NavigableString can confuse other parts of the code, so we + # convert from NavigableString to a real unicode() object using unicode(). + return unicode(soup.string) + def _parse_attachment_element(self, element, bug_id): + attachment = {} attachment['bug_id'] = bug_id attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1") @@ -435,9 +448,9 @@ class Bugzilla(object): attachment['id'] = int(element.find('attachid').string) # FIXME: No need to parse out the url here. attachment['url'] = self.attachment_url_for_id(attachment['id']) - attachment['name'] = unicode(element.find('desc').string) - attachment['attacher_email'] = str(element.find('attacher').string) - attachment['type'] = str(element.find('type').string) + attachment['name'] = self._string_contents(element.find('desc')) + attachment['attacher_email'] = self._string_contents(element.find('attacher')) + attachment['type'] = self._string_contents(element.find('type')) self._parse_attachment_flag( element, 'review', attachment, 'reviewer_email') self._parse_attachment_flag( @@ -448,10 +461,10 @@ class Bugzilla(object): soup = BeautifulSoup(page) bug = {} bug["id"] = int(soup.find("bug_id").string) - bug["title"] = unicode(soup.find("short_desc").string) - bug["reporter_email"] = str(soup.find("reporter").string) - bug["assigned_to_email"] = str(soup.find("assigned_to").string) - bug["cc_emails"] = [str(element.string) + bug["title"] = self._string_contents(soup.find("short_desc")) + bug["reporter_email"] = self._string_contents(soup.find("reporter")) + bug["assigned_to_email"] = self._string_contents(soup.find("assigned_to")) + bug["cc_emails"] = [self._string_contents(element) for element in soup.findAll('cc')] bug["attachments"] = [self._parse_attachment_element(element, bug["id"]) for element in soup.findAll('attachment')] return bug @@ -476,6 +489,12 @@ class Bugzilla(object): def fetch_bug(self, bug_id): return Bug(self.fetch_bug_dictionary(bug_id), self) + def fetch_attachment_contents(self, attachment_id): + attachment_url = self.attachment_url_for_id(attachment_id) + # We need to authenticate to download patches from security bugs. + self.authenticate() + return self.browser.open(attachment_url).read() + def _parse_bug_id_from_attachment_page(self, page): # The "Up" relation happens to point to the bug. up_link = BeautifulSoup(page).find('link', rel='Up') @@ -568,6 +587,7 @@ class Bugzilla(object): patch_name = "bug-%s-%s.patch" % (bug_id, timestamp()) else: patch_name ="%s.patch" % timestamp() + self.browser.add_file(patch_file_object, "text/plain", patch_name, @@ -575,7 +595,7 @@ class Bugzilla(object): def add_patch_to_bug(self, bug_id, - patch_file_object, + diff, description, comment_text=None, mark_for_review=False, @@ -594,6 +614,11 @@ class Bugzilla(object): self.browser.open("%sattachment.cgi?action=enter&bugid=%s" % ( self.bug_server_url, bug_id)) self.browser.select_form(name="entryform") + + # _fill_attachment_form expects a file-like object + # Patch files are already binary, so no encoding needed. + assert(isinstance(diff, str)) + patch_file_object = StringIO.StringIO(diff) self._fill_attachment_form(description, patch_file_object, mark_for_review=mark_for_review, @@ -628,7 +653,7 @@ class Bugzilla(object): bug_title, bug_description, component=None, - patch_file_object=None, + diff=None, patch_description=None, cc=None, blocked=None, @@ -653,11 +678,15 @@ class Bugzilla(object): if cc: self.browser["cc"] = cc if blocked: - self.browser["blocked"] = str(blocked) + self.browser["blocked"] = unicode(blocked) self.browser["short_desc"] = bug_title self.browser["comment"] = bug_description - if patch_file_object: + if diff: + # _fill_attachment_form expects a file-like object + # Patch files are already binary, so no encoding needed. + assert(isinstance(diff, str)) + patch_file_object = StringIO.StringIO(diff) self._fill_attachment_form( patch_description, patch_file_object, diff --git a/WebKitTools/Scripts/webkitpy/common/net/buildbot.py b/WebKitTools/Scripts/webkitpy/common/net/buildbot.py index 753e909..6c6ed43 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/buildbot.py +++ b/WebKitTools/Scripts/webkitpy/common/net/buildbot.py @@ -44,7 +44,7 @@ _log = get_logger(__file__) class Builder(object): def __init__(self, name, buildbot): - self._name = unicode(name) + self._name = name self._buildbot = buildbot self._builds_cache = {} self._revision_to_build_number = None @@ -223,12 +223,12 @@ class LayoutTestResults(object): parsed_results = {} tables = BeautifulSoup(page).findAll("table") for table in tables: - table_title = table.findPreviousSibling("p").string + table_title = unicode(table.findPreviousSibling("p").string) if table_title not in cls.expected_keys: # This Exception should only ever be hit if run-webkit-tests changes its results.html format. - raise Exception("Unhandled title: %s" % str(table_title)) + raise Exception("Unhandled title: %s" % table_title) # We might want to translate table titles into identifiers before storing. - parsed_results[table_title] = [row.find("a").string for row in table.findAll("tr")] + parsed_results[table_title] = [unicode(row.find("a").string) for row in table.findAll("tr")] return parsed_results @@ -319,7 +319,6 @@ class BuildBot(object): "Leopard", "Tiger", "Windows.*Build", - "Windows.*Debug.*Test", "GTK", "Qt", "Chromium", @@ -361,7 +360,7 @@ class BuildBot(object): # First cell is the name name_link = status_cells[0].find('a') - builder["name"] = name_link.string + builder["name"] = unicode(name_link.string) self._parse_last_build_cell(builder, status_cells[1]) self._parse_current_build_cell(builder, status_cells[2]) @@ -410,13 +409,13 @@ class BuildBot(object): return urllib2.urlopen(build_status_url) def _parse_twisted_file_row(self, file_row): - string_or_empty = lambda string: str(string) if string else "" + string_or_empty = lambda soup: unicode(soup.string) if soup.string else u"" file_cells = file_row.findAll('td') return { - "filename" : string_or_empty(file_cells[0].find("a").string), - "size" : string_or_empty(file_cells[1].string), - "type" : string_or_empty(file_cells[2].string), - "encoding" : string_or_empty(file_cells[3].string), + "filename": string_or_empty(file_cells[0].find("a")), + "size": string_or_empty(file_cells[1]), + "type": string_or_empty(file_cells[2]), + "encoding": string_or_empty(file_cells[3]), } def _parse_twisted_directory_listing(self, page): diff --git a/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py index f765f6e..5e04745 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py @@ -51,7 +51,7 @@ class BuilderTest(unittest.TestCase): def setUp(self): self.buildbot = BuildBot() - self.builder = Builder("Test Builder", self.buildbot) + self.builder = Builder(u"Test Builder \u2661", self.buildbot) self._install_fetch_build(lambda build_number: ["test1", "test2"]) def test_find_failure_transition(self): @@ -271,7 +271,6 @@ class BuildBotTest(unittest.TestCase): "Leopard", "Tiger", "Windows.*Build", - "Windows.*Debug.*Test", "GTK", "Qt", "Chromium", @@ -286,7 +285,6 @@ class BuildBotTest(unittest.TestCase): {'name': u'SnowLeopard Intel Release (Tests)', }, {'name': u'Windows Release (Build)', }, {'name': u'Windows Debug (Build)', }, - {'name': u'Windows Debug (Tests)', }, {'name': u'GTK Linux 32-bit Release', }, {'name': u'GTK Linux 32-bit Debug', }, {'name': u'GTK Linux 64-bit Debug', }, diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py index a9e5b1a..9cc97f2 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py +++ b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py @@ -26,6 +26,7 @@ # (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 logging import os import re import stat @@ -50,23 +51,19 @@ class Rietveld(object): def __init__(self, executive, dryrun=False): self.dryrun = dryrun self._executive = executive - self._upload_py = upload.__file__ - # Chop off the last character so we modify permissions on the py file instead of the pyc. - if os.path.splitext(self._upload_py)[1] == ".pyc": - self._upload_py = self._upload_py[:-1] - os.chmod(self._upload_py, os.stat(self._upload_py).st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) def url_for_issue(self, codereview_issue): if not codereview_issue: return None return "%s%s" % (config.codereview_server_url, codereview_issue) - def post(self, message=None, codereview_issue=None, cc=None): + def post(self, diff, message=None, codereview_issue=None, cc=None): if not message: raise ScriptError("Rietveld requires a message.") args = [ - self._upload_py, + # First argument is empty string to mimic sys.argv. + "", "--assume_yes", "--server=%s" % config.codereview_server_host, "--message=%s" % message, @@ -80,10 +77,15 @@ class Rietveld(object): log("Would have run %s" % args) return - output = self._executive.run_and_throw_if_fail(args) - match = re.search("Issue created\. URL: " + - config.codereview_server_regex + - "(?P<codereview_issue>\d+)", - output) - if match: - return int(match.group('codereview_issue')) + # Set logging level to avoid rietveld's logging spew. + old_level_name = logging.getLogger().getEffectiveLevel() + logging.getLogger().setLevel(logging.ERROR) + + # Use RealMain instead of calling upload from the commandline so that + # we can pass in the diff ourselves. Otherwise, upload will just use + # git diff for git checkouts, which doesn't respect --squash and --git-commit. + issue, patchset = upload.RealMain(args[1:], data=diff) + + # Reset logging level to the original value. + logging.getLogger().setLevel(old_level_name) + return issue diff --git a/WebKitTools/Scripts/webkitpy/common/net/statusserver.py b/WebKitTools/Scripts/webkitpy/common/net/statusserver.py index e8987a9..d9b52a2 100644 --- a/WebKitTools/Scripts/webkitpy/common/net/statusserver.py +++ b/WebKitTools/Scripts/webkitpy/common/net/statusserver.py @@ -52,9 +52,9 @@ class StatusServer: if not patch: return if patch.bug_id(): - self.browser["bug_id"] = str(patch.bug_id()) + self.browser["bug_id"] = unicode(patch.bug_id()) if patch.id(): - self.browser["patch_id"] = str(patch.id()) + self.browser["patch_id"] = unicode(patch.id()) def _add_results_file(self, results_file): if not results_file: @@ -79,7 +79,7 @@ class StatusServer: update_svn_revision_url = "%s/update-svn-revision" % self.url self.browser.open(update_svn_revision_url) self.browser.select_form(name="update_svn_revision") - self.browser["number"] = str(svn_revision_number) + self.browser["number"] = unicode(svn_revision_number) self.browser["broken_bot"] = broken_bot return self.browser.submit().read() diff --git a/WebKitTools/Scripts/webkitpy/common/prettypatch.py b/WebKitTools/Scripts/webkitpy/common/prettypatch.py index 8157f9c..4e92a53 100644 --- a/WebKitTools/Scripts/webkitpy/common/prettypatch.py +++ b/WebKitTools/Scripts/webkitpy/common/prettypatch.py @@ -31,11 +31,15 @@ import tempfile class PrettyPatch(object): + # FIXME: PrettyPatch should not require checkout_root. def __init__(self, executive, checkout_root): self._executive = executive self._checkout_root = checkout_root def pretty_diff_file(self, diff): + # Diffs can contain multiple text files of different encodings + # so we always deal with them as byte arrays, not unicode strings. + assert(isinstance(diff, str)) pretty_diff = self.pretty_diff(diff) diff_file = tempfile.NamedTemporaryFile(suffix=".html") diff_file.write(pretty_diff) @@ -43,6 +47,11 @@ class PrettyPatch(object): return diff_file def pretty_diff(self, diff): + # pretify.rb will hang forever if given no input. + # Avoid the hang by returning an empty string. + if not diff: + return "" + pretty_patch_path = os.path.join(self._checkout_root, "BugsSite", "PrettyPatch") prettify_path = os.path.join(pretty_patch_path, "prettify.rb") @@ -52,4 +61,6 @@ class PrettyPatch(object): pretty_patch_path, prettify_path, ] - return self._executive.run_command(args, input=diff) + # 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) diff --git a/WebKitTools/Scripts/webkitpy/common/prettypatch_unittest.py b/WebKitTools/Scripts/webkitpy/common/prettypatch_unittest.py new file mode 100644 index 0000000..1307856 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/common/prettypatch_unittest.py @@ -0,0 +1,70 @@ +# Copyright (C) 2010 Google Inc. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (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 os.path +import unittest + +from webkitpy.common.system.executive import Executive +from webkitpy.common.prettypatch import PrettyPatch + + +class PrettyPatchTest(unittest.TestCase): + + _diff_with_multiple_encodings = """ +Index: utf8_test +=================================================================== +--- utf8_test\t(revision 0) ++++ utf8_test\t(revision 0) +@@ -0,0 +1 @@ ++utf-8 test: \xc2\xa0 +Index: latin1_test +=================================================================== +--- latin1_test\t(revision 0) ++++ latin1_test\t(revision 0) +@@ -0,0 +1 @@ ++latin1 test: \xa0 +""" + + def _webkit_root(self): + webkitpy_common = os.path.dirname(__file__) + webkitpy = os.path.dirname(webkitpy_common) + scripts = os.path.dirname(webkitpy) + webkit_tools = os.path.dirname(scripts) + webkit_root = os.path.dirname(webkit_tools) + return webkit_root + + def test_pretty_diff_encodings(self): + pretty_patch = PrettyPatch(Executive(), self._webkit_root()) + pretty = pretty_patch.pretty_diff(self._diff_with_multiple_encodings) + self.assertTrue(pretty) # We got some output + self.assertTrue(isinstance(pretty, str)) # It's a byte array, not unicode + + def test_pretty_print_empty_string(self): + # Make sure that an empty diff does not hang the process. + pretty_patch = PrettyPatch(Executive(), self._webkit_root()) + self.assertEqual(pretty_patch.pretty_diff(""), "") diff --git a/WebKitTools/Scripts/webkitpy/common/system/autoinstall.py b/WebKitTools/Scripts/webkitpy/common/system/autoinstall.py index 32fd2cf..9adab29 100755 --- a/WebKitTools/Scripts/webkitpy/common/system/autoinstall.py +++ b/WebKitTools/Scripts/webkitpy/common/system/autoinstall.py @@ -30,6 +30,10 @@ """Support for automatically downloading Python packages from an URL.""" + +from __future__ import with_statement + +import codecs import logging import new import os @@ -114,7 +118,7 @@ class AutoInstaller(object): os.makedirs(path) - def _write_file(self, path, text): + def _write_file(self, path, text, encoding): """Create a file at the given path with given text. This method overwrites any existing file. @@ -122,11 +126,8 @@ class AutoInstaller(object): """ _log.debug("Creating file...") _log.debug(' "%s"' % path) - file = open(path, "w") - try: + with codecs.open(path, "w", encoding) as file: file.write(text) - finally: - file.close() def _set_up_target_dir(self, target_dir, append_to_search_path, make_package): @@ -154,7 +155,7 @@ class AutoInstaller(object): if not os.path.exists(init_path): text = ("# This file is required for Python to search this " "directory for modules.\n") - self._write_file(init_path, text) + self._write_file(init_path, text, "ascii") def _create_scratch_directory_inner(self, prefix): """Create a scratch directory without exception handling. @@ -216,11 +217,8 @@ class AutoInstaller(object): _log.debug("No URL file found.") return False - file = open(version_path, "r") - try: + with codecs.open(version_path, "r", "utf-8") as file: version = file.read() - finally: - file.close() return version.strip() == url.strip() @@ -231,7 +229,7 @@ class AutoInstaller(object): _log.debug(' URL: "%s"' % url) _log.debug(' To: "%s"' % version_path) - self._write_file(version_path, url) + self._write_file(version_path, url, "utf-8") def _extract_targz(self, path, scratch_dir): # tarfile.extractall() extracts to a path without the @@ -284,6 +282,8 @@ class AutoInstaller(object): # Otherwise, it is a file. try: + # We open this file w/o encoding, as we're reading/writing + # the raw byte-stream from the zip file. outfile = open(path, 'wb') except IOError, err: # Not all zip files seem to list the directories explicitly, @@ -384,9 +384,8 @@ class AutoInstaller(object): self._log_transfer("Starting download...", url, target_path) - stream = file(target_path, "wb") - bytes = self._download_to_stream(url, stream) - stream.close() + with open(target_path, "wb") as stream: + bytes = self._download_to_stream(url, stream) _log.debug("Downloaded %s bytes." % bytes) diff --git a/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging.py b/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging.py index ba1c5eb..9e6b529 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging.py +++ b/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging.py @@ -30,24 +30,30 @@ # WebKit's Python module for logging # This module is now deprecated in favor of python's built-in logging.py. +import codecs import os import sys + def log(string): print >> sys.stderr, string + def error(string): log("ERROR: %s" % string) exit(1) + # Simple class to split output between multiple destinations class tee: def __init__(self, *files): self.files = files - def write(self, string): + # Callers should pass an already encoded string for writing. + def write(self, bytes): for file in self.files: - file.write(string) + file.write(bytes) + class OutputTee: def __init__(self): @@ -71,7 +77,7 @@ class OutputTee: (log_directory, log_name) = os.path.split(log_path) if log_directory and not os.path.exists(log_directory): os.makedirs(log_directory) - return open(log_path, 'a+') + return codecs.open(log_path, "a+", "utf-8") def _tee_outputs_to_files(self, files): if not self._original_stdout: diff --git a/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging_unittest.py b/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging_unittest.py index 2b71803..3778162 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging_unittest.py @@ -27,7 +27,6 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import os -import subprocess import StringIO import tempfile import unittest diff --git a/WebKitTools/Scripts/webkitpy/common/system/executive.py b/WebKitTools/Scripts/webkitpy/common/system/executive.py index b6126e4..11eb051 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/executive.py +++ b/WebKitTools/Scripts/webkitpy/common/system/executive.py @@ -87,10 +87,20 @@ def run_command(*args, **kwargs): class Executive(object): + def _should_close_fds(self): + # We need to pass close_fds=True to work around Python bug #2320 + # (otherwise we can hang when we kill DumpRenderTree when we are running + # multiple threads). See http://bugs.python.org/issue2320 . + # Note that close_fds isn't supported on Windows, but this bug only + # shows up on Mac and Linux. + return sys.platform not in ('win32', 'cygwin') + def _run_command_with_teed_output(self, args, teed_output): + args = map(unicode, args) # Popen will throw an exception if args are non-strings (like int()) child_process = subprocess.Popen(args, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) + stderr=subprocess.STDOUT, + close_fds=self._should_close_fds()) # Use our own custom wait loop because Popen ignores a tee'd # stderr/stdout. @@ -98,15 +108,24 @@ class Executive(object): while True: output_line = child_process.stdout.readline() if output_line == "" and child_process.poll() != None: + # poll() is not threadsafe and can throw OSError due to: + # http://bugs.python.org/issue1731717 return child_process.poll() + # We assume that the child process wrote to us in utf-8, + # so no re-encoding is necessary before writing here. teed_output.write(output_line) - def run_and_throw_if_fail(self, args, quiet=False): + # FIXME: Remove this deprecated method and move callers to run_command. + # FIXME: This method is a hack to allow running command which both + # capture their output and print out to stdin. Useful for things + # like "build-webkit" where we want to display to the user that we're building + # but still have the output to stuff into a log file. + def run_and_throw_if_fail(self, args, quiet=False, decode_output=True): # Cache the child's output locally so it can be used for error reports. child_out_file = StringIO.StringIO() tee_stdout = sys.stdout if quiet: - dev_null = open(os.devnull, "w") + dev_null = open(os.devnull, "w") # FIXME: Does this need an encoding? tee_stdout = dev_null child_stdout = tee(child_out_file, tee_stdout) exit_code = self._run_command_with_teed_output(args, child_stdout) @@ -116,6 +135,10 @@ class Executive(object): child_output = child_out_file.getvalue() child_out_file.close() + # We assume the child process output utf-8 + if decode_output: + child_output = child_output.decode("utf-8") + if exit_code: raise ScriptError(script_args=args, exit_code=exit_code, @@ -140,17 +163,39 @@ class Executive(object): return 2 def kill_process(self, pid): + """Attempts to kill the given pid. + Will fail silently if pid does not exist or insufficient permisssions.""" if platform.system() == "Windows": # According to http://docs.python.org/library/os.html # os.kill isn't available on Windows. However, when I tried it # using Cygwin, it worked fine. We should investigate whether # we need this platform specific code here. - subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)), - stdin=open(os.devnull, 'r'), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + command = ["taskkill.exe", "/f", "/pid", str(pid)] + # taskkill will exit 128 if the process is not found. + self.run_command(command, error_handler=self.ignore_error) return - os.kill(pid, signal.SIGKILL) + try: + os.kill(pid, signal.SIGKILL) + except OSError, e: + # FIXME: We should make non-silent failure an option. + pass + + def kill_all(self, process_name): + """Attempts to kill processes matching process_name. + Will fail silently if no process are found.""" + if platform.system() == "Windows": + # We might want to automatically append .exe? + command = ["taskkill.exe", "/f", "/im", process_name] + # taskkill will exit 128 if the process is not found. + self.run_command(command, error_handler=self.ignore_error) + return + + # FIXME: This is inconsistent that kill_all uses TERM and kill_process + # uses KILL. Windows is always using /f (which seems like -KILL). + # We should pick one mode, or add support for switching between them. + # Note: Mac OS X 10.6 requires -SIGNALNAME before -u USER + command = ["killall", "-TERM", "-u", os.getenv("USER"), process_name] + self.run_command(command, error_handler=self.ignore_error) # Error handlers do not need to be static methods once all callers are # updated to use an Executive object. @@ -163,38 +208,51 @@ class Executive(object): def ignore_error(error): pass - # FIXME: This should be merged with run_and_throw_if_fail + def _compute_stdin(self, input): + """Returns (stdin, string_to_communicate)""" + # FIXME: We should be returning /dev/null for stdin + # or closing stdin after process creation to prevent + # child processes from getting input from the user. + if not input: + return (None, None) + if hasattr(input, "read"): # Check if the input is a file. + return (input, None) # Assume the file is in the right encoding. + + # Popen in Python 2.5 and before does not automatically encode unicode objects. + # http://bugs.python.org/issue5290 + # See https://bugs.webkit.org/show_bug.cgi?id=37528 + # for an example of a regresion caused by passing a unicode string directly. + # FIXME: We may need to encode differently on different platforms. + if isinstance(input, unicode): + input = input.encode("utf-8") + return (subprocess.PIPE, input) + # FIXME: run_and_throw_if_fail should be merged into this method. def run_command(self, args, cwd=None, input=None, error_handler=None, return_exit_code=False, - return_stderr=True): - if hasattr(input, 'read'): # Check if the input is a file. - stdin = input - string_to_communicate = None - else: - stdin = None - if input: - stdin = subprocess.PIPE - # string_to_communicate seems to need to be a str for proper - # communication with shell commands. - # See https://bugs.webkit.org/show_bug.cgi?id=37528 - # For an example of a regresion caused by passing a unicode string through. - string_to_communicate = str(input) - if return_stderr: - stderr = subprocess.STDOUT - else: - stderr = None + return_stderr=True, + decode_output=True): + """Popen wrapper for convenience and to work around python bugs.""" + args = map(unicode, args) # Popen will throw an exception if args are non-strings (like int()) + stdin, string_to_communicate = self._compute_stdin(input) + stderr = subprocess.STDOUT if return_stderr else None process = subprocess.Popen(args, stdin=stdin, stdout=subprocess.PIPE, stderr=stderr, - cwd=cwd) + cwd=cwd, + close_fds=self._should_close_fds()) output = process.communicate(string_to_communicate)[0] + # run_command automatically decodes to unicode() unless explicitly told not to. + if decode_output: + output = output.decode("utf-8") + # wait() is not threadsafe and can throw OSError due to: + # http://bugs.python.org/issue1731717 exit_code = process.wait() if return_exit_code: diff --git a/WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py b/WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py index ac380f8..ce91269 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py +++ b/WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py @@ -1,4 +1,4 @@ -# Copyright (C) 2009 Google Inc. All rights reserved. +# Copyright (C) 2010 Google Inc. All rights reserved. # Copyright (C) 2009 Daniel Bates (dbates@intudata.com). All rights reserved. # # Redistribution and use in source and binary forms, with or without @@ -27,10 +27,14 @@ # (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 signal +import subprocess +import sys import unittest from webkitpy.common.system.executive import Executive, run_command + class ExecutiveTest(unittest.TestCase): def test_run_command_with_bad_command(self): @@ -38,5 +42,52 @@ class ExecutiveTest(unittest.TestCase): run_command(["foo_bar_command_blah"], error_handler=Executive.ignore_error, return_exit_code=True) self.failUnlessRaises(OSError, run_bad_command) -if __name__ == '__main__': - unittest.main() + def test_run_command_with_unicode(self): + """Validate that it is safe to pass unicode() objects + to Executive.run* methods, and they will return unicode() + objects by default unless decode_output=False""" + executive = Executive() + unicode_tor = u"WebKit \u2661 Tor Arne Vestb\u00F8!" + utf8_tor = unicode_tor.encode("utf-8") + + output = executive.run_command(["cat"], input=unicode_tor) + self.assertEquals(output, unicode_tor) + + output = executive.run_command(["echo", "-n", unicode_tor]) + self.assertEquals(output, unicode_tor) + + output = executive.run_command(["echo", "-n", unicode_tor], decode_output=False) + self.assertEquals(output, utf8_tor) + + # Make sure that str() input also works. + output = executive.run_command(["cat"], input=utf8_tor, decode_output=False) + self.assertEquals(output, utf8_tor) + + # FIXME: We should only have one run* method to test + output = executive.run_and_throw_if_fail(["echo", "-n", unicode_tor], quiet=True) + self.assertEquals(output, unicode_tor) + + output = executive.run_and_throw_if_fail(["echo", "-n", unicode_tor], quiet=True, decode_output=False) + self.assertEquals(output, utf8_tor) + + def test_kill_process(self): + executive = Executive() + # FIXME: This may need edits to work right on windows. + # We use "yes" because it loops forever. + process = subprocess.Popen(["yes"], stdout=subprocess.PIPE) + self.assertEqual(process.poll(), None) # Process is running + executive.kill_process(process.pid) + self.assertEqual(process.wait(), -signal.SIGKILL) + # Killing again should fail silently. + executive.kill_process(process.pid) + + def test_kill_all(self): + executive = Executive() + # FIXME: This may need edits to work right on windows. + # We use "yes" because it loops forever. + process = subprocess.Popen(["yes"], stdout=subprocess.PIPE) + self.assertEqual(process.poll(), None) # Process is running + executive.kill_all("yes") + self.assertEqual(process.wait(), -signal.SIGTERM) + # Killing again should fail silently. + executive.kill_all("yes") diff --git a/WebKitTools/Scripts/webkitpy/common/system/user.py b/WebKitTools/Scripts/webkitpy/common/system/user.py index 076f965..64995bb 100644 --- a/WebKitTools/Scripts/webkitpy/common/system/user.py +++ b/WebKitTools/Scripts/webkitpy/common/system/user.py @@ -62,11 +62,13 @@ class User(object): def edit(self, files): editor = os.environ.get("EDITOR") or "vi" args = shlex.split(editor) + # Note: Not thread safe: http://bugs.python.org/issue2320 subprocess.call(args + files) def page(self, message): pager = os.environ.get("PAGER") or "less" try: + # Note: Not thread safe: http://bugs.python.org/issue2320 child_process = subprocess.Popen([pager], stdin=subprocess.PIPE) child_process.communicate(input=message) except IOError, e: |