When style bot/check-webkit-style processed attachment 220739 [details] (bug #126698) it died with the the following backtrace: Traceback (most recent call last): File "Tools/Scripts/check-webkit-style", line 48, in <module> sys.exit(CheckWebKitStyle().main()) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main patch_checker.check(patch) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check self._text_file_reader.process_file(file_path=path, line_numbers=None) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file lines = self._read_lines(file_path) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines contents = file.read() File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read return self.reader.read(size) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read newchars, decodedbytes = self.decode(data, self.errors) UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte For completeness, see bug #126698 comment 2 for more details.
Also bug 125689 comment 28.
This regressed in <http://trac.webkit.org/changeset/160085>.
Created attachment 221013 [details] Patch
Comment on attachment 221013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221013&action=review > Tools/Scripts/webkitpy/common/system/filesystem.py:203 > + def open_file_for_read(self, path): > + return codecs.open(path, 'r', 'utf8', 'replace') > + We shouldn't be adding this one off function with such an ambitious name here. r-. I could see open_text_file_for_reading having an optional argument to do this instead.
Can a regression test be added for this fix?
Created attachment 221142 [details] Patch
(In reply to comment #4) > We shouldn't be adding this one off function with such an ambitious name here. You are right, i think it looks better now. (In reply to comment #5) > Can a regression test be added for this fix? Yes, good idea. I added a new test case for that.
Comment on attachment 221142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221142&action=review Seems fine to land to stop bot failures as an emergency fix. Thanks for looking into this! Longer term, I think that the approach could be further improved. We could skip known non-text files, and we could also handle decoding failures more gracefully. In fact, we should probably report an explicit error when there is malformed utf-8 in actual text files. I do not know what our compilers do in this case, especially when it's in comments. > Tools/ChangeLog:8 > + Resolve regression and remove a FIXME comment that is already fixed. The final patch doesn't remove any FIXMEs. > Tools/Scripts/webkitpy/common/system/filesystem.py:201 > + def open_text_file_for_reading(self, path, errors='strict'): It's not good that we are calling a function named "open_text_file_for_reading" on all files, not only text ones. This should be fixed.
Comment on attachment 221142 [details] Patch Clearing flags on attachment: 221142 Committed r161986: <http://trac.webkit.org/changeset/161986>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > > Tools/ChangeLog:8 > > + Resolve regression and remove a FIXME comment that is already fixed. > > The final patch doesn't remove any FIXMEs. > :( I missed it from the second version of the patch. That FIXME was fixed in r160085. We Should remove that comment. Sorry for that.