Closed Bug 1365878 Opened 5 years ago Closed 5 years ago
Change severity of New
Api lint checks from warning to error
59 bytes, text/x-review-board-request
To block future unchecked NewApi issues, we should change the severity to error so that the lint check will fail and in turn fail the gradle build.
There are 3 ways to white list, the best would be (1) utilizing baseline of gradle plugin 2.3 (See: https://bugzilla.mozilla.org/show_bug.cgi?id=1235670#c3 ) However we're currently equipped with 2.1.3, so we have only the two following options: (2) Use issue.ignore in lint.xml (3) Add java @TargetApi (@SuppressLint is not an option because it is unblocking some prospective issues) annotations and tools:ignore attributes I have picked (2) because although latter is more precise (will not let any new issues pass through) but it is harder to trace and we might utilize baseline in the future. I'm also utilizing the lint ignore path wild card because we'll need to change our code base structure otherwise. (See: http://stackoverflow.com/questions/43994420/what-path-is-the-issue-ignore-path-element-in-lint-xml-relative-to )
Comment on attachment 8868957 [details] Bug 1365878 - Change severity of NewApi lint checks from warning to error; https://reviewboard.mozilla.org/r/140622/#review144146 I'm fine with this. Thanks for taking some ownership of the lint configuration! You might run this by mcomella as well, who has traditionally owned the lint configuration.
Attachment #8868957 - Flags: review?(nalexander) → review+
(In reply to Teng-pao Yu [:tyu] from comment #2) > There are 3 ways to white list, the best would be (1) utilizing baseline of > gradle plugin 2.3 (See:[ > https://bugzilla.mozilla.org/show_bug.cgi?id=1235670#c3 ) Thanks for digging into this. It might not be hard to upgrade Gradle to version 2.3; the hardest part will be updating the build machines -- but there's some documentation on how to achieve that in https://gecko.readthedocs.io/en/latest/build/buildsystem/toolchains.html#firefox-for-android-with-gradle. I'd love you to try driving the upgrade. You'd need to: * bump the Gradle wrapper locally; * fix any local build fallout; * try to push to try with "hg push-to-try -m "try: -b o -p android-api-15-gradle-dependencies"; * upload the new archives to tooltool; * push try builds to the builders that use the new tooltool archives. This isn't _hard_, but it's a little involved. Follow https://bugzilla.mozilla.org/show_bug.cgi?id=1268453 to see how to go about this. I can help!
Resend a review request since I feel like adding what I elaborated in the comments would be a good idea :) Regarding the gradle plugin upgrade, I have a brief discussion with the Taipei Fennec front-end team and Julian think it'd be a good idea to upgrade it later so https://bugzilla.mozilla.org/show_bug.cgi?id=1254353 would not be interfered. @nalexander What's your opinion on this statement? It might also be worthy to bring up that Android Studio 3.0 was just fresh launched and maybe it is also a good idea to wait for it to move to stable channel so we can go directly to 3.0.
Comment on attachment 8868957 [details] Bug 1365878 - Change severity of NewApi lint checks from warning to error; https://reviewboard.mozilla.org/r/140622/#review144614
(In reply to Teng-pao Yu [:tyu] from comment #6) > Resend a review request since I feel like adding what I elaborated in the > comments would be a good idea :) > > Regarding the gradle plugin upgrade, I have a brief discussion with the > Taipei Fennec front-end team and Julian think it'd be a good idea to upgrade > it later so https://bugzilla.mozilla.org/show_bug.cgi?id=1254353 would not > be interfered. @nalexander What's your opinion on this statement? I doubt that bumping the Gradle version and the Android-Gradle plugin version will impact Bug 1254353, but it might. We could at least do the try builds to figure out if there are problems, independent of Bug 1254353. > It might also be worthy to bring up that Android Studio 3.0 was just fresh > launched and maybe it is also a good idea to wait for it to move to stable > channel so we can go directly to 3.0. I think this is a fine idea: - wait until 3.0 is stable - (optionally) wait until Bug 1254353 is landed - figure out what the safest versions of Gradle and the Android-Gradle plugin for use with AS 3.0 are - update Fennec to safe versions of Gradle and the Android-Gradle plugin Could you file a ticket to track that work? Thanks!
Filed Bug 1366644 to track the tasks regarding upgrading Android Gradle plugin to 2.3+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/3b98429334b7 Change severity of NewApi lint checks from warning to error; r=nalexander
The whitelist doesn't seem to be extensive enough, as this causes lint failures on treeherder
Changing from warning to error has, unsurprisingly resulted in a lint error being reported https://queue.taskcluster.net/v1/task/LemHNwGoTB2LbUBpolmS3g/runs/0/artifacts/public/android/lint/lint-results-officialAustralisDebug.html Can you either fix it or I'll back out the patch?
I ended up backing this out so I could get a clean merge to mozilla-central. https://hg.mozilla.org/mozilla-central/rev/d712c82c59ec5a277047a75d09bec48be4a64b87
Sorry the whitelist should not have contained the "mozilla-central/" part which can usually be renamed. Just received my try permission and tried this new patch with try: -b a -p all -u none -t none --artifact https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc62f263642e75b5711407dfb3db04070710c71&selectedJob=101447660
<ignore path="**/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java"/> This is also added in the previous commit since this is a recently added issue after last review.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f65733ecdd9a Change severity of NewApi lint checks from warning to error; r=nalexander
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.