Change severity of NewApi lint checks from warning to error

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tyu, Assigned: tyu)

Tracking

(Blocks: 1 bug)

55 Branch
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → osimpleo
(Assignee)

Comment 2

2 years ago
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 3

2 years ago
mozreview-review
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!
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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 7

2 years ago
mozreview-review
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)
(Assignee)

Comment 9

2 years ago
Filed Bug 1366644 to track the tasks regarding upgrading Android Gradle plugin to 2.3+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1218477

Comment 10

2 years ago
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)
I ended up backing this out so I could get a clean merge to mozilla-central.

https://hg.mozilla.org/mozilla-central/rev/d712c82c59ec5a277047a75d09bec48be4a64b87
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(osimpleo)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
<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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f65733ecdd9a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.