Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10790014:
Add Java support to checkdeps.py (Closed)
|
Created:
8 years, 5 months ago by Iain Merrick Modified:
8 years, 5 months ago CC:
chromium-reviews, pam+watch_chromium.org, Jói Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Java support to checkdeps.py
I've moved the C++-specific parts of checkdeps.py into objcpp_checker.py,
and added a corresponding java_checker.py module for Java code. The Java
module does an extra pass over the files (see Pydoc for details) but this
has almost no impact on the overall running time.
The Java checker is intended to be compatible with the existing DEPS
format despite Java's different import mechanism.
In the Android repo, there are a few extra DEPS files needed for the Java
code. That code has not yet been upstreamed back to Chromium, so those
DEPS aren't needed yet -- they can be added as and when they're needed.
BUG=137081
TEST=checkdeps.py
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148094
Patch Set 1 #
Total comments: 41
Patch Set 2 : Ditched checker.py #
Total comments: 8
Patch Set 3 : More cleanup #
Total comments: 2
Patch Set 4 : Fixed indentation #Patch Set 5 : Tweaks for pylint #Patch Set 6 : Fix path errors when base_directory isn't cwd #Patch Set 7 : Fix path errors due to weird separators on Windows #Patch Set 8 : Convert backslashes to slashes in Java include paths #
Messages
Total messages: 21 (0 generated)
This is code that we're using in the Android port. I'm happy to make any changes that you guys request.
I'm not a qualified reviewer for this
Sorry, I was going by recent names in the change log for this dir, but I guess it was misleading. Removed jam, added maruel.
http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py File tools/checkdeps/checkdeps.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:62: import checker No need to import 'checker'. Furthermore doing that would cause aliasing at line 288. My take: def checker_factory(filename): """Returns an implementation to 'check' the source files.""" ext = os.path.splitext(filename)[1] if ext in java_checker.Checker.EXTENSIONS: return java_checker.Checker() elif ext in cpp_checker.Checker.EXTENSIONS: return cpp_checker.Checker() Your factory is now 7 lines. :) http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:290: if file_status != None: if file_status: Then you can remove the weird checks for len(ret_val) != 0. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checker.py File tools/checkdeps/checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checker.py#new... tools/checkdeps/checker.py:8: class Checker(object): Note that an "interface" is not a contractual definition in python. As such, I'm not even sure this is useful. Some people used Zope to simulate proper interfaces but this is just barely useful. The only time I really use base classes is when a partial implementation is needed. In that case, there's _base_directory so it may be worth keeping it. In any case, kill the Factory class. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checker.py#new... tools/checkdeps/checker.py:37: class Factory(object): This is overdesign at that point; this is no Java. :D Remove this whole class. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py File tools/checkdeps/cpp_checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:25: EXTRACT_INCLUDE_PATH = re.compile('[ \t]*#[ \t]*(?:include|import)[ \t]+"(.*)"') This could be a protected class member instead. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:30: def Extensions(self): EXTENSIONS = [ http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:51: include_path.replace('\\', '/') Err. The return value is ignored. I'm glad we don't allow '\\' in the style. You should just error out when one is found. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:53: if include_path.find('/') < 0: if '/' not in include_path: http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:63: retval = '\nFor ' + rules.__str__() retval = '\nFor %s' % rules http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:78: cur_file = open(file_name, 'r') with codecs.open(filepath, encoding='utf-8') as f: http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:114: # Map empty string to None for easier checking. This check is completely unnecessary. Just add "return ret_val" at line 108. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:118: Remove extra line. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.py File tools/checkdeps/java_checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:36: EXTENSIONS = [ '.java', ] http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:42: self._classmap = {} Define it in the __init__ function. Make sure your code is pylint error free. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:65: def _PrescanFile(self, filename): It's really more a filepath than a filename, but I don't mind. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:69: f = fileinput.input(filename) I don't see why you are using fileinput at all here. with codecs.open(filename, encoding='utf-8') as f: or something like that. Beware of default file encoding. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:71: short_class_name, _ = os.path.splitext(os.path.basename(filename)) short_class_name = os.path.splitext(os.path.basename(filename))[0] http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:87: def Extensions(self): Remove. This doesn't need to be a member function. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:95: f = fileinput.input(filename) with fileinput.input(filename) as f: Remove the finally section. Note that python 2.6 is required for depot_tools so using the 'with' statement is fine. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:121: if len(result) == 0: if result: return result None is implicitly returned otherwise. Not sure why you would do that. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:124: Remove extra line.
Thanks for the detailed comments! http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py File tools/checkdeps/checkdeps.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:62: import checker On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > No need to import 'checker'. Furthermore doing that would cause aliasing at line > 288. > > My take: > > def checker_factory(filename): > """Returns an implementation to 'check' the source files.""" > ext = os.path.splitext(filename)[1] > if ext in java_checker.Checker.EXTENSIONS: > return java_checker.Checker() > elif ext in cpp_checker.Checker.EXTENSIONS: > return cpp_checker.Checker() > > Your factory is now 7 lines. :) That's sorta what I had originally, then I was convinced to make it more OO-y. :) The only twist is that we want to construct the java checker once, as it pre-scans the files during __init__. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:290: if file_status != None: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > if file_status: > > Then you can remove the weird checks for len(ret_val) != 0. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checker.py File tools/checkdeps/checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/checker.py#new... tools/checkdeps/checker.py:8: class Checker(object): On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Note that an "interface" is not a contractual definition in python. As such, I'm > not even sure this is useful. > > Some people used Zope to simulate proper interfaces but this is just barely > useful. The only time I really use base classes is when a partial implementation > is needed. > > In that case, there's _base_directory so it may be worth keeping it. In any > case, kill the Factory class. OK, if you prefer to keep the contract implicit, this whole module can probably go. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py File tools/checkdeps/cpp_checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:25: EXTRACT_INCLUDE_PATH = re.compile('[ \t]*#[ \t]*(?:include|import)[ \t]+"(.*)"') On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > This could be a protected class member instead. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:30: def Extensions(self): On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > EXTENSIONS = [ Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:51: include_path.replace('\\', '/') On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Err. The return value is ignored. > > I'm glad we don't allow '\\' in the style. You should just error out when one is > found. Sounds good to me. (Though I'd better check there aren't any existing broken files, of course...) http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:53: if include_path.find('/') < 0: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > if '/' not in include_path: Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:63: retval = '\nFor ' + rules.__str__() On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > retval = '\nFor %s' % rules Wow, yes http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:78: cur_file = open(file_name, 'r') On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > with codecs.open(filepath, encoding='utf-8') as f: Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:114: # Map empty string to None for easier checking. On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > This check is completely unnecessary. Just add "return ret_val" at line 108. I just copied the existing code as is as far as possible, but yes, agreed. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/cpp_checker.py... tools/checkdeps/cpp_checker.py:118: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Remove extra line. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.py File tools/checkdeps/java_checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:36: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > EXTENSIONS = [ > '.java', > ] Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:42: self._classmap = {} On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Define it in the __init__ function. Make sure your code is pylint error free. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:69: f = fileinput.input(filename) On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > I don't see why you are using fileinput at all here. Just insufficient time spent searching the library docs -- I'm only an occasional Pythoner. :) > with codecs.open(filename, encoding='utf-8') as f: > or something like that. Beware of default file encoding. Done http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:87: def Extensions(self): On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Remove. This doesn't need to be a member function. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:95: f = fileinput.input(filename) On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > with fileinput.input(filename) as f: > > Remove the finally section. Note that python 2.6 is required for depot_tools so > using the 'with' statement is fine. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:121: if len(result) == 0: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > if result: > return result > > None is implicitly returned otherwise. Not sure why you would do that. Done. http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:124: On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > Remove extra line. Done.
http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.py File tools/checkdeps/java_checker.py (right): http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:65: def _PrescanFile(self, filename): On 2012/07/17 14:23:20, Marc-Antoine Ruel wrote: > It's really more a filepath than a filename, but I don't mind. Changed (and I just noticed that my search-and-replace also affected the docstring, but it seems valid) http://codereview.chromium.org/10790014/diff/1/tools/checkdeps/java_checker.p... tools/checkdeps/java_checker.py:114: except IOError: Note: I've removed all IOError handling, as this is an exceptional situation and we shouldn't try to keep running. Errors will dump a stack trace which seems okay. If we don't want a stack trace we should handle the exception at the top level, in checkdeps.py
Assuming utf-8 won't work. I wrote a small script because I was wondering about
the #if 0 check if they were worthwhile at all. I found that the way #if 0 check
is done is not useful, because there are lines like "# if 0" in the tree. I
think there's also #elif 0.
Here's the script
--- CUT HERE ---
import codecs, os, re, subprocess
files = subprocess.check_output(['git', 'ls-files']).splitlines()
print 'Found %d files' % len(files)
for filepath in files:
if os.path.splitext(filepath)[1] not in ('.cc', '.c', '.h', '.m', '.mm'):
continue
try:
with codecs.open(filepath, encoding='utf-8') as f:
conditions = []
for num, line in enumerate(f):
line = line.strip()
if re.match(r'\#\s*if.+', line):
conditions.append(bool(re.match(r'\#\s*if\s+0.*', line)))
continue
if re.match(r'\#\s*endif.*', line):
if not conditions:
print '%s(%d): Inbalanced #if / #endif' % (filepath, num)
break
conditions.pop()
continue
if re.match(r'\#\s*include.+', line):
if conditions and conditions[-1]:
print '%s(%d): Found include inside #if 0' % (filepath, num)
break
except UnicodeDecodeError:
print '%s: File is not in utf-8' % filepath
--- CUT HERE ---
and the result
--- CUT HERE ---
Found 38835 files
chrome/test/perf/url_parse_perftest.cc(17): Found include inside #if 0
jingle/notifier/listener/push_notifications_subscribe_task.cc: File is not in
utf-8
net/third_party/nss/ssl/unix_err.c(47): Found include inside #if 0
third_party/harfbuzz/contrib/harfbuzz-freetype.c(7): Found include inside #if 0
third_party/harfbuzz/src/harfbuzz-stream.c(33): Found include inside #if 0
third_party/jemalloc/chromium/rb.h(75): Found include inside #if 0
third_party/jemalloc/vendor/rb.h(75): Found include inside #if 0
third_party/libxml/src/entities.c: File is not in utf-8
third_party/libxml/src/include/libxml/encoding.h(36): Found include inside #if 0
third_party/libxml/src/runtest.c: File is not in utf-8
third_party/libxml/src/testapi.c: File is not in utf-8
third_party/libxslt/libxslt/xslt.c: File is not in utf-8
third_party/mesa/MesaLib/src/glx/XF86dri.c(89): Found include inside #if 0
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_dd.c: File is not in
utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_ioctl.h: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_lock.c: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_native_vb.c: File is
not in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_native_vbtmp.h: File
is not in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_reg.h: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_screen.h: File is
not in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_tex.c: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_tex.h: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_texstate.c: File is
not in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_tris.c: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_tris.h: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_vb.c: File is not in
utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_vb.h: File is not in
utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/mach64/mach64_vbtmp.h: File is not
in utf-8
third_party/mesa/MesaLib/src/mesa/drivers/dri/tdfx/tdfx_span.c(177): Found
include inside #if 0
third_party/mesa/MesaLib/src/mesa/drivers/dri/unichrome/server/via_driver.h(37):
Found include inside #if 0
third_party/mesa/MesaLib/src/mesa/x86/mmx_blendtmp.h: File is not in utf-8
third_party/npapi/npspy/extern/plugin/npruntime.h: File is not in utf-8
third_party/openmax/il/OMX_Other.h: File is not in utf-8
third_party/snappy/win32/snappy-stubs-public.h(39): Found include inside #if 0
third_party/talloc/libreplace/timegm.c: File is not in utf-8
third_party/tcmalloc/chromium/src/libc_override_glibc.h(132): Found include
inside #if 0
third_party/tcmalloc/chromium/src/tests/tcmalloc_unittest.cc(993): Found include
inside #if 0
third_party/tcmalloc/chromium/src/windows/ia32_modrm_map.cc: File is not in
utf-8
third_party/tcmalloc/chromium/src/windows/ia32_opcode_map.cc: File is not in
utf-8
third_party/tcmalloc/chromium/src/windows/mini_disassembler.h: File is not in
utf-8
third_party/tcmalloc/vendor/src/libc_override_glibc.h(121): Found include inside
#if 0
third_party/tcmalloc/vendor/src/tests/tcmalloc_unittest.cc(993): Found include
inside #if 0
third_party/tcmalloc/vendor/src/windows/ia32_modrm_map.cc: File is not in utf-8
third_party/tcmalloc/vendor/src/windows/ia32_opcode_map.cc: File is not in utf-8
third_party/tcmalloc/vendor/src/windows/mini_disassembler.h: File is not in
utf-8
tools/memory_watcher/mini_disassembler.h: File is not in utf-8
--- CUT HERE ---
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker.py
File tools/checkdeps/cpp_checker.py (right):
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker...
tools/checkdeps/cpp_checker.py:77: for line_num in xrange(sys.maxint):
An alternative is:
for line_num, line in enumerate(f):
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker...
tools/checkdeps/cpp_checker.py:81: cur_line =
f.readline(self._MAX_LINE_LENGTH).strip()
line = line.strip()
I don't think line 82-83 are a good idea. It would stop too early.
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker...
tools/checkdeps/cpp_checker.py:86: if cur_line == '#if 0':
if line.startswith('#if 0'):
would catch: #if 0 // This code is broken.
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/java_checke...
File tools/checkdeps/java_checker.py (right):
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/java_checke...
tools/checkdeps/java_checker.py:98: result += '\nFor ' + rules.__str__()
Same.
http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker.py File tools/checkdeps/cpp_checker.py (right): http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker... tools/checkdeps/cpp_checker.py:77: for line_num in xrange(sys.maxint): On 2012/07/18 14:19:37, Marc-Antoine Ruel wrote: > An alternative is: > > for line_num, line in enumerate(f): Good call, done. And I think that's the only reason for importing "sys" so I can lose that too. http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker... tools/checkdeps/cpp_checker.py:81: cur_line = f.readline(self._MAX_LINE_LENGTH).strip() On 2012/07/18 14:19:37, Marc-Antoine Ruel wrote: > line = line.strip() > > I don't think line 82-83 are a good idea. It would stop too early. Seemed like a harmless optimization to me, so I left it in...? Not sure what you mean about stopping early, but sure, happy to get rid of this. http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/cpp_checker... tools/checkdeps/cpp_checker.py:86: if cur_line == '#if 0': On 2012/07/18 14:19:37, Marc-Antoine Ruel wrote: > if line.startswith('#if 0'): > > would catch: #if 0 // This code is broken. Good idea, done. This is just the original algorithm from checkdeps.py, so I don't fully understand the intentions. But it seems okay for it to catch only *some* of the commented-out includes. The only way to be completely accurate would be to preprocess all the files, which is clearly a bad idea. Therefore, an imperfect heuristic seems fine; but your suggestion is essentially free and strictly better, so I'll do that. http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/java_checke... File tools/checkdeps/java_checker.py (right): http://codereview.chromium.org/10790014/diff/6001/tools/checkdeps/java_checke... tools/checkdeps/java_checker.py:98: result += '\nFor ' + rules.__str__() On 2012/07/18 14:19:37, Marc-Antoine Ruel wrote: > Same. Done.
On 2012/07/18 14:19:37, Marc-Antoine Ruel wrote: > Assuming utf-8 won't work. Wow, ouch, good find. It looks like all the non-UTF8 files are under third_party, and therefore don't get scanned. But it's fragile -- java_checker.py explicitly skips third_party, but cpp_checker.py relies on there being DEPS rules to skip those. As all the patterns we're looking for are ASCII, what do you think of just opening files in a basic 8-bit encoding? That should at least safely skip past any non-ASCII characters. I'll give that a try, let me know what you think.
We had an IM discussion about this, and Marc-Antoine started writing a script to fix up all our non-UTF-8 source files -- yay! But now I'm unclear on whether more changes are needed in this patch. Is this okay to submit as is? Or is there some other work that needs to be done first?
lgtm http://codereview.chromium.org/10790014/diff/7/tools/checkdeps/checkdeps.py File tools/checkdeps/checkdeps.py (right): http://codereview.chromium.org/10790014/diff/7/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:383: checkers = dict((extension, checker) Weird alignment. Either all at +4 or at (. But I prefer +4.
http://codereview.chromium.org/10790014/diff/7/tools/checkdeps/checkdeps.py File tools/checkdeps/checkdeps.py (right): http://codereview.chromium.org/10790014/diff/7/tools/checkdeps/checkdeps.py#n... tools/checkdeps/checkdeps.py:383: checkers = dict((extension, checker) On 2012/07/20 12:53:27, Marc-Antoine Ruel wrote: > Weird alignment. Either all at +4 or at (. But I prefer +4. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/7
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/13003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/19005
Try job failure for 10790014-19005 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/19005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/12
Try job failure for 10790014-12 (retry) on win_rel for step "check_deps". It's a second try, previously, step "check_deps" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/husky@chromium.org/10790014/21002
Change committed as 148094 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
