Closed
Bug 1258769
Opened 8 years ago
Closed 8 years ago
Add checkstyle gradle task
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
Save us some time in review.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
The checkstyle task should pass now. Review commit: https://reviewboard.mozilla.org/r/41789/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41789/
Attachment #8733475 -
Flags: review?(nalexander)
Assignee | ||
Updated•8 years ago
|
Blocks: checkstyle
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
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)
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
The checkstyle checks should pass now. Review commit: https://reviewboard.mozilla.org/r/41817/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41817/
Attachment #8733575 -
Flags: review?(nalexander)
Assignee | ||
Comment 8•8 years ago
|
||
Good call on the plugin, Nick.
Updated•8 years ago
|
Attachment #8733474 -
Flags: review?(nalexander)
Comment 9•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8733575 -
Flags: review?(nalexander) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8733575 [details] MozReview Request: Bug 1258769 - Don't wrap import line. r=nalexander https://reviewboard.mozilla.org/r/41817/#review38321 Land it.
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c2bcf970d89ef1b0ec535525db0b8aa36d71b452 Bug 1258769 - Remove tabs from various files to pass checkstyle check. r=nalexander https://hg.mozilla.org/integration/fx-team/rev/84371bcce3dbce17b00011f5aaee9363935fe42b Bug 1258769 - Don't wrap import line. r=nalexander https://hg.mozilla.org/integration/fx-team/rev/fe11700b6cdfdb0fca3a9b2a54bf6d339792df24 Bug 1258769 - Use soter to add checkstyle gradle task with simple checks. r=nalexander
Assignee | ||
Comment 18•8 years ago
|
||
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...
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/81d754a0681160c34aa9ad4ba9d398d0d149eb7b Bug 1258769 - Backed out changeset fe11700b6cdf.
Assignee | ||
Comment 20•8 years ago
|
||
Full dependency list: https://github.com/dlabs/soter/blob/master/build.gradle#L40
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01df7136ee93
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05adc0939fd9
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3cc7298c27
Assignee | ||
Comment 24•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fde8692acfc
Assignee | ||
Comment 25•8 years ago
|
||
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
Assignee | ||
Comment 26•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8733575 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8733474 -
Flags: review+ → review?(nalexander)
Assignee | ||
Updated•8 years ago
|
Attachment #8733475 -
Flags: review+ → review?(nalexander)
Assignee | ||
Comment 27•8 years ago
|
||
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+
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2bcf970d89e https://hg.mozilla.org/mozilla-central/rev/84371bcce3db https://hg.mozilla.org/mozilla-central/rev/fe11700b6cdf https://hg.mozilla.org/mozilla-central/rev/81d754a06811
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
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
Comment 32•8 years ago
|
||
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)
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
(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.
Assignee | ||
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8733474 -
Attachment is obsolete: true
Comment 37•8 years ago
|
||
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+
Assignee | ||
Comment 38•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd4b65b7165
Assignee | ||
Comment 39•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/20df8310f734ecc938f1ad53e27759253ae14040 Bug 1258769 - Add checkstyle gradle task. r=nalexander
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20df8310f734
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•5 years ago
|
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.
Description
•