Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1870073002:
Properly set up classpath for Android Lint (and fix its stderr filter) (Closed)
DescriptionProperly set up classpath for Android Lint (and fix its stderr filter)
Lint's stderr filter actually was dropping all output of the tool
instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this
revealed a lot of internal lint errors.
Most of them were CRC errors from interface JARs, so I switched back to
use normal JARs.
Others were internal errors of InvalidPackageName check which was trying
to check Android SDK classes, because these classes were added to
classpath as a jar. It turns out that Lint treats SDK jar differently but
only if it is properly specified. The only way to tell Lint which SDK
jar to use is to create project.properties file (like in old Ant-based
build). So lint.py now accepts SDK version as a command-line argument
and creates dummy project.properties in temporary folder.
BUG=599052
Committed: https://crrev.com/ef5e20f9fe84651ca03fb53ed15910121cedfc0a
Cr-Commit-Position: refs/heads/master@{#386188}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Skip --android-sdk-version for GYP build #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 ========== to ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 ==========
mlopatkin@yandex-team.ru changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org
Please take a look. This is Lint fix, as a promised in https://codereview.chromium.org/1828693002 I cannot run try jobs. Could you please run any necessary ones?
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870073002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870073002/1
https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py File build/android/gyp/lint.py (right): https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:159: r'^.*_JAVA_OPTIONS.*\n', '', l, flags=re.MULTILINE) nit: I don't think you need MULTILINE unless you use ^ or $. (which might be worth adding) https://codereview.chromium.org/1870073002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (left): https://codereview.chromium.org/1870073002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:95: "--classpath=['$rebased_android_sdk_jar']", do we not need to pass android.jar at all now? I think the .gyp version will still have it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1870073002/diff/1/build/android/lint_action.gypi File build/android/lint_action.gypi (right): https://codereview.chromium.org/1870073002/diff/1/build/android/lint_action.g... build/android/lint_action.gypi:40: '--android-sdk-version=<(android_sdk_version)', Since at the moment lint warnings fail GYP builds but not GN builds, I think the best way to move forward is to revert this change so that GYP still compiles but the warnings show up for GN. There's a separate bug for fixing these new warnings and turning on --can-fail-build for GN. https://codereview.chromium.org/1853463002/
https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py File build/android/gyp/lint.py (right): https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:159: r'^.*_JAVA_OPTIONS.*\n', '', l, flags=re.MULTILINE) On 2016/04/08 13:40:29, agrieve wrote: > nit: I don't think you need MULTILINE unless you use ^ or $. (which might be > worth adding) My intention was to replace all lines that contain '_JAVA_OPTIONS' to nothing (including end-of-line symbol); I was using MULTILINE to make ^ match any line, not only the first line, but it turns out that ^ isn't necessary at all: first '.*' matches from the beginning of the line to the first '_JAVA_OPTIONS'. https://codereview.chromium.org/1870073002/diff/1/build/android/lint_action.gypi File build/android/lint_action.gypi (right): https://codereview.chromium.org/1870073002/diff/1/build/android/lint_action.g... build/android/lint_action.gypi:40: '--android-sdk-version=<(android_sdk_version)', On 2016/04/08 15:16:12, agrieve wrote: > Since at the moment lint warnings fail GYP builds but not GN builds, I think the > best way to move forward is to revert this change so that GYP still compiles but > the warnings show up for GN. There's a separate bug for fixing these new > warnings and turning on --can-fail-build for GN. > > https://codereview.chromium.org/1853463002/ We can make --android-sdk-version optional for now and pass it only in GN build like we already do with classpath jars. I've updated CL to do so. https://codereview.chromium.org/1870073002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (left): https://codereview.chromium.org/1870073002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:95: "--classpath=['$rebased_android_sdk_jar']", On 2016/04/08 13:40:29, agrieve wrote: > do we not need to pass android.jar at all now? I think the .gyp version will > still have it. That's the point of the CL. We should not pass android.jar in --libraries; we specify target in project.properties instead and then Lint itself find android.jar in the SDK and adds it to the classpath. So Lint doesn't run class-level checks on android.jar.
lgtm. Thanks for fixing this up! https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py File build/android/gyp/lint.py (right): https://codereview.chromium.org/1870073002/diff/1/build/android/gyp/lint.py#n... build/android/gyp/lint.py:159: r'^.*_JAVA_OPTIONS.*\n', '', l, flags=re.MULTILINE) On 2016/04/08 16:55:16, mlopatkin wrote: > On 2016/04/08 13:40:29, agrieve wrote: > > nit: I don't think you need MULTILINE unless you use ^ or $. (which might be > > worth adding) > > My intention was to replace all lines that contain '_JAVA_OPTIONS' to nothing > (including end-of-line symbol); I was using MULTILINE to make ^ match any line, > not only the first line, but it turns out that ^ isn't necessary at all: first > '.*' matches from the beginning of the line to the first '_JAVA_OPTIONS'. oh garbage. Sorry about that. I just didn't see the ^ the first time around.
The CQ bit was checked by mlopatkin@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlopatkin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870073002/20001
Message was sent while issue was closed.
Description was changed from ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 ========== to ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 ========== to ========== Properly set up classpath for Android Lint (and fix its stderr filter) Lint's stderr filter actually was dropping all output of the tool instead of just single 'Picking up _JAVA_OPTIONS' line. Fixing this revealed a lot of internal lint errors. Most of them were CRC errors from interface JARs, so I switched back to use normal JARs. Others were internal errors of InvalidPackageName check which was trying to check Android SDK classes, because these classes were added to classpath as a jar. It turns out that Lint treats SDK jar differently but only if it is properly specified. The only way to tell Lint which SDK jar to use is to create project.properties file (like in old Ant-based build). So lint.py now accepts SDK version as a command-line argument and creates dummy project.properties in temporary folder. BUG=599052 Committed: https://crrev.com/ef5e20f9fe84651ca03fb53ed15910121cedfc0a Cr-Commit-Position: refs/heads/master@{#386188} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef5e20f9fe84651ca03fb53ed15910121cedfc0a Cr-Commit-Position: refs/heads/master@{#386188} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
