Commit eeafa0ea authored by Bob Haarman's avatar Bob Haarman Committed by Commit Bot

Revert "gclient_utils: buffer output as bytestrings in Annotated"

This reverts commit 5d284fdf.

Reason for revert: breaks buildbots, crbug.com/1011982

Original change's description:
> gclient_utils: buffer output as bytestrings in Annotated
> 
> In Python 3 byestrings and normal strings can't be concatenated.
> To fix this we buffer as bytestrings in the Annotated wrapper.
> We can't decode to a string because the output might come byte-by-byte, so it doesn't work with Unicode characters like .
> 
> Also had to update gclient_test.py, where double-wrapping stdout with Annotated caused made output not work and include_zero=True working caused other unintended side-effects.
> 
> Example error from "fetch chromium":
> Traceback (most recent call last):
>   File "C:\Google\depot_tools\gclient_scm.py", line 1045, in _Clone
>     self._Run(clone_cmd, options, cwd=self._root_dir, retry=True,
>   File "C:\Google\depot_tools\gclient_scm.py", line 1370, in _Run
>     gclient_utils.CheckCallAndFilter(cmd, env=env, **kwargs)
>   File "C:\Google\depot_tools\gclient_utils.py", line 583, in CheckCallAndFilter
>     show_header_if_necessary(needs_header, attempt)
>   File "C:\Google\depot_tools\gclient_utils.py", line 533, in show_header_if_necessary
>     stdout_write(header.encode())
>   File "C:\Google\depot_tools\gclient_utils.py", line 391, in write
>     obj[0] += out
> TypeError: can only concatenate str (not "bytes") to str
> 
> Bug: 984182
> Change-Id: If7037d30e9faf524f2405258281f6e6cd0bcdcae
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1778745
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
> Auto-Submit: Raul Tambre <raul@tambre.ee>

TBR=dpranke@chromium.org,ehmaldonado@chromium.org,raul@tambre.ee

Change-Id: I5ea8d3249c58a3487996649a264bb5be059fe884
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 984182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1845500Reviewed-by: 's avatarEdward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
parent 3306bbe4
......@@ -40,10 +40,8 @@ import subprocess2
if sys.version_info.major == 2:
from cStringIO import StringIO
string_type = basestring
else:
from io import StringIO
string_type = str
RETRY_MAX = 3
......@@ -373,10 +371,6 @@ class Annotated(Wrapper):
def write(self, out):
index = getattr(threading.currentThread(), 'index', 0)
if not index and not self.__include_zero:
# Store as bytes to ensure Unicode characters get output correctly.
if isinstance(out, bytes):
out = out.decode('utf-8')
# Unindexed threads aren't buffered.
return self._wrapped.write(out)
......@@ -386,32 +380,28 @@ class Annotated(Wrapper):
# Strings are immutable, requiring to keep a lock for the whole dictionary
# otherwise. Using an array is faster than using a dummy object.
if not index in self.__output_buffers:
obj = self.__output_buffers[index] = [b'']
obj = self.__output_buffers[index] = ['']
else:
obj = self.__output_buffers[index]
finally:
self.lock.release()
# Store as bytes to ensure Unicode characters get output correctly.
if isinstance(out, string_type):
out = out.encode('utf-8')
# Continue lockless.
obj[0] += out
while True:
# TODO(agable): find both of these with a single pass.
cr_loc = obj[0].find(b'\r')
lf_loc = obj[0].find(b'\n')
cr_loc = obj[0].find('\r')
lf_loc = obj[0].find('\n')
if cr_loc == lf_loc == -1:
break
elif cr_loc == -1 or (lf_loc >= 0 and lf_loc < cr_loc):
line, remaining = obj[0].split(b'\n', 1)
line, remaining = obj[0].split('\n', 1)
if line:
self._wrapped.write('%d>%s\n' % (index, line.decode('utf-8')))
self._wrapped.write('%d>%s\n' % (index, line))
elif lf_loc == -1 or (cr_loc >= 0 and cr_loc < lf_loc):
line, remaining = obj[0].split(b'\r', 1)
line, remaining = obj[0].split('\r', 1)
if line:
self._wrapped.write('%d>%s\r' % (index, line.decode('utf-8')))
self._wrapped.write('%d>%s\r' % (index, line))
obj[0] = remaining
def flush(self):
......@@ -433,7 +423,7 @@ class Annotated(Wrapper):
# Don't keep the lock while writting. Will append \n when it shouldn't.
for orphan in orphans:
if orphan[1]:
self._wrapped.write('%d>%s\n' % (orphan[0], orphan[1].decode('utf-8')))
self._wrapped.write('%d>%s\n' % (orphan[0], orphan[1]))
return self._wrapped.flush()
......
......@@ -76,10 +76,14 @@ class GclientTest(trial_dir.TestCase):
self._old_createscm = gclient.gclient_scm.GitWrapper
gclient.gclient_scm.GitWrapper = SCMMock
SCMMock.unit_test = self
self._old_sys_stdout = sys.stdout
sys.stdout = gclient.gclient_utils.MakeFileAutoFlush(sys.stdout)
sys.stdout = gclient.gclient_utils.MakeFileAnnotated(sys.stdout)
def tearDown(self):
self.assertEqual([], self._get_processed())
gclient.gclient_scm.GitWrapper = self._old_createscm
sys.stdout = self._old_sys_stdout
os.chdir(self.previous_dir)
super(GclientTest, self).tearDown()
......@@ -1362,9 +1366,9 @@ class GclientTest(trial_dir.TestCase):
if __name__ == '__main__':
sys.stdout = gclient_utils.MakeFileAutoFlush(sys.stdout)
sys.stdout = gclient_utils.MakeFileAnnotated(sys.stdout)
sys.stdout = gclient_utils.MakeFileAnnotated(sys.stdout, include_zero=True)
sys.stderr = gclient_utils.MakeFileAutoFlush(sys.stderr)
sys.stderr = gclient_utils.MakeFileAnnotated(sys.stderr)
sys.stderr = gclient_utils.MakeFileAnnotated(sys.stderr, include_zero=True)
logging.basicConfig(
level=[logging.ERROR, logging.WARNING, logging.INFO, logging.DEBUG][
min(sys.argv.count('-v'), 3)],
......
......@@ -127,7 +127,7 @@ class CheckCallAndFilterTestCase(unittest.TestCase):
'after a short nap...')
@mock.patch('subprocess2.Popen')
def testCheckCallAndFilter_PrintStdout(self, mockPopen):
def testCHeckCallAndFilter_PrintStdout(self, mockPopen):
cwd = 'bleh'
args = ['boo', 'foo', 'bar']
test_string = 'ahah\naccb\nallo\naddb\n✔'
......@@ -139,8 +139,10 @@ class CheckCallAndFilterTestCase(unittest.TestCase):
print_stdout=True)
self.assertEqual(result, test_string.encode('utf-8'))
self.assertEqual(self.stdout.getvalue().splitlines(), [
b"________ running 'boo foo bar' in 'bleh'",
self.assertEqual(
self.stdout.getvalue().splitlines(),
[
b'________ running \'boo foo bar\' in \'bleh\'',
b'ahah',
b'accb',
b'allo',
......@@ -149,43 +151,6 @@ class CheckCallAndFilterTestCase(unittest.TestCase):
])
class AnnotatedTestCase(unittest.TestCase):
def setUp(self):
self.out = gclient_utils.MakeFileAnnotated(io.StringIO())
self.outz = gclient_utils.MakeFileAnnotated(
io.StringIO(), include_zero=True)
def testString(self):
self.outz.write('test string\n')
self.assertEqual(self.outz.getvalue(), '0>test string\n')
def testBytes(self):
self.out.write(b'test string\n')
self.assertEqual(self.out.getvalue(), 'test string\n')
def testBytesZero(self):
self.outz.write(b'test string\n')
self.assertEqual(self.outz.getvalue(), '0>test string\n')
def testUnicode(self):
self.out.write(b'\xe2\x9c\x94\n')
self.assertEqual(self.out.getvalue(), '✔\n')
def testUnicodeZero(self):
self.outz.write('✔\n')
self.assertEqual(self.outz.getvalue(), '0>✔\n')
def testMultiline(self):
self.out.write(b'first line\nsecond line')
self.assertEqual(self.out.getvalue(), 'first line\nsecond line')
def testFlush(self):
self.outz.write(b'first line\nsecond line')
self.assertEqual(self.outz.getvalue(), '0>first line\n')
self.outz.flush()
self.assertEqual(self.outz.getvalue(), '0>first line\n0>second line\n')
class SplitUrlRevisionTestCase(unittest.TestCase):
def testSSHUrl(self):
url = "ssh://test@example.com/test.git"
......@@ -306,6 +271,7 @@ class GClientUtilsTest(trial_dir.TestCase):
if __name__ == '__main__':
import unittest
unittest.main()
# vim: ts=2:sw=2:tw=80:et:
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment