novoda / gradle-static-analysis-plugin

Easy setup of static analysis tools for Android and Java projects.
Apache License 2.0
410 stars 28 forks source link

Create separate evaluation tasks per tool #196

Closed lgvalle closed 5 years ago

lgvalle commented 5 years ago

Problem

The Static Analysis Plugin serves two main purposes:

  1. A common language to configure and collect results from static analysis tools
  2. A task (evaluateViolations) to execute all those analyses in one go.

The problem is that some projects may be interested in those static analysis tasks separately per tool. For example, to parallelise them in CI pipelines or to defer the execution of the most expensive ones to later build stages.

This was raised here too https://github.com/novoda/gradle-static-analysis-plugin/issues/164

Solution

This is my naive approach to the problem. It creates separate tasks per analysis tool:

evaluateLintViolations
evaluatePMDViolations
evaluateFindbugsViolations
evaluateCheckstyleViolations
evaluateDetektViolations
evaluateKtLintViolations

and makes the original evaluateViolations task depends on all of them.

The solution Works For Me but it's incomplete existing tests pass, but there are no new ones If this feature is considered interesting for the project I'm happy to pair to make it better.

Generated tasks

These are the tasks generated for the sample project inside this repo before and after the change. Notice the new evaluate<TOOL>Violation tasks. (Sorry for the hacks to align code blocks side by side on markdown :P)

Before After
Verification tasks
------------------
check
checkstyleAndroidTest
checkstyleAndroidTestDebug
checkstyleDebug
checkstyleMain
checkstyleRelease
checkstyleTest
checkstyleTestDebug
checkstyleTestRelease
collectcheckstyleDebugAndroidTestVariantViolations
collectcheckstyleDebugUnitTestVariantViolations
collectcheckstyleDebugVariantViolations
collectpmdDebugAndroidTestVariantViolations
collectpmdDebugUnitTestVariantViolations
collectpmdDebugVariantViolations
connectedAndroidTest
connectedCheck
connectedDebugAndroidTest
detekt
detektBaseline
detektGenerateConfig
detektIdeaFormat
detektIdeaInspect
deviceAndroidTest
deviceCheck
evaluateViolations
findbugsDebug
findbugsDebugAndroidTest
findbugsDebugUnitTest
ktlintCheck
ktlintDebugCheck
ktlintReleaseCheck
lint
lintDebug
lintRelease
pmdAndroidTest
pmdAndroidTestDebug
pmdDebug
pmdMain
pmdRelease
pmdTest
pmdTestDebug
pmdTestRelease
test
testDebugUnitTest
testReleaseUnitTest
.
.
.
.
.
.
Verification tasks
------------------
check
checkstyleAndroidTest
checkstyleAndroidTestDebug
checkstyleDebug
checkstyleMain
checkstyleRelease
checkstyleTest
checkstyleTestDebug
checkstyleTestRelease
collectcheckstyleDebugAndroidTestVariantViolations
collectcheckstyleDebugUnitTestVariantViolations
collectcheckstyleDebugVariantViolations
collectpmdDebugAndroidTestVariantViolations
collectpmdDebugUnitTestVariantViolations
collectpmdDebugVariantViolations
connectedAndroidTest
connectedCheck
connectedDebugAndroidTest
detekt
detektBaseline
detektGenerateConfig
detektIdeaFormat
detektIdeaInspect
deviceAndroidTest
deviceCheck
evaluateCheckstyleViolations
evaluateDetektViolations
evaluateFindbugsViolations
evaluateKtLintViolations
evaluateLintViolations
evaluatePMDViolations
evaluateViolations
findbugsDebug
findbugsDebugAndroidTest
findbugsDebugUnitTest
ktlintCheck
ktlintDebugCheck
ktlintReleaseCheck
lint
lintDebug
lintRelease
pmdAndroidTest
pmdAndroidTestDebug
pmdDebug
pmdMain
pmdRelease
pmdTest
pmdTestDebug
pmdTestRelease
test
testDebugUnitTest
testReleaseUnitTest

Now we can execute individual evaluate<TOOL>Violations tasks separately or the main evaluateViolations which triggers all the others.

Sample: evaluateCheckstyleViolations

If we choose to execute only an individual task, it runs only its tool analysis and produces only results for itself:

./gradlew evaluateCheckstyleViolations

> Task :app:evaluateCheckstyleViolations

Violations found (1 errors, 0 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html

BUILD SUCCESSFUL in 1s
3 actionable tasks: 2 executed, 1 up-to-date

EvaluateViolations

For existing users changes in this PR are transparent. They'll see individual tasks reporting messages, but the main one will remain exactly the same. Executing evaluateViolations with the same project will produce:

./gradlew evaluateViolations
Starting a Gradle Daemon, 1 busy and 1 stopped Daemons could not be reused, use --status for details

> Configure project :app

> Task :app:evaluateCheckstyleViolations

Violations found (1 errors, 0 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html

> Task :app:evaluateDetektViolations

Violations found (0 errors, 26 warnings)

> Detekt violations found (0 errors, 26 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html

> Task :app:evaluateFindbugsViolations

Violations found (0 errors, 2 warnings)

> Findbugs violations found (0 errors, 2 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html

> Task :app:lint
Ran lint on variant release: 1 issues found
Ran lint on variant debug: 1 issues found
Wrote HTML report to file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
Wrote XML report to file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.xml

> Task :app:evaluateLintViolations

Violations found (1 errors, 0 warnings)

> Lint violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html

> Task :app:evaluatePMDViolations

Violations found (1 errors, 13 warnings)

> PMD violations found (1 errors, 13 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html

(...)

> Task :app:evaluatektlintViolations

Violations found (37 errors, 0 warnings)

> ktlint violations found (37 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

> Task :app:evaluateViolations

Violations found (40 errors, 41 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
> Detekt violations found (0 errors, 26 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html
> Findbugs violations found (0 errors, 2 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html
> Lint violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
> PMD violations found (1 errors, 13 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html
> ktlint violations found (37 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

BUILD SUCCESSFUL in 3s
61 actionable tasks: 15 executed, 46 up-to-date
tasomaniac commented 5 years ago

As mentioned in the original issue, the reporting behavior may change. We used to provide all reports at once at the end when everything is finished. Would be good if you can provide before/after screenshots of outputs.

On the side, my personal opinion is that this does not add much value.

High level of parallelization is already provided by Gradle. I am not sure if this improves it.

The way to make check faster (particularly on CI) is to keep modules smaller. When Gradle modules are small, the mentioned overhead is matter of seconds.

lgvalle commented 5 years ago

@tasomaniac thanks for your comments. Here's a use case that can benefit from this feature IMHO:

With this change, you could potentially do on your CI:

  1. All static analysis (except lint)
  2. All unit tests
  3. Lint

So you have very fast feedback for most cases instead of waiting for lint to finish before running tests that may fail.

tasomaniac commented 5 years ago

Hmm, it's unfortunate that lint is very slow and not cacheable πŸ˜” but that's the only one. Tests will not wait until lint is finished. Tests will run separately and in parallel, no?

tasomaniac commented 5 years ago

On a 2nd thought and looking at the implementation (very clever solution btw) I think this is a good change. Especially for those projects where lint is taking extremely long. For regular projects tho, we should keep it backwards compatible.

lgvalle commented 5 years ago

Yep, unfortunately, you're right @tasomaniac : tests will run separately and in parallel :(

I've updated the ticket's description to include a list of generated tasks. A good thing about this change is that it's 100% backward compatible (to my best knowledge) since it makes evaluateViolations depends on all the other evaluate<tool>Violations.

tasomaniac commented 5 years ago

It could be that the CLI output may be different.

lgvalle commented 5 years ago

Good news and bad news.

Good

Running evaluateViolations on master and this branch produces the same final output:

> Task :app:evaluateViolations

Violations found (40 errors, 41 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
> Detekt violations found (0 errors, 26 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html
> Findbugs violations found (0 errors, 2 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html
> Lint violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
> PMD violations found (1 errors, 13 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html
> ktlint violations found (37 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

Bad

On this branch, each individual steps prints a summary as well (since they really are evaluateViolations with one single configured tool) -

> Task :app:evaluateLintViolations

Violations found (39 errors, 28 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
> Detekt violations found (0 errors, 26 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html
> Findbugs violations found (0 errors, 2 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html
> Lint violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
> ktlint violations found (37 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

> Task :app:evaluatePMDViolations

Violations found (40 errors, 41 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
> Detekt violations found (0 errors, 26 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html
> Findbugs violations found (0 errors, 2 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html
> Lint violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
> PMD violations found (1 errors, 13 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html
> ktlint violations found (37 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

(...)

❌ This is not ok. Each of them should only print a summary for its tool and the last one, a total summary. Let's see if I can figure this out

tasomaniac commented 5 years ago

What about this? These individual tasks would print (and also stop the build) only when the number of errors/warnings exceeds the limit.

If they don't exceed, the evaluateViolations would print a summary of all tools.

lgvalle commented 5 years ago

@tasomaniac I've updated the code a bit to improve reporting and updated the PR description

tasomaniac commented 5 years ago

I think this looks good and is a very clever solution to the problem we have at hand. I would just modify the default evaluator to skip total count when there is a single tool report.

Instead of

Violations found (1 errors, 0 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html

Have

Checkstyle violations found (1 errors, 0 warnings). See the reports at:
- file:///Users/luisvalle/Developer/workspace/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
tasomaniac commented 5 years ago

Hope you don't mind pushing to your branch. I just did my requested change. Here is how the output looks like now. Summary parts are not printed for individual tasks.

Will approve. Please have a look at my change and merge if you think it's alright.

> Task :app:evaluateCheckstyleViolations

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html

> Task :app:evaluateDetektViolations

> Detekt violations found (0 errors, 26 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html

> Task :app:findbugsDebug
(..)
> Task :app:evaluateFindbugsViolations

> Findbugs violations found (0 errors, 2 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html

> Task :app:lint
(..)
> Task :app:evaluateLintViolations

> Lint violations found (1 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html

> Task :app:evaluatePMDViolations

> PMD violations found (1 errors, 13 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html

> Task :app:ktlintMainSourceSetCheck
(..)
> Task :app:evaluatektlintViolations

> ktlint violations found (37 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html

> Task :app:evaluateViolations

Violations found (40 errors, 41 warnings)

> Checkstyle violations found (1 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/checkstyle/main.html
> Detekt violations found (0 errors, 26 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/detekt/detekt.html
> Findbugs violations found (0 errors, 2 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/findbugs/debug.html
> Lint violations found (1 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/lint-results.html
> PMD violations found (1 errors, 13 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/pmd/main.html
> ktlint violations found (37 errors, 0 warnings). See the reports at:
file:///Users/sdane/dev/taso/gradle-static-analysis-plugin/sample/app/build/reports/ktlint/ktlintMainSourceSetCheck.html
lgvalle commented 5 years ago

Thanks very much for contributing @tasomaniac 😊 The dash was definitely annoying, I'm glad to see it removed. I've fixed a couple of tests that were broken after the change (they were comparing report outputs)

tasomaniac commented 5 years ago

Damn. Sorry about that. Do you think we should get Toto's opinion about this. Can you ping him to review ☺️