Closed Bug 1365878 Opened 7 years ago Closed 7 years ago

Change severity of NewApi lint checks from warning to error

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Version:
55 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tyu, Assigned: tyu)

References

Details

Attachments

(1 file)

Bug 1365878 - Change severity of NewApi lint checks from warning to error;
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
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.
Assignee: nobody → osimpleo
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.
Flags: needinfo?(nalexander)
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!
Flags: needinfo?(nalexander)
Filed Bug 1366644 to track the tasks regarding upgrading Android Gradle plugin to 2.3+
Keywords: checkin-needed
Blocks: lint-newapi
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3b98429334b7
Change severity of NewApi lint checks from warning to error; r=nalexander
Keywords: checkin-needed
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?
Flags: needinfo?(osimpleo)
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
Flags: needinfo?(osimpleo)
<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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f65733ecdd9a
Change severity of NewApi lint checks from warning to error; r=nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f65733ecdd9a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.

Attachment

General

Creator:
Created:
Updated:
Size: