Closed Bug 1258769 Opened 4 years ago Closed 4 years ago

Add checkstyle gradle task

Categories

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

All
Android
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

Save us some time in review.
I found the task configuration from:
  http://vincentbrison.com/2014/07/19/how-to-improve-quality-and-syntax-of-your-android-code/#The_Gradle_way

The official documentation, however, indicates this should be easier (i.e.
mostly automatic):
  https://docs.gradle.org/current/userguide/checkstyle_plugin.html

There are TOODs left in the code where I don't know how to set up the optimal
task.

For the addition of future checks, it's worth noting Google's config is
available:
  https://github.com/checkstyle/checkstyle/blob/3e4367941c3e9680703e8ea8400abbd5dc78e1d9/src/main/resources/google_checks.xml

And this version contains links with descriptions of each of the tasks:
  http://checkstyle.sourceforge.net/google_style.html

Review commit: https://reviewboard.mozilla.org/r/41787/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41787/
Attachment #8733474 - Flags: review?(nalexander)
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

https://reviewboard.mozilla.org/r/41789/#review38217

Land this (maybe "No bug"), since it's obviously wrong.
Attachment #8733475 - Flags: review?(nalexander) → review+
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

https://reviewboard.mozilla.org/r/41787/#review38219

Let's do this right, or at least close to right, by using an existing Android/Gradle plugin rather than hand-hacking a checkstyle task.  There are many such plugins, but https://github.com/dlabs/soter looks pretty comprehensive and was updated just weeks ago.  Can you try it out?
Attachment #8733474 - Flags: review?(nalexander)
Attachment #8733474 - Attachment description: MozReview Request: Bug 1258769 - Add checkstyle gradle task with simple checks. r=nalexander → MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander
Attachment #8733474 - Flags: review?(nalexander)
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41787/diff/1-2/
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41789/diff/1-2/
Good call on the plugin, Nick.
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

https://reviewboard.mozilla.org/r/41787/#review38319

I don't want to land this as is, for reasons explained below.  We can either stick it in tree, or do a little legwork to generalize the repository definition to be a list.  I'm happy with either.

::: build.gradle:30
(Diff revision 2)
>          // For android-sdk-manager SNAPSHOT releases.
>          maven {
>              url "file://${gradle.mozconfig.topsrcdir}/mobile/android/gradle/m2repo"
>          }
> +        // For soter
> +        maven {

nit: full sentence above.

I'm not strongly against adding another Maven repository, but I'd prefer not to.  How do you feel about shipping this plugin in-tree?  If we don't, we'll need to make `GRADLE_MAVEN_REPOSITORY` a list of repositories, since this plugin *is not* in jcenter.

We can't just add this line since automation needs to use snapshotted dependencies.  I just added documentation explaining how to fetch dependencies for this situation: see https://dxr.mozilla.org/mozilla-central/source/build/docs/toolchains.rst in fx-team (or that link tomorrow).

::: mobile/android/app/build.gradle:341
(Diff revision 2)
>      android.sourceSets."${productFlavor}${buildType.capitalize()}".assets.srcDir syncOmnijarFromDistDir.destinationDir
>      android.sourceSets."${productFlavor}${buildType.capitalize()}".assets.srcDir syncAssetsFromDistDir.destinationDir
>      android.sourceSets."${productFlavor}${buildType.capitalize()}".jniLibs.srcDir syncLibsFromDistDir.destinationDir
>  }
>  
> +apply plugin: 'si.dlabs.soter'

These parts look fine.
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41787/diff/2-3/
Attachment #8733474 - Flags: review?(nalexander)
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41789/diff/2-3/
Comment on attachment 8733575 [details]
MozReview Request: Bug 1258769 - Don't wrap import line. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41817/diff/1-2/
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

https://reviewboard.mozilla.org/r/41787/#review38847

Well done, Mike!  There are some extra files that need to go before landing, but this looks good.  The license is Apache, so we're good to ship it in tree.  (This is compile-time only, and a check, like lint, so I'm not concerned about shipping source in tree.)

I'd like to see the pre: commits land first, since this commit will fail without them.  You'll need to remove another tab in `mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckForUpdatesAction.java` that crept in.

Finally, please include the invocation line in the commit message: `./mach gradle app:checkstyle` worked for me.

Will you file follow-up to make this a TC task?  If you wanted to add it to the existing task, it would be easy -- just update https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/builds/releng_sub_android_configs/64_api_15_frontend.py#10.  But I'd rather split the TC task into 3 to make this clear.  They're all lightning fast anyway.

::: build.gradle:43
(Diff revision 3)
>              // Without these, we get errors linting.
>              exclude module: 'guava'
>          }
>          // Provided in tree.
>          classpath 'com.jakewharton.sdkmanager:gradle-plugin:1.5.0-SNAPSHOT'
> +        classpath "gradle.plugin.si.dlabs.gradle:soter:1.0.6"

I see you added toplevel soter/.  It's not necessary, remove those files.  (Probably an hg error.)

::: mobile/android/app/checkstyle.xml:36
(Diff revision 3)
> +-->
> +
> +<module name="Checker">
> +    <property name="charset" value="UTF-8"/>
> +
> +    <!-- TODO: <property name="fileExtensions" value="java, properties, xml"/> -->

I wanted to see a failure, so I uncommented this -- it didn't fail :)
Attachment #8733474 - Flags: review?(nalexander) → review+
https://reviewboard.mozilla.org/r/41787/#review38847

> I wanted to see a failure, so I uncommented this -- it didn't fail :)

I left this out because I wasn't entirely certain what it does. I guessed it is a filter on the types of files checkstyle will look for so if you run the task on `mobile/*`, it'll only run it on files called `*.java`, `*.properties`, and `*.xml`.

However, soter might only pass it `*.java` anyway (even though checkstyle could look at our xml resources too!) so there's more investigation work to do here.
https://reviewboard.mozilla.org/r/41787/#review38847

> The license is Apache, so we're good to ship it in tree.  (This is compile-time only, and a check, like lint, so I'm not concerned about shipping source in tree.)

On github, [the license was MIT](https://github.com/dlabs/soter/blob/master/LICENSE). I noticed the jar does not ship with the license inside so I added the license file directly to the directory we distribute it in. If this was incorrect, we can fix it later.

> Will you file follow-up to make this a TC task?

bug 1258787.
This caused some failures:

18:56:04     INFO -  A problem occurred configuring root project 'src'.
18:56:04     INFO -  > Could not resolve all dependencies for configuration ':classpath'.
18:56:04     INFO -     > Could not find si.dlabs:aws-java-sdk-s3:1.9.35.
18:56:04     INFO -       Searched in the following locations:
18:56:04     INFO -           http://localhost:8081/nexus/content/repositories/central/si/dlabs/aws-java-sdk-s3/1.9.35/aws-java-sdk-s3-1.9.35.pom
18:56:04     INFO -           http://localhost:8081/nexus/content/repositories/central/si/dlabs/aws-java-sdk-s3/1.9.35/aws-java-sdk-s3-1.9.35.jar
18:56:04     INFO -           file:/home/worker/workspace/build/src/mobile/android/gradle/m2repo/si/dlabs/aws-java-sdk-s3/1.9.35/aws-java-sdk-s3-1.9.35.pom
18:56:04     INFO -           file:/home/worker/workspace/build/src/mobile/android/gradle/m2repo/si/dlabs/aws-java-sdk-s3/1.9.35/aws-java-sdk-s3-1.9.35.jar
18:56:04     INFO -       Required by:
18:56:04     INFO -           :src:unspecified > gradle.plugin.si.dlabs.gradle:soter:1.0.6
18:56:04     INFO -     > Could not find com.github.blazsolar.gradle:gradle-hipchat-plugin:0.2.0.
18:56:04     INFO -       Searched in the following locations:
18:56:04     INFO -           http://localhost:8081/nexus/content/repositories/central/com/github/blazsolar/gradle/gradle-hipchat-plugin/0.2.0/gradle-hipchat-plugin-0.2.0.pom
18:56:04     INFO -           http://localhost:8081/nexus/content/repositories/central/com/github/blazsolar/gradle/gradle-hipchat-plugin/0.2.0/gradle-hipchat-plugin-0.2.0.jar
18:56:04     INFO -           file:/home/worker/workspace/build/src/mobile/android/gradle/m2repo/com/github/blazsolar/gradle/gradle-hipchat-plugin/0.2.0/gradle-hipchat-plugin-0.2.0.pom
18:56:04     INFO -           file:/home/worker/workspace/build/src/mobile/android/gradle/m2repo/com/github/blazsolar/gradle/gradle-hipchat-plugin/0.2.0/gradle-hipchat-plugin-0.2.0.jar
18:56:04     INFO -       Required by:
18:56:04     INFO -           :src:unspecified > gradle.plugin.si.dlabs.gradle:soter:1.0.6

The pom file specifies some dependencies, which I must have downloaded locally from their repositories.

Backing out...
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41789/diff/3-4/
Attachment #8733475 - Attachment description: MozReview Request: Bug 1258769 - Remove tab from RemoteTabs* to pass checkstyle check. r=nalexander → MozReview Request: Bug 1258769 - Add soter dependencies. r=nalexander
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41787/diff/3-4/
Attachment #8733575 - Attachment is obsolete: true
Attachment #8733474 - Flags: review+ → review?(nalexander)
Attachment #8733475 - Flags: review+ → review?(nalexander)
Comment on attachment 8733474 [details]
MozReview Request: Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander

Already r+'d.
Attachment #8733474 - Flags: review?(nalexander) → review+
Nick, I tried to use your mechanism in comment 9 to download the dependencies, but it doesn't seem to be working (or I have a conceptual misunderstanding).

I added the try job (android-api-15-gradle-dependencies) and inspected the task output but the maven local repository is missing (and noticeably my build failed and yours didn't). I ended up resolving the dependencies by hand, but how can I do this better in the future?
Flags: needinfo?(nalexander)
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

https://reviewboard.mozilla.org/r/41789/#review39355

So, this soter thing doesn't look too good.  hipchat?  Some (apparently unused) thing?  A custom httpclient repack?  I looked further and this is some shop's internal plugin dumping ground.  I see other, perhaps better, plugins, but perhaps let's just go back to the `checkstyle` plugin directly, and work to normalize our sources over time.  Exhibit A: move `mobile/android/{search,stumbler}` into the main source code directories.

Sorry for the goose chase!
Attachment #8733475 - Flags: review?(nalexander)
Spoke on irc – we should try (in this order):
* use other plugins (e.g. https://github.com/noveogroup/android-check)
* Steal their file glob strategies (e.g. https://github.com/noveogroup/android-check/blob/master/android-check-plugin/src/main/groovy/com/noveogroup/android/check/checkstyle/CheckstyleCheck.groovy#L56)
* Try this thing (http://ncona.com/2015/01/enforce-android-coding-style-with-checkstyle/) – it's can break things since we're subtracting away Java functionality, however
* Use the approach I took originally
Clearing NI.  mcomella, feel free to flag me for your original approach again, with the TC changes to run it.  Let's get something out there rather than digging through the gradle-plugin muck.
Flags: needinfo?(nalexander)
mcomella: hey, I'd really like this to check our license headers (outside of thirdparty) and test headers (in tests/).  Can that be done?
Flags: needinfo?(michael.l.comella)
Blocks: 1258787
Flags: needinfo?(michael.l.comella)
(In reply to Nick Alexander :nalexander from comment #33)
> mcomella: hey, I'd really like this to check our license headers (outside of
> thirdparty) and test headers (in tests/).  Can that be done?

Filed bug 1261485.
I'm not sure what you mean by TC changes (maybe you're thinking of bug 1260874?), but I'll reflag with my original approach.
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41789/diff/4-5/
Attachment #8733475 - Attachment description: MozReview Request: Bug 1258769 - Add soter dependencies. r=nalexander → MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander
Attachment #8733475 - Flags: review?(nalexander)
Attachment #8733474 - Attachment is obsolete: true
Comment on attachment 8733475 [details]
MozReview Request: Bug 1258769 - Add checkstyle gradle task. r=nalexander

https://reviewboard.mozilla.org/r/41789/#review40521

wfm.  Be sure to push to try to see if you need to update the Gradle dependencies in the tooltool manifests.

For future use, see http://stackoverflow.com/a/20362150 for how to make an HTML report, or how to post-process the XML report to print errors.
Attachment #8733475 - Flags: review?(nalexander) → review+
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 48 → mozilla48
You need to log in before you can comment on or make changes to this bug.