Closed Bug 1512487 Opened Last year Closed 4 months ago

Integrate android and GeckoView lints with phabricator

Categories

(GeckoView :: General, enhancement, P3)

enhancement

Tracking

(firefox65 wontfix, firefox69 wontfix, firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox65 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: Agi, Assigned: nalexander, Mentored)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(6 files, 8 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
We should make |mach lint| understand our lints so that we can run them as part of normal try builds (right now they only run as part of a specific target) and while sending reviews to phabricator.
From what I can see we should be able to do this as long as we let gradle work without a build-id. Nick said he's going to dump some information here.
Flags: needinfo?(nalexander)
(In reply to Agi [:agi] from comment #1)
> From what I can see we should be able to do this as long as we let gradle
> work without a build-id. Nick said he's going to dump some information here.

I think there's more going on here than meets the eye.  The reason that the |mach android-*| tasks are defined in taskcluster/ci/build/android-stuff.yml (and not in taskcluster/ci/source-test with the rest of the lints) is that these tasks look like artifact (really, !COMPILE_ENVIRONMENT) _builds_.  They require toolchains; they have a mozconfig; they need to do certain build steps.  They're build-like, not lint-like/static-analysis-like.

Now, the build steps required are not _that_ complicated: AFAIK, |mach build export| is sufficient to get the build ID _and_ the other bits and pieces (strings.xml, etc).  But that's not really guaranteed, at least not right now (and not in the past).

So there are a lot of things in play:

1) the Gradle integration really wants a mozconfig, i.e., to be a build.  With Fennec, the mozconfig determined features (both in the Java code and in the Gecko code) -- but that's much less true with GeckoView (which has very few Java code features).

2) the JVM-level "lints" all expect to compile JVM-level code before doing static analysis.  That is, the analysis is mostly on the .class files, not on the .java files.  (That's not true for checkstyle, but it's the odd one out).

3) the linting tools don't really fit into the |mach lint| patterns -- it's not easy to run per-file lints and the lints generally don't map well to per-diff-hunk display.  My guess is that this can be addressed, although it will require a bunch of fiddly report transformations to match Gradle outputs (which are all tool-specific, sigh) to whatever |mach lint| can report.

My read here is that we can make |mach lint| do more for mobile/android builds and get the integration we want.  Probably |mach lint| and other static analysis tools already set patterns for this, since we do some native code analysis that requires the clang plugin already (AFAIK).  (That didn't exist when I made these all build jobs, way back when.)
Flags: needinfo?(nalexander)
Thanks for the write up Nick!

I noticed most of these while trying to move the Android lint to |mach lint| :) I'm almost questioning if we want to do this at this point, especially because running the full lint suite is kinda slow (a couple of minutes right now with apilint and the Android lint).

> My read here is that we can make |mach lint| do more for mobile/android builds and get the integration we want.

Running |mach build export| (TIL) seems to be enough for android lint, so that's nice. Were you thinking of adding that as a dependency of |mach lint|?

> Probably |mach lint| and other static analysis tools already set patterns for this, since we do some native code analysis that requires the clang plugin already (AFAIK).  (That didn't exist when I made these all build jobs, way back when.)

Not that I can see. All lints in //tools/lint are very simple static checks that don't require any build stuff (maybe I'm missing something?) I'll look into this more.

> 3) the linting tools don't really fit into the |mach lint| patterns -- it's not easy to run per-file lints and the lints generally don't map well to per-diff-hunk display.  My guess is that this can be addressed, although it will require a bunch of fiddly report transformations to match Gradle outputs (which are all tool-specific, sigh) to whatever |mach lint| can report.

From what I can see the only way is to run everything whenever the right files are modified, which could be good enough for us. The mapping is actually not too bad and the code is mostly an adaptation from [0] e.g. this would be the mapping for Android Lint:

for issue in root.findall("issue"):
    err = { 
        'level': issue.get("severity").lower(),
        'rule': issue.get("id"),
        'message': issue.get("message"),
        'path': issue[0].get("file").replace(self.topsrcdir, ""),
        'lineno': int(issue[0].get("line") or 0), 
    }   
    results.append(result.from_config(config, **err))


[0]: https://searchfox.org/mozilla-central/source/mobile/android/mach_commands.py#116
(In reply to Agi [:agi] from comment #3)
> Thanks for the write up Nick!
> 
> I noticed most of these while trying to move the Android lint to |mach lint|
> :) I'm almost questioning if we want to do this at this point, especially
> because running the full lint suite is kinda slow (a couple of minutes right
> now with apilint and the Android lint).

I more or less concur.  When I did this (with mcomella, IIRC) many moons ago, we decided not to integrate with |mach lint| for all these reasons.  Of course, the world has moved forward...

> > My read here is that we can make |mach lint| do more for mobile/android builds and get the integration we want.
> 
> Running |mach build export| (TIL) seems to be enough for android lint, so
> that's nice. Were you thinking of adding that as a dependency of |mach lint|?

I wasn't thinking of making it a dependency up front, but more of a prerequisite.  I.e., for mobile/android, |mach lint| requires a build (and package?) to succeed.  Just like |mach android lint| does... although that actually does the Gradle build.  Which, IIRC, doesn't actually succeed on a tree that has had |mach configure| but not |mach build export|.  Tricky stuff!

> > Probably |mach lint| and other static analysis tools already set patterns for this, since we do some native code analysis that requires the clang plugin already (AFAIK).  (That didn't exist when I made these all build jobs, way back when.)
> 
> Not that I can see. All lints in //tools/lint are very simple static checks
> that don't require any build stuff (maybe I'm missing something?) I'll look
> into this more.

Oh.  That's unfortunate.  We'll have to dig more into this.

> > 3) the linting tools don't really fit into the |mach lint| patterns -- it's not easy to run per-file lints and the lints generally don't map well to per-diff-hunk display.  My guess is that this can be addressed, although it will require a bunch of fiddly report transformations to match Gradle outputs (which are all tool-specific, sigh) to whatever |mach lint| can report.
> 
> From what I can see the only way is to run everything whenever the right
> files are modified, which could be good enough for us. The mapping is
> actually not too bad and the code is mostly an adaptation from [0] e.g. this
> would be the mapping for Android Lint:
> 
> for issue in root.findall("issue"):
>     err = { 
>         'level': issue.get("severity").lower(),
>         'rule': issue.get("id"),
>         'message': issue.get("message"),
>         'path': issue[0].get("file").replace(self.topsrcdir, ""),
>         'lineno': int(issue[0].get("line") or 0), 
>     }   
>     results.append(result.from_config(config, **err))

That's not bad, then.  We should probably try to move this into Gradle itself: if we could migrate to Kotlin for our build.gradle files, then we'd get better type-checking and be more likely to get the bits right over time.  But that's strictly separate!

So, next steps: how about you investigate whether we have any "whole tree" or "compilation" lints, and how those feel; and I'll re-acquaint myself with the |mach lint| mechanism and think about what it takes to run the |mach android *| commands via |mach lint| as well.
Agi says P3 because this might be nice-to-have if we can address the `mach lint` issues.
Priority: -- → P3
Product: Firefox for Android → GeckoView

(In reply to Nick Alexander :nalexander [he/him] from comment #4)

Probably |mach lint| and other static analysis tools already set patterns for this, since we do some native code analysis that requires the clang plugin already (AFAIK). (That didn't exist when I made these all build jobs, way back when.)

Not that I can see. All lints in //tools/lint are very simple static checks
that don't require any build stuff (maybe I'm missing something?) I'll look
into this more.

Oh. That's unfortunate. We'll have to dig more into this.

Just noticed that ./mach static-analysis check <file> does more or less what our lint does and it's integrated with phabricator, so it should be possible to do this!

I think the way to go for this is:

We create a job that runs for code-review tasks and dumps the results in /public/code-review/android-lint.json or something, similar to the json output for lints.

See this for example: https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/taskcluster/ci/source-test/clang.yml#1-57

This would give us comments in phabricator for lints failure which is what we really want.

Summary: Integrate android and GeckoView lints with mach lint → Integrate android and GeckoView lints with phabricator
Assignee: agi → nobody
Mentor: agi

Feel free to needinfo me, or hit me up on #mobile on IRC if you want to work on this.

This was Gradle-only and then !Gradle-only. Now Gradle is required
and this is unused.

Assignee: nobody → nalexander

See also https://github.com/mozilla-mobile/gradle-apilint/issues/61, which tracks exposing file/line numbers of offending signature changes and is needed for decent Phab integration. I see no way to surface conflicting "API version" information; if the line isn't updated in a posted patch, the mismatch doesn't have an "anchor point" for Phab to attach the conflict comment to. This is a fundamental limitation of Phab that I am not aware of any short or long term plans to address.

Just like Bug 1405408, which was for private toolchains.

Depends on D31381

Attachment #9065296 - Attachment description: Bug 1512487 - Part 1: Move 64_api_lint.py mozharness configuration to YAML. → Bug 1512487 - Convert android-specific code analyses into mozlints.
Attachment #9065299 - Attachment description: Bug 1512487 - DO NOT LAND - Make api-lint fail. → Bug 1512487 - DO NOT LAND - Make api-lint, lint, checkstyle, findbugs, test fail.
Attachment #9065295 - Attachment description: Bug 1512487 - Pre: Remove unused android-api-16-gradle mozconfig. → Bug 1512487 - Pre: Remove unused android-api-16-gradle mozconfig. r?agi
Attachment #9065296 - Attachment description: Bug 1512487 - Convert android-specific code analyses into mozlints. → Bug 1512487 - Pre: Support private fetches. r?tomprince

It's not worth accommodating all the ways to invoke commands from
Python, so expose the lock itself so that consumers can use
subprocess, Popen, etc as they choose.

Depends on D31382

This allows lints to "condition" themselves on having a build
environment or a specific build application. It also adds the "name"
parameter, so that setup functions can be shared across lints.

MozbuildObject cannot be used as parameters to functions distributed
via multiprocessing, since they cannot be pickled (due, currently, to
internal terminal handles). Therefore we extract just a few key
parts of the environment to expose.

Depends on D35273

Global lints never shard into chunks; they are, by definition global
and act on all inputs at one time. It's up to the global task to
manage command line sizes, etc.

This patch avoids running global lints unless a file or directory that
matches their configuration (via extensions and exclude) has been
modified or specified.

Depends on D35274

Attachment #9065296 - Attachment description: Bug 1512487 - Pre: Support private fetches. r?tomprince → Bug 1512487 - Part 3: Convert Android-specific code analyses into mozlints. r?ahal,agi
Attachment #9065297 - Attachment is obsolete: true
Attachment #9065298 - Attachment is obsolete: true
Attachment #9065299 - Attachment is obsolete: true
Attachment #9065300 - Attachment is obsolete: true
Attachment #9065296 - Attachment description: Bug 1512487 - Part 3: Convert Android-specific code analyses into mozlints. r?ahal,agi → Bug 1512487 - Pre: Support private fetches. r?tomprince

API lint is arguably the most valuable lint of all, but it's also hard
to fit into the Phab ecosystem, since there's no place to hang the
"API hash not correct" message in the case when the hash hasn't been
updated at all. Therefore, this commit doesn't convert it. See also
https://github.com/mozilla-mobile/gradle-apilint/issues/61 for adding
file/line information to API lint.

Depends on D35275

Comment on attachment 9069558 [details]
Bug 1512487 - Pre: Support private fetches. r?tomprince

Revision D33588 was moved to bug 1569032. Setting attachment 9069558 [details] to obsolete.

Attachment #9069558 - Attachment is obsolete: true
Attachment #9065296 - Attachment is obsolete: true
Attachment #9069559 - Attachment is obsolete: true

Just FYI, folks: I'm hitting the first round of review comments and will push a fresh stack shortly. ahal, your suggestions have made the global lint stuff much cleaner \o/. I'm hoping Agi will push this across the line after I refresh.

Attachment #9072718 - Attachment description: Bug 1512487 - Part 2: Accommodate "global" lints. r?ahal → Bug 1512487 - Part 2: Add "global" lint type. r?ahal

It's a pity that Mach's conditions facility can't handle subcommands,
but it's a deep enough limitation that it's not worth addressing for
this patch.

Depends on D35277

Here's a try build with lints that should fail; this one is looking like I expect:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=258544547&revision=53dac49036dad73629cc064108cc3bd6136c9a16

Here's a try build with no failing lints; this one shows a race/timeout issue that I've addressed locally:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=258544547&revision=63d1bee6bd8e827fa1272d9fd3d8d10a5a36df4a

I'll refresh the stack and then it's over to you, Agi!

Assignee: nalexander → agi
Status: NEW → ASSIGNED
Attachment #9082084 - Attachment is obsolete: true
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2891ae9a4a6
Pre: Remove unused android-api-16-gradle mozconfig. r=agi
https://hg.mozilla.org/integration/autoland/rev/d4dfcec89e3d
Pre: Expose `gradle_lock` to consumers. r=agi
https://hg.mozilla.org/integration/autoland/rev/f9cdcb926d1a
Part 1: Allow lints to inspect part of the build environment. r=ahal
https://hg.mozilla.org/integration/autoland/rev/49e9a49784bb
Part 2: Add "global" lint type. r=ahal
https://hg.mozilla.org/integration/autoland/rev/c78639b3c8ab
Part 3: Convert Android-specific code analyses into mozlints. r=ahal,agi
https://hg.mozilla.org/integration/autoland/rev/576589b8cb89
Part 4: Deprecate `mach android {api-lint,checkstyle,findbugs,lint,test}`. r=agi

🎉🎉🎉 I'm so excited about this, thanks Nick!

Assignee: agi → nalexander

Clap clap, results will be displayed here: https://static-analysis.moz.tools/

Regressions: 1571308
Regressions: 1571326
Regressions: 1571492

firefox69=wontfix because we don't need to uplift these lints to Beta.

You need to log in before you can comment on or make changes to this bug.