Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1136573002:
Use the Errorprone Compiler (Closed)
|
Created:
5 years, 7 months ago by raywilliams Modified:
5 years, 6 months ago CC:
chromium-reviews, cjhopman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the Errorprone Java Compiler
These changes let the errorprone compiler find problems when building Android.
A global flag disabled Errorprone by default.
When enabled, code problems will be shown with suggestions on how to fix them.
BUG=485599
Committed: https://crrev.com/6ffb1179d6adf08edd64848b45b7415b6b6de43d
Cr-Commit-Position: refs/heads/master@{#335509}
Patch Set 1 : Using the Compiler #
Total comments: 29
Patch Set 2 : Reverted unwanted changes #Patch Set 3 : Combined files from third_party #Patch Set 4 : Changed default to disabled #
Total comments: 40
Patch Set 5 : Addressed Review Comments #Patch Set 6 : Extracted some constants #
Total comments: 15
Patch Set 7 : Addressed Review Comments #Patch Set 8 : Renamed an option #Patch Set 9 : Change Global Flag #
Total comments: 6
Patch Set 10 : Addressed Review Comments #Patch Set 11 : Rebase and parser update #
Total comments: 9
Patch Set 12 : Cleanup and testing #
Total comments: 4
Patch Set 13 : use public jar instead of local #
Total comments: 2
Patch Set 14 : toggled a flag #Patch Set 15 : removed mode from sun jar copy #Patch Set 16 : Full Sync and Update #
Messages
Total messages: 51 (9 generated)
raywilliams@google.com changed reviewers: + cjhopman@chromium.org, jbudorick@chromium.org
https://codereview.chromium.org/1136573002/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1136573002/diff/1/BUILD.gn#newcode222 BUILD.gn:222: "//third_party/errorprone:chromium_errorprone", I'm not sure that this needs to be in here, and I am sure that none of the other changes should be in here. https://codereview.chromium.org/1136573002/diff/1/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/all.gyp#newcode82 build/all.gyp:82: '../third_party/errorprone/errorprone.gyp:chromium_errorprone', Same deal: I'm not sure this needs to be in here. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... build/android/gyp/find_sun_tools_jar.py:21: parser.add_option('--output', help='Path to copy the tools.jar to.') There has got to be a better way to do this than copying a jar out of the JRE. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:93: disabled_checks = [ The contents of this if block should probably be in its own function. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:106: assert errorprone_path != None Do we use assertions in the gyp scripts? I know we've generally been shying away from them in the test runner scripts. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:207: '--disable-errorprone', Having this as a disable flag implies that errorprone is on by default. I'm not sure that's the stance we want to take, especially at introduction. Also, doing this as an enable flag makes your use of it below less awkward. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:273: options.chromium_code and not options.disable_errorprone, (this is the awkward double-negative part referred to above) https://codereview.chromium.org/1136573002/diff/1/build/android/rezip.gyp File build/android/rezip.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/android/rezip.gyp#new... build/android/rezip.gyp:39: '--disable-errorprone', Yeah, I'm definitely not a fan of default-on. https://codereview.chromium.org/1136573002/diff/1/build/android/setup.gyp File build/android/setup.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/android/setup.gyp#new... build/android/setup.gyp:19: '<(android_stlport_libs_dir)/libstlport_shared.so', Oh, you haven't rebased in a while. Do so before you upload your next patchset. https://codereview.chromium.org/1136573002/diff/1/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/1/build/java_apk.gypi#newcode183 build/java_apk.gypi:183: 'disable_errorprone%': '0', If errorprone is enabled, this should add the errorprone gyp target as a dependency.
Thanks for reviewing. Sorry, had a few unrelated changes in there. I backed up my work, then pulled from master, then copied my changes back. This caused some unrelated changes to be lost. I've corrected it now. I'll take a look and respond to your comments that are not related to the reverted changes...
https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... build/android/gyp/find_sun_tools_jar.py:21: parser.add_option('--output', help='Path to copy the tools.jar to.') On 2015/05/07 18:49:26, jbudorick wrote: > There has got to be a better way to do this than copying a jar out of the JRE. That would be nice. You need to use the tools.jar from the JRE. Then, I really think you want to have a java_prebuilt/host_prebuilt_jar.gypi for it. To do that, you need to know the path at gyp/gn time. I can think of a couple ways to do that: 1. At build time, make sure that the jar is at a specific path. That's this approach where you copy it into the place you want it. 2. Find it at gyp/gn time and use whatever path it has then. We really want to avoid doing work like that at gyp/gn time. This also has a problem that it might be wrong if you change your java version in-between gyp and building (though that probably breaks other things too). 3. Find it at gclient sync time and copy it into your tree somewhere. This wouldn't be too bad, but it's not really much better than (1). 4. Find it at gclient sync time and write its path somewhere. Then use that path at gyp/gn time... 5. Include a jre in our checkout and use that when building. Then you know where the tools.jar is. This is actually what I think we should be doing and not using your system one at all... but I definitely wouldn't want to take on that project for this change. 6. Make java_prebuilt/host_jar_prebuilt.gypi (or new versions of them) that work with just a build-time string value so that gyp/gn doesn't actually know the path. I don't know if this is actually possible (I think in gn it would be, but not in gyp). Then, another possibility I guess is to not use java_prebuilt/host_jar_prebuilt.gypi, but I expect that that would get really complicated... you would essentially have to duplicate all the stuff that those things already handle for you.
raywilliams@chromium.org changed reviewers: + raywilliams@chromium.org
Addressed comments https://codereview.chromium.org/1136573002/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1136573002/diff/1/BUILD.gn#newcode222 BUILD.gn:222: "//third_party/errorprone:chromium_errorprone", On 2015/05/07 18:49:26, jbudorick wrote: > I'm not sure that this needs to be in here, and I am sure that none of the other > changes should be in here. I'll try taking this out and see if it still works. The other stuff was the result of a version control mistake, they were all fixed in patch set 2. https://codereview.chromium.org/1136573002/diff/1/BUILD.gn#newcode222 BUILD.gn:222: "//third_party/errorprone:chromium_errorprone", On 2015/05/07 18:49:26, jbudorick wrote: > I'm not sure that this needs to be in here, and I am sure that none of the other > changes should be in here. Done. https://codereview.chromium.org/1136573002/diff/1/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/all.gyp#newcode82 build/all.gyp:82: '../third_party/errorprone/errorprone.gyp:chromium_errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Same deal: I'm not sure this needs to be in here. I'll take this out and see what happens :) https://codereview.chromium.org/1136573002/diff/1/build/all.gyp#newcode82 build/all.gyp:82: '../third_party/errorprone/errorprone.gyp:chromium_errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Same deal: I'm not sure this needs to be in here. Done. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/find_sun_... build/android/gyp/find_sun_tools_jar.py:21: parser.add_option('--output', help='Path to copy the tools.jar to.') On 2015/05/07 19:15:18, cjhopman wrote: > On 2015/05/07 18:49:26, jbudorick wrote: > > There has got to be a better way to do this than copying a jar out of the JRE. From cjhopman's answer, it sounds like this is the least messy way to do it. We can talk if you still want this to change... https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:93: disabled_checks = [ On 2015/05/07 18:49:26, jbudorick wrote: > The contents of this if block should probably be in its own function. helper method extracted https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:93: disabled_checks = [ On 2015/05/07 18:49:26, jbudorick wrote: > The contents of this if block should probably be in its own function. Done. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:106: assert errorprone_path != None On 2015/05/07 18:49:26, jbudorick wrote: > Do we use assertions in the gyp scripts? I know we've generally been shying away > from them in the test runner scripts. removed https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:106: assert errorprone_path != None On 2015/05/07 18:49:26, jbudorick wrote: > Do we use assertions in the gyp scripts? I know we've generally been shying away > from them in the test runner scripts. Done. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:207: '--disable-errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Having this as a disable flag implies that errorprone is on by default. I'm not > sure that's the stance we want to take, especially at introduction. > > Also, doing this as an enable flag makes your use of it below less awkward. changed the disable flag to an enable flag https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:207: '--disable-errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Having this as a disable flag implies that errorprone is on by default. I'm not > sure that's the stance we want to take, especially at introduction. > > Also, doing this as an enable flag makes your use of it below less awkward. Done. https://codereview.chromium.org/1136573002/diff/1/build/android/gyp/javac.py#... build/android/gyp/javac.py:273: options.chromium_code and not options.disable_errorprone, On 2015/05/07 18:49:26, jbudorick wrote: > (this is the awkward double-negative part referred to above) Done. https://codereview.chromium.org/1136573002/diff/1/build/android/rezip.gyp File build/android/rezip.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/android/rezip.gyp#new... build/android/rezip.gyp:39: '--disable-errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Yeah, I'm definitely not a fan of default-on. I'll change it in the next patch set. (Actually, with enable off by default this file can be fully reverted) https://codereview.chromium.org/1136573002/diff/1/build/android/rezip.gyp#new... build/android/rezip.gyp:39: '--disable-errorprone', On 2015/05/07 18:49:26, jbudorick wrote: > Yeah, I'm definitely not a fan of default-on. Done. https://codereview.chromium.org/1136573002/diff/1/build/android/setup.gyp File build/android/setup.gyp (right): https://codereview.chromium.org/1136573002/diff/1/build/android/setup.gyp#new... build/android/setup.gyp:19: '<(android_stlport_libs_dir)/libstlport_shared.so', On 2015/05/07 18:49:27, jbudorick wrote: > Oh, you haven't rebased in a while. Do so before you upload your next patchset. Yeah, that caused some false changes in this review. Patch set 2 fixes that. https://codereview.chromium.org/1136573002/diff/1/build/android/setup.gyp#new... build/android/setup.gyp:19: '<(android_stlport_libs_dir)/libstlport_shared.so', On 2015/05/07 18:49:27, jbudorick wrote: > Oh, you haven't rebased in a while. Do so before you upload your next patchset. Done. https://codereview.chromium.org/1136573002/diff/1/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/1/build/java_apk.gypi#newcode183 build/java_apk.gypi:183: 'disable_errorprone%': '0', On 2015/05/07 18:49:27, jbudorick wrote: > If errorprone is enabled, this should add the errorprone gyp target as a > dependency. added the condition https://codereview.chromium.org/1136573002/diff/1/build/java_apk.gypi#newcode183 build/java_apk.gypi:183: 'disable_errorprone%': '0', On 2015/05/07 18:49:27, jbudorick wrote: > If errorprone is enabled, this should add the errorprone gyp target as a > dependency. Done.
New patch set, addressed comments, changed disable flag to enable flag, default to disabled
I do think the jar should be in a third_party/errorprone/lib/ directory that gets DEPS'ed in from a separate repository. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:18: def main(argv): nit: drop the argv param and don't pass anything to parse_args https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:19: parser = optparse.OptionParser() oh, and use argparse. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:25: stdout = build_utils.CheckOutput( Move the logic for finding sun_tools_jar_path into its own function. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:27: rt_jar_finder = re.compile(r'\[Opened (.*)/jre/lib/rt.jar\]') Do this as a constant. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:29: for ln in stdout.split('\n'): splitlines? https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:36: raise Exception('Couldn\'t find tools.jar') nit: use double quotes when the string has a single quote inside. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:92: def GetErrorproneCommand(): This should not be done with a local function. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:102: errorprone_options = [ errorprone_options can just be a constant, right? https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:208: parser.add_option( Do we need both of these options? Passing --enable-errorprone means --errorprone-path is required, and passing --errorprone-path without --enable-errorprone is meaningless. What if --errorprone-path did both? https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi File build/host_jar.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi#new... build/host_jar.gypi:56: 'disable_errorprone%': '0', nit: rename as enable_errorprone https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi#new... build/host_jar.gypi:81: ['disable_errorprone == 0', { er... if disable_errorprone is 0, you're passing --errorprone-path but not --enable-errorprone. if disable_errorprone is 1, you're passing --enable-errorprone but not --errorprone-path. https://codereview.chromium.org/1136573002/diff/60001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/java.gypi#newcode264 build/java.gypi:264: 'extra_args': [ '--enable-errorprone' ], Similarly here w.r.t. --errorprone-path & --enable-errorprone, except without the double negative. https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:184: No need for line breaks. https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:230: same https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:691: 'extra_args': [ '--errorprone-path=<(errorprone_exe_path)' ], same args comment, although this one is flipped somewhat -- the other enable_errorprone was == 1, this one is == 0
https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:18: def main(argv): On 2015/05/15 18:37:12, jbudorick wrote: > nit: drop the argv param and don't pass anything to parse_args Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:19: parser = optparse.OptionParser() On 2015/05/15 18:37:12, jbudorick wrote: > oh, and use argparse. Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:25: stdout = build_utils.CheckOutput( On 2015/05/15 18:37:12, jbudorick wrote: > Move the logic for finding sun_tools_jar_path into its own function. helper method extracted https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:25: stdout = build_utils.CheckOutput( On 2015/05/15 18:37:12, jbudorick wrote: > Move the logic for finding sun_tools_jar_path into its own function. Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:27: rt_jar_finder = re.compile(r'\[Opened (.*)/jre/lib/rt.jar\]') On 2015/05/15 18:37:12, jbudorick wrote: > Do this as a constant. Do you mean pull rt_jar_finder out of main() ? https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:29: for ln in stdout.split('\n'): On 2015/05/15 18:37:12, jbudorick wrote: > splitlines? sure :) https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:29: for ln in stdout.split('\n'): On 2015/05/15 18:37:12, jbudorick wrote: > splitlines? Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:36: raise Exception('Couldn\'t find tools.jar') On 2015/05/15 18:37:12, jbudorick wrote: > nit: use double quotes when the string has a single quote inside. Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:92: def GetErrorproneCommand(): On 2015/05/15 18:37:12, jbudorick wrote: > This should not be done with a local function. Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:102: errorprone_options = [ On 2015/05/15 18:37:12, jbudorick wrote: > errorprone_options can just be a constant, right? Hmm... I don't think python has constants. I can kill the helper method and put this back as a regular var (like in patch-set 1). Let me know if you want me to do that. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:208: parser.add_option( On 2015/05/15 18:37:12, jbudorick wrote: > Do we need both of these options? Passing --enable-errorprone means > --errorprone-path is required, and passing --errorprone-path without > --enable-errorprone is meaningless. What if --errorprone-path did both? Good idea :) https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:208: parser.add_option( On 2015/05/15 18:37:12, jbudorick wrote: > Do we need both of these options? Passing --enable-errorprone means > --errorprone-path is required, and passing --errorprone-path without > --enable-errorprone is meaningless. What if --errorprone-path did both? Done. https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi File build/host_jar.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi#new... build/host_jar.gypi:56: 'disable_errorprone%': '0', On 2015/05/15 18:37:12, jbudorick wrote: > nit: rename as enable_errorprone Done. https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi#new... build/host_jar.gypi:81: ['disable_errorprone == 0', { On 2015/05/15 18:37:12, jbudorick wrote: > er... if disable_errorprone is 0, you're passing --errorprone-path but not > --enable-errorprone. if disable_errorprone is 1, you're passing > --enable-errorprone but not --errorprone-path. Fixed https://codereview.chromium.org/1136573002/diff/60001/build/host_jar.gypi#new... build/host_jar.gypi:81: ['disable_errorprone == 0', { On 2015/05/15 18:37:12, jbudorick wrote: > er... if disable_errorprone is 0, you're passing --errorprone-path but not > --enable-errorprone. if disable_errorprone is 1, you're passing > --enable-errorprone but not --errorprone-path. Done. https://codereview.chromium.org/1136573002/diff/60001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/java.gypi#newcode264 build/java.gypi:264: 'extra_args': [ '--enable-errorprone' ], On 2015/05/15 18:37:12, jbudorick wrote: > Similarly here w.r.t. --errorprone-path & --enable-errorprone, except without > the double negative. Fixed https://codereview.chromium.org/1136573002/diff/60001/build/java.gypi#newcode264 build/java.gypi:264: 'extra_args': [ '--enable-errorprone' ], On 2015/05/15 18:37:12, jbudorick wrote: > Similarly here w.r.t. --errorprone-path & --enable-errorprone, except without > the double negative. Done. https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:184: On 2015/05/15 18:37:13, jbudorick wrote: > No need for line breaks. Done. https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:230: On 2015/05/15 18:37:12, jbudorick wrote: > same Done. https://codereview.chromium.org/1136573002/diff/60001/build/java_apk.gypi#new... build/java_apk.gypi:691: 'extra_args': [ '--errorprone-path=<(errorprone_exe_path)' ], On 2015/05/15 18:37:12, jbudorick wrote: > same args comment, although this one is flipped somewhat -- the other > enable_errorprone was == 1, this one is == 0 Done.
didn't re-review yet, just following up https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:27: rt_jar_finder = re.compile(r'\[Opened (.*)/jre/lib/rt.jar\]') On 2015/05/18 19:53:23, raywilliams1 wrote: > On 2015/05/15 18:37:12, jbudorick wrote: > > Do this as a constant. > > Do you mean pull rt_jar_finder out of main() ? yes, and name it RT_JAR_FINDER https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:102: errorprone_options = [ On 2015/05/18 19:53:24, raywilliams1 wrote: > On 2015/05/15 18:37:12, jbudorick wrote: > > errorprone_options can just be a constant, right? > > Hmm... I don't think python has constants. I can kill the helper method and put > this back as a regular var (like in patch-set 1). Let me know if you want me to > do that. Python doesn't have constants in that the interpreter won't enforce constants, but conventionally ALL_CAPS_WITH_UNDERSCORES denotes a constant (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Namin...). We typically use these at module scope (though there are some uses of class-scope constants), and that's what I'd advise doing here.
Thanks for the review https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/find_... build/android/gyp/find_sun_tools_jar.py:27: rt_jar_finder = re.compile(r'\[Opened (.*)/jre/lib/rt.jar\]') On 2015/05/18 19:58:31, jbudorick wrote: > On 2015/05/18 19:53:23, raywilliams1 wrote: > > On 2015/05/15 18:37:12, jbudorick wrote: > > > Do this as a constant. > > > > Do you mean pull rt_jar_finder out of main() ? > > yes, and name it RT_JAR_FINDER Done. https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac.py File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:102: errorprone_options = [ On 2015/05/18 19:58:32, jbudorick wrote: > On 2015/05/18 19:53:24, raywilliams1 wrote: > > On 2015/05/15 18:37:12, jbudorick wrote: > > > errorprone_options can just be a constant, right? > > > > Hmm... I don't think python has constants. I can kill the helper method and > put > > this back as a regular var (like in patch-set 1). Let me know if you want me > to > > do that. > > Python doesn't have constants in that the interpreter won't enforce constants, > but conventionally ALL_CAPS_WITH_UNDERSCORES denotes a constant > (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Namin...). > We typically use these at module scope (though there are some uses of > class-scope constants), and that's what I'd advise doing here. cool - extracted constant and re-tested to make sure it works :) https://codereview.chromium.org/1136573002/diff/60001/build/android/gyp/javac... build/android/gyp/javac.py:102: errorprone_options = [ On 2015/05/18 19:58:32, jbudorick wrote: > On 2015/05/18 19:53:24, raywilliams1 wrote: > > On 2015/05/15 18:37:12, jbudorick wrote: > > > errorprone_options can just be a constant, right? > > > > Hmm... I don't think python has constants. I can kill the helper method and > put > > this back as a regular var (like in patch-set 1). Let me know if you want me > to > > do that. > > Python doesn't have constants in that the interpreter won't enforce constants, > but conventionally ALL_CAPS_WITH_UNDERSCORES denotes a constant > (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Namin...). > We typically use these at module scope (though there are some uses of > class-scope constants), and that's what I'd advise doing here. Done.
https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:42: ["java", "-verbose", "-version"], print_stderr=False, print_stdout=False) print_stdout=False is the default. https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... build/android/gyp/javac.py:59: # Something in chrome_private_java makes this check crash. We should file a chromium bug about these. And maybe file an errorprone bug, too, but we're pretty far behind their trunk and the bugs may have been fixed (and so it may not be worth tracking down a small repro case for them). https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... build/android/gyp/javac.py:205: help='Path to the errorprone compiler executable.') If this is going to make us use errorprone instead of javac, we should make that clear at least in the help message. An option name that indiciated that it was going to enable errorprone would be nice, but I can't really think of a good one that also indicates that it should contain the path... maybe: --use-errorprone --use-errorprone-path Idk. --errorprone-path is probably fine if you can't find something you like better. https://codereview.chromium.org/1136573002/diff/100001/build/config/android/i... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/i... build/config/android/internal_rules.gni:822: args += [ "--enable-errorprone" ] This condition shouldn't add anything to args https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... build/config/android/rules.gni:735: # enable_errorprone: If true, enables the errorprone compiler. What is our plan for enabling this? I don't think we really want to do it target by target, but would rather have like a global on/off flag. Is that the case? In the end I think we want a global on/off with a way for targets to explicitly opt-out (like for errorprone itself). We probably wouldn't need an explicit opt-in when it is globally off. As we ramp up, maybe we want to have the opt-in while globally off. In gyp, with this change, we essentially have global on/off with both opt-in and opt-out. What do we want in gn? https://codereview.chromium.org/1136573002/diff/100001/third_party/errorprone... File third_party/errorprone/errorprone.gyp (right): https://codereview.chromium.org/1136573002/diff/100001/third_party/errorprone... third_party/errorprone/errorprone.gyp:26: 'disable_errorprone': 1, change to: 'enable_errorprone': 0
https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:42: ["java", "-verbose", "-version"], print_stderr=False, print_stdout=False) On 2015/05/20 02:21:29, cjhopman wrote: > print_stdout=False is the default. Done. https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... build/android/gyp/javac.py:59: # Something in chrome_private_java makes this check crash. On 2015/05/20 02:21:29, cjhopman wrote: > We should file a chromium bug about these. And maybe file an errorprone bug, > too, but we're pretty far behind their trunk and the bugs may have been fixed > (and so it may not be worth tracking down a small repro case for them). Yeah, maybe this can go away if we ever switch to a newer version of errorprone. https://codereview.chromium.org/1136573002/diff/100001/build/android/gyp/java... build/android/gyp/javac.py:205: help='Path to the errorprone compiler executable.') On 2015/05/20 02:21:29, cjhopman wrote: > If this is going to make us use errorprone instead of javac, we should make that > clear at least in the help message. > > An option name that indiciated that it was going to enable errorprone would be > nice, but I can't really think of a good one that also indicates that it should > contain the path... > > maybe: > --use-errorprone > --use-errorprone-path > > Idk. --errorprone-path is probably fine if you can't find something you like > better. Interesting observation! I actually like use-errorprone-path it's like saying "we're using errorprone, and here's the path" I'll change it to use-errorprone-path for now (in other places, too). https://codereview.chromium.org/1136573002/diff/100001/build/config/android/i... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/i... build/config/android/internal_rules.gni:822: args += [ "--enable-errorprone" ] On 2015/05/20 02:21:29, cjhopman wrote: > This condition shouldn't add anything to args Done. https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... build/config/android/rules.gni:735: # enable_errorprone: If true, enables the errorprone compiler. On 2015/05/20 02:21:29, cjhopman wrote: > What is our plan for enabling this? I don't think we really want to do it target > by target, but would rather have like a global on/off flag. Is that the case? > > In the end I think we want a global on/off with a way for targets to explicitly > opt-out (like for errorprone itself). We probably wouldn't need an explicit > opt-in when it is globally off. As we ramp up, maybe we want to have the opt-in > while globally off. > > In gyp, with this change, we essentially have global on/off with both opt-in and > opt-out. What do we want in gn? Alright, I added a default to false. I wasn't sure where to put it, so I put it right under chromium_code part. https://codereview.chromium.org/1136573002/diff/100001/third_party/errorprone... File third_party/errorprone/errorprone.gyp (right): https://codereview.chromium.org/1136573002/diff/100001/third_party/errorprone... third_party/errorprone/errorprone.gyp:26: 'disable_errorprone': 1, On 2015/05/20 02:21:29, cjhopman wrote: > change to: > 'enable_errorprone': 0 Done.
https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... build/config/android/rules.gni:735: # enable_errorprone: If true, enables the errorprone compiler. On 2015/05/20 21:31:55, raywilliams1 wrote: > On 2015/05/20 02:21:29, cjhopman wrote: > > What is our plan for enabling this? I don't think we really want to do it > target > > by target, but would rather have like a global on/off flag. Is that the case? > > > > In the end I think we want a global on/off with a way for targets to > explicitly > > opt-out (like for errorprone itself). We probably wouldn't need an explicit > > opt-in when it is globally off. As we ramp up, maybe we want to have the > opt-in > > while globally off. > > > > In gyp, with this change, we essentially have global on/off with both opt-in > and > > opt-out. What do we want in gn? > > Alright, I added a default to false. > I wasn't sure where to put it, so I put it right under chromium_code part. That change seems to just set it to false for all apks. I don't think that's what we want. I think for apks, we should do the same as we are doing for java/android libraries. Add an enable_errorprone arg and pass it to the java_library_impl. To make it so that we have global on/off with explicit opt-in/out, add a flag in the declare_args block in build/config/android/config.gni (maybe use_errorprone, or use_errorprone_java_compiler, or something). Then, in compile_java where we actually decide to use errorprone or not, do: _enable_errorprone = use_errorprone_java_compiler if (defined(invoker.enable_errorprone)) { _enable_errorprone = invoker.enable_errorprone }
https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... build/config/android/rules.gni:735: # enable_errorprone: If true, enables the errorprone compiler. On 2015/05/21 22:52:24, cjhopman wrote: > That change seems to just set it to false for all apks. I don't think that's > what we want. > > I think for apks, we should do the same as we are doing for java/android > libraries. Add an enable_errorprone arg and pass it to the java_library_impl. > > To make it so that we have global on/off with explicit opt-in/out, add a flag in > the declare_args block in build/config/android/config.gni (maybe use_errorprone, > or use_errorprone_java_compiler, or something). Then, in compile_java where we > actually decide to use errorprone or not, do: > > _enable_errorprone = use_errorprone_java_compiler > if (defined(invoker.enable_errorprone)) { > _enable_errorprone = invoker.enable_errorprone > } I'm still trying to figure out how these gni/gyp/gypi files work. I'll give this another try. Thanks for the review.
https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1136573002/diff/100001/build/config/android/r... build/config/android/rules.gni:735: # enable_errorprone: If true, enables the errorprone compiler. On 2015/05/21 22:52:24, cjhopman wrote: > On 2015/05/20 21:31:55, raywilliams1 wrote: > > On 2015/05/20 02:21:29, cjhopman wrote: > > > What is our plan for enabling this? I don't think we really want to do it > > target > > > by target, but would rather have like a global on/off flag. Is that the > case? > > > > > > In the end I think we want a global on/off with a way for targets to > > explicitly > > > opt-out (like for errorprone itself). We probably wouldn't need an explicit > > > opt-in when it is globally off. As we ramp up, maybe we want to have the > > opt-in > > > while globally off. > > > > > > In gyp, with this change, we essentially have global on/off with both opt-in > > and > > > opt-out. What do we want in gn? > > > > Alright, I added a default to false. > > I wasn't sure where to put it, so I put it right under chromium_code part. > > That change seems to just set it to false for all apks. I don't think that's > what we want. > > I think for apks, we should do the same as we are doing for java/android > libraries. Add an enable_errorprone arg and pass it to the java_library_impl. > > To make it so that we have global on/off with explicit opt-in/out, add a flag in > the declare_args block in build/config/android/config.gni (maybe use_errorprone, > or use_errorprone_java_compiler, or something). Then, in compile_java where we > actually decide to use errorprone or not, do: > > _enable_errorprone = use_errorprone_java_compiler > if (defined(invoker.enable_errorprone)) { > _enable_errorprone = invoker.enable_errorprone > } Done.
lgtm https://codereview.chromium.org/1136573002/diff/160001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/1136573002/diff/160001/build/config/android/c... build/config/android/config.gni:46: use_errorprone_java_compiler = false Add a comment above this. The comment will be displayed when someone does `gn help args` https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... build/config/android/internal_rules.gni:753: _enable_errorprone = false this should be: _enable_errorprone = use_errorprone_java_compiler https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... build/config/android/internal_rules.gni:955: _enable_errorprone = use_errorprone_java_compiler delete this line and just forward the invoker's enable_errorprone. The compile_java template can handle checking the global flag vs the invoker's
Thanks for the review :) https://codereview.chromium.org/1136573002/diff/160001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/1136573002/diff/160001/build/config/android/c... build/config/android/config.gni:46: use_errorprone_java_compiler = false On 2015/05/26 22:05:32, cjhopman wrote: > Add a comment above this. The comment will be displayed when someone does `gn > help args` Done. https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... build/config/android/internal_rules.gni:753: _enable_errorprone = false On 2015/05/26 22:05:32, cjhopman wrote: > this should be: > _enable_errorprone = use_errorprone_java_compiler Done. https://codereview.chromium.org/1136573002/diff/160001/build/config/android/i... build/config/android/internal_rules.gni:955: _enable_errorprone = use_errorprone_java_compiler On 2015/05/26 22:05:32, cjhopman wrote: > delete this line and just forward the invoker's enable_errorprone. The > compile_java template can handle checking the global flag vs the invoker's Done.
https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:24: 'action\'s first output.') nit: indent this line to where the help string starts on the preceding line https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:49: sun_tools_jar_path = os.path.join(match.group(1), 'lib', 'tools.jar') nit: just return os.path.join(...) here and return None outside of the for loop. https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/java... File build/android/gyp/javac.py (right): https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/java... build/android/gyp/javac.py:59: # Something in chrome_private_java makes this check crash. This comment needs to be updated, as this target no longer exists. Also, I think this was a downstream target, which we probably shouldn't reference upstream. https://codereview.chromium.org/1136573002/diff/200001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/200001/build/java_apk.gypi#ne... build/java_apk.gypi:256: 'dependencies': [ Why is this only added to dependencies here in java_apk.gypi? Should we also be doing this in host_jar.gypi and java.gypi?
https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:24: 'action\'s first output.') On 2015/05/28 15:38:00, jbudorick wrote: > nit: indent this line to where the help string starts on the preceding line Done. https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:49: sun_tools_jar_path = os.path.join(match.group(1), 'lib', 'tools.jar') On 2015/05/28 15:38:00, jbudorick wrote: > nit: just return os.path.join(...) here and return None outside of the for loop. I like that idea https://codereview.chromium.org/1136573002/diff/200001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:49: sun_tools_jar_path = os.path.join(match.group(1), 'lib', 'tools.jar') On 2015/05/28 15:38:00, jbudorick wrote: > nit: just return os.path.join(...) here and return None outside of the for loop. Done. https://codereview.chromium.org/1136573002/diff/200001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/200001/build/java_apk.gypi#ne... build/java_apk.gypi:256: 'dependencies': [ On 2015/05/28 15:38:00, jbudorick wrote: > Why is this only added to dependencies here in java_apk.gypi? Should we also be > doing this in host_jar.gypi and java.gypi? added to conditions in the other two jars https://codereview.chromium.org/1136573002/diff/200001/build/java_apk.gypi#ne... build/java_apk.gypi:256: 'dependencies': [ On 2015/05/28 15:38:00, jbudorick wrote: > Why is this only added to dependencies here in java_apk.gypi? Should we also be > doing this in host_jar.gypi and java.gypi? Done.
https://codereview.chromium.org/1136573002/diff/220001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/220001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:46: match = re.match(RT_JAR_FINDER, ln) nit: match = RT_JAR_FINDER.match(ln) https://codereview.chromium.org/1136573002/diff/220001/third_party/errorprone... File third_party/errorprone/errorprone.gyp (right): https://codereview.chromium.org/1136573002/diff/220001/third_party/errorprone... third_party/errorprone/errorprone.gyp:12: 'jar_path': 'error_prone_core-1.1.2.jar', (couldn't comment directly on the jar) Thanks for reminding me about the separate repository. crbug/488591 is tracking that. Once it gets created, the jar should go in there, and then this CL should DEPS in that repo.
https://codereview.chromium.org/1136573002/diff/220001/build/android/gyp/find... File build/android/gyp/find_sun_tools_jar.py (right): https://codereview.chromium.org/1136573002/diff/220001/build/android/gyp/find... build/android/gyp/find_sun_tools_jar.py:46: match = re.match(RT_JAR_FINDER, ln) On 2015/06/01 22:28:42, jbudorick wrote: > nit: match = RT_JAR_FINDER.match(ln) Done. https://codereview.chromium.org/1136573002/diff/220001/third_party/errorprone... File third_party/errorprone/errorprone.gyp (right): https://codereview.chromium.org/1136573002/diff/220001/third_party/errorprone... third_party/errorprone/errorprone.gyp:12: 'jar_path': 'error_prone_core-1.1.2.jar', On 2015/06/01 22:28:42, jbudorick wrote: > (couldn't comment directly on the jar) > > Thanks for reminding me about the separate repository. crbug/488591 is tracking > that. Once it gets created, the jar should go in there, and then this CL should > DEPS in that repo. Done.
https://codereview.chromium.org/1136573002/diff/240001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/240001/build/java_apk.gypi#ne... build/java_apk.gypi:211: 'enable_errorprone%': '1', Should be '0'
https://codereview.chromium.org/1136573002/diff/240001/build/java_apk.gypi File build/java_apk.gypi (right): https://codereview.chromium.org/1136573002/diff/240001/build/java_apk.gypi#ne... build/java_apk.gypi:211: 'enable_errorprone%': '1', On 2015/06/04 14:00:14, jbudorick wrote: > Should be '0' Done.
lgtm
raywilliams@chromium.org changed reviewers: + cpu@chromium.org
1- can you add more context on the bug what this does, why we needed it etc? 2- have the open source folks ok the license https://www.chromium.org/developers/adding-3rd-party-libraries
On 2015/06/12 21:30:28, cpu wrote: > 1- can you add more context on the bug what this does, why we needed it etc? > > 2- have the open source folks ok the license Okay, I've added more information to the bug and added open-source-third-party-reviews to this review.
raywilliams@chromium.org changed reviewers: + open-source-third-party-reviews@google.com
raywilliams@chromium.org changed reviewers: - open-source-third-party-reviews@google.com
On 2015/06/15 17:28:22, raywilliams1 wrote: > On 2015/06/12 21:30:28, cpu wrote: > > 1- can you add more context on the bug what this does, why we needed it etc? > > > > 2- have the open source folks ok the license > > Okay, I've added more information to the bug > and added open-source-third-party-reviews to this review. Actually, I've taken open-source-third-party-reviews back off since they've already responded to an earlier version of this saying that this has no third party files in it (since Errorprone was made by Google).
On 2015/06/15 17:33:50, raywilliams1 wrote: > On 2015/06/15 17:28:22, raywilliams1 wrote: > > On 2015/06/12 21:30:28, cpu wrote: > > > 1- can you add more context on the bug what this does, why we needed it etc? > > > > > > 2- have the open source folks ok the license > > > > Okay, I've added more information to the bug > > and added open-source-third-party-reviews to this review. > > Actually, I've taken open-source-third-party-reviews back off > since they've already responded to an earlier version of this saying that > this has no third party files in it (since Errorprone was made by Google). Here's the earlier review with the response from open-source-third-party-reviews https://codereview.chromium.org/1133673002
On 2015/06/15 at 17:37:46, raywilliams wrote: > On 2015/06/15 17:33:50, raywilliams1 wrote: > > On 2015/06/15 17:28:22, raywilliams1 wrote: > > > On 2015/06/12 21:30:28, cpu wrote: > > > > 1- can you add more context on the bug what this does, why we needed it etc? > > > > > > > > 2- have the open source folks ok the license > > > > > > Okay, I've added more information to the bug > > > and added open-source-third-party-reviews to this review. > > > > Actually, I've taken open-source-third-party-reviews back off > > since they've already responded to an earlier version of this saying that > > this has no third party files in it (since Errorprone was made by Google). > > Here's the earlier review with the response from open-source-third-party-reviews > https://codereview.chromium.org/1133673002 would you mind sending this to chrome-eng-review@ first (cf https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review)
On 2015/06/17 08:04:29, jochen wrote: > On 2015/06/15 at 17:37:46, raywilliams wrote: > > On 2015/06/15 17:33:50, raywilliams1 wrote: > > > On 2015/06/15 17:28:22, raywilliams1 wrote: > > > > On 2015/06/12 21:30:28, cpu wrote: > > > > > 1- can you add more context on the bug what this does, why we needed it > etc? > > > > > > > > > > 2- have the open source folks ok the license > > > > > > > > Okay, I've added more information to the bug > > > > and added open-source-third-party-reviews to this review. > > > > > > Actually, I've taken open-source-third-party-reviews back off > > > since they've already responded to an earlier version of this saying that > > > this has no third party files in it (since Errorprone was made by Google). > > > > Here's the earlier review with the response from > open-source-third-party-reviews > > https://codereview.chromium.org/1133673002 > > would you mind sending this to chrome-eng-review@ first (cf > https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review) This was sent to chrome-eng-review some months ago. See message titled "[Android] error-prone java compiler" from me.
On 2015/06/17 20:19:57, cjhopman wrote: > On 2015/06/17 08:04:29, jochen wrote: > > On 2015/06/15 at 17:37:46, raywilliams wrote: > > > On 2015/06/15 17:33:50, raywilliams1 wrote: > > > > On 2015/06/15 17:28:22, raywilliams1 wrote: > > > > > On 2015/06/12 21:30:28, cpu wrote: > > > > > > 1- can you add more context on the bug what this does, why we needed > it > > etc? > > > > > > > > > > > > 2- have the open source folks ok the license > > > > > > > > > > Okay, I've added more information to the bug > > > > > and added open-source-third-party-reviews to this review. > > > > > > > > Actually, I've taken open-source-third-party-reviews back off > > > > since they've already responded to an earlier version of this saying that > > > > this has no third party files in it (since Errorprone was made by Google). > > > > > > Here's the earlier review with the response from > > open-source-third-party-reviews > > > https://codereview.chromium.org/1133673002 > > > > would you mind sending this to chrome-eng-review@ first (cf > > > https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review) > > This was sent to chrome-eng-review some months ago. See message titled > "[Android] error-prone java compiler" from me. 11/19/14
I had forgotten. lgtm
The CQ bit was checked by raywilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org, cpu@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1136573002/#ps300001 (title: "Full Sync and Update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136573002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by raywilliams@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136573002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/6ffb1179d6adf08edd64848b45b7415b6b6de43d Cr-Commit-Position: refs/heads/master@{#335509}
Message was sent while issue was closed.
When adding a new DEPS plz remember to add also the project to .gitignore (as indicated in the comment on top of DEPS *) or you end up causing dev's checkouts to be in a stale state. There is no need to do for this one, this is now being addressed in crrev.com/1209933002 * "When adding a new dependency, please update the top-level .gitignore file to list the dependency's destination directory. 9 # to list the dependency's destination directory."
Message was sent while issue was closed.
When adding a new DEPS plz remember to add also the project to .gitignore (as indicated in the comment on top of DEPS *) or you end up causing dev's checkouts to be in a stale state. There is no need to do for this one, this is now being addressed in crrev.com/1209933002 * "When adding a new dependency, please update the top-level .gitignore file to list the dependency's destination directory. 9 # to list the dependency's destination directory." |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
