Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847823003:
🍫 Enable support-annotations lint checks (Closed)
|
Created:
4 years, 8 months ago by agrieve Modified:
4 years, 8 months ago Reviewers:
jbudorick CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lint-swiper Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable support-annotations and android framework lint checks
annotations such as: @IntDef, @MainThread, etc
warnings such as: HandlerLeak, MissingSuperCall, ValidFragment
Problem was that we were not passing lint the classpath.
This change is GN-only since GYP lint warnings fail the build.
Will follow-up with a GYP change once warnings are fixed.
BUG=599052
Committed: https://crrev.com/eb3dd1ad7c04c091faf285aa2f78da0b7cec7a76
Cr-Commit-Position: refs/heads/master@{#385373}
Patch Set 1 #
Total comments: 6
Patch Set 2 : redo ijars #
Total comments: 4
Patch Set 3 : review nit #Patch Set 4 : add --classpath for gyp #Patch Set 5 : add android.jar, revert .gypi change #
Messages
Total messages: 30 (11 generated)
Description was changed from ========== Fix Android Lint not checking @IntDef usage Problem was that we were not passing it the classpath. BUG=599052 ========== to ========== Fix Android Lint not checking @IntDef usage Problem was that we were not passing it the classpath. BUG=599052 ==========
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
Description was changed from ========== Fix Android Lint not checking @IntDef usage Problem was that we were not passing it the classpath. BUG=599052 ========== to ========== Enable support-annotations lint checks Problem was that we were not passing lint the classpath. BUG=599052 ==========
Description was changed from ========== Enable support-annotations lint checks Problem was that we were not passing lint the classpath. BUG=599052 ========== to ========== Enable support-annotations lint checks @IntDef, @MainThread, etc. Problem was that we were not passing lint the classpath. BUG=599052 ==========
On 2016/03/31 19:22:24, agrieve wrote: > Description was changed from > > ========== > Enable support-annotations lint checks > > Problem was that we were not passing lint the classpath. > > BUG=599052 > ========== > > to > > ========== > Enable support-annotations lint checks > > @IntDef, @MainThread, etc. > > Problem was that we were not passing lint the classpath. > > BUG=599052 > ==========
https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py File build/android/gyp/lint.py (right): https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:83: # TODO(agrieve): Figure out how IDEs make do incremental lint. s/make// ? https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:103: # --libraries is the classpath (excluding active target). this naming is now less than stellar, with jar_path going to --classpath and classpath going to --libraries, but I guess that's more of an issue with lint's bad command line arguments. https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:290: classpath = [re.sub(r'\.jar$', '.interface.jar', p) for p in classpath] Do we know that a corresponding .interface.jar class will always exist? What about having the build system pass the .interface.jar if it wants to use that? I'd prefer not having the script automagically use something other than what gets passed.
https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py File build/android/gyp/lint.py (right): https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:83: # TODO(agrieve): Figure out how IDEs make do incremental lint. On 2016/03/31 20:05:26, jbudorick wrote: > s/make// ? Done. https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:103: # --libraries is the classpath (excluding active target). On 2016/03/31 20:05:26, jbudorick wrote: > this naming is now less than stellar, with jar_path going to --classpath and > classpath going to --libraries, but I guess that's more of an issue with lint's > bad command line arguments. Agree. Took me many minutes of reading through its source to figure out what the difference was between --classpath and --libraries https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:290: classpath = [re.sub(r'\.jar$', '.interface.jar', p) for p in classpath] On 2016/03/31 20:05:26, jbudorick wrote: > Do we know that a corresponding .interface.jar class will always exist? What > about having the build system pass the .interface.jar if it wants to use that? > I'd prefer not having the script automagically use something other than what > gets passed. Done, but had to rebase onto: https://codereview.chromium.org/1846113002/ (also for your review :P)
On 2016/04/01 00:45:07, agrieve wrote: > https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py > File build/android/gyp/lint.py (right): > > https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... > build/android/gyp/lint.py:83: # TODO(agrieve): Figure out how IDEs make do > incremental lint. > On 2016/03/31 20:05:26, jbudorick wrote: > > s/make// ? > > Done. > > https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... > build/android/gyp/lint.py:103: # --libraries is the classpath (excluding active > target). > On 2016/03/31 20:05:26, jbudorick wrote: > > this naming is now less than stellar, with jar_path going to --classpath and > > classpath going to --libraries, but I guess that's more of an issue with > lint's > > bad command line arguments. > > Agree. Took me many minutes of reading through its source to figure out what the > difference was between --classpath and --libraries > > https://codereview.chromium.org/1847823003/diff/1/build/android/gyp/lint.py#n... > build/android/gyp/lint.py:290: classpath = [re.sub(r'\.jar$', '.interface.jar', > p) for p in classpath] > On 2016/03/31 20:05:26, jbudorick wrote: > > Do we know that a corresponding .interface.jar class will always exist? What > > about having the build system pass the .interface.jar if it wants to use that? > > I'd prefer not having the script automagically use something other than what > > gets passed. > > Done, but had to rebase onto: https://codereview.chromium.org/1846113002/ (also > for your review :P) 🎒
https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:83: _rebased_build_config = rebase_path(invoker.build_config, root_build_dir) nit: move this down by _rebased_java_files https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:95: "--classpath=@FileArg($_rebased_build_config:javac:interface_classpath)", Should there be an equivalent change to //build/android/lint_action.gypi?
https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:83: _rebased_build_config = rebase_path(invoker.build_config, root_build_dir) On 2016/04/05 13:11:20, jbudorick wrote: > nit: move this down by _rebased_java_files Done. https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:95: "--classpath=@FileArg($_rebased_build_config:javac:interface_classpath)", On 2016/04/05 13:11:20, jbudorick wrote: > Should there be an equivalent change to //build/android/lint_action.gypi? If we cared about GYP, yes. It doesn't break without it though.
On 2016/04/05 13:40:02, agrieve wrote: > https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:83: _rebased_build_config = > rebase_path(invoker.build_config, root_build_dir) > On 2016/04/05 13:11:20, jbudorick wrote: > > nit: move this down by _rebased_java_files > > Done. > > https://codereview.chromium.org/1847823003/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:95: > "--classpath=@FileArg($_rebased_build_config:javac:interface_classpath)", > On 2016/04/05 13:11:20, jbudorick wrote: > > Should there be an equivalent change to //build/android/lint_action.gypi? > > If we cared about GYP, yes. It doesn't break without it though. we do care about gyp until the trybots & main waterfall bots are switched over.
er, done that is.
lgtm
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847823003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847823003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Description was changed from ========== Enable support-annotations lint checks @IntDef, @MainThread, etc. Problem was that we were not passing lint the classpath. BUG=599052 ========== to ========== Enable support-annotations and android framework lint checks annotations such as: @IntDef, @MainThread, etc warnings such as: HandlerLeak, MissingSuperCall, ValidFragment Problem was that we were not passing lint the classpath. This change is GN-only since GYP lint warnings fail the build. Will follow-up with a GYP change once warnings are fixed. BUG=599052 ==========
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1847823003/#ps80001 (title: "add android.jar, revert .gypi change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847823003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847823003/80001
Message was sent while issue was closed.
Description was changed from ========== Enable support-annotations and android framework lint checks annotations such as: @IntDef, @MainThread, etc warnings such as: HandlerLeak, MissingSuperCall, ValidFragment Problem was that we were not passing lint the classpath. This change is GN-only since GYP lint warnings fail the build. Will follow-up with a GYP change once warnings are fixed. BUG=599052 ========== to ========== Enable support-annotations and android framework lint checks annotations such as: @IntDef, @MainThread, etc warnings such as: HandlerLeak, MissingSuperCall, ValidFragment Problem was that we were not passing lint the classpath. This change is GN-only since GYP lint warnings fail the build. Will follow-up with a GYP change once warnings are fixed. BUG=599052 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable support-annotations and android framework lint checks annotations such as: @IntDef, @MainThread, etc warnings such as: HandlerLeak, MissingSuperCall, ValidFragment Problem was that we were not passing lint the classpath. This change is GN-only since GYP lint warnings fail the build. Will follow-up with a GYP change once warnings are fixed. BUG=599052 ========== to ========== Enable support-annotations and android framework lint checks annotations such as: @IntDef, @MainThread, etc warnings such as: HandlerLeak, MissingSuperCall, ValidFragment Problem was that we were not passing lint the classpath. This change is GN-only since GYP lint warnings fail the build. Will follow-up with a GYP change once warnings are fixed. BUG=599052 Committed: https://crrev.com/eb3dd1ad7c04c091faf285aa2f78da0b7cec7a76 Cr-Commit-Position: refs/heads/master@{#385373} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/eb3dd1ad7c04c091faf285aa2f78da0b7cec7a76 Cr-Commit-Position: refs/heads/master@{#385373}
Message was sent while issue was closed.
Can this issue cause errors on Android GN build? ChromeSyncShellTest https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/33660
Message was sent while issue was closed.
On 2016/04/06 06:49:47, loyso wrote: > Can this issue cause errors on Android GN build? ChromeSyncShellTest > https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/33660 To add to this ^^ It is only on the Android GN bot. Since this is a GN-only change (and I'm not sure if the other commits in this range are suspect) it seems like a potential candidate. The error seems to be a missing *.isolate file.
Message was sent while issue was closed.
On 2016/04/06 06:52:16, cblume wrote: > On 2016/04/06 06:49:47, loyso wrote: > > Can this issue cause errors on Android GN build? ChromeSyncShellTest > > https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/33660 > > To add to this ^^ > It is only on the Android GN bot. Since this is a GN-only change (and I'm not > sure if the other commits in this range are suspect) it seems like a potential > candidate. > > The error seems to be a missing *.isolate file. My best guess is that this was caused by a change to the recipes to use wrapper scripts: https://codereview.chromium.org/1855663002/ Might be that the BUILD.gn for chrome_sync_shell has the wrong .isolate path. I'll look at this in about an hour if no one else gets a chance first.
Message was sent while issue was closed.
On 2016/04/06 12:41:03, agrieve wrote: > On 2016/04/06 06:52:16, cblume wrote: > > On 2016/04/06 06:49:47, loyso wrote: > > > Can this issue cause errors on Android GN build? ChromeSyncShellTest > > > > https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/33660 > > > > To add to this ^^ > > It is only on the Android GN bot. Since this is a GN-only change (and I'm not > > sure if the other commits in this range are suspect) it seems like a potential > > candidate. > > > > The error seems to be a missing *.isolate file. > > My best guess is that this was caused by a change to the recipes to use wrapper > scripts: https://codereview.chromium.org/1855663002/ > > Might be that the BUILD.gn for chrome_sync_shell has the wrong .isolate path. > > I'll look at this in about an hour if no one else gets a chance first. yeah, the GN side has the wrong isolate file path here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/BUI... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
