novoda / gradle-static-analysis-plugin

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

evaluateViolations doesn't stop w/ errors? #221

Closed kenyee closed 2 years ago

kenyee commented 4 years ago

Not sure how to debug this so I'm hoping someone has seen this. This is using the latest 1.2 version of the plugin.

When I run ./gradlew evaluateViolations, it claims the build is successful. When I do a --dry-run, it seems to run the correct tasks: :app:ktlintDebugSourceSetCheck SKIPPED :app:collectKtlintDebugViolations SKIPPED :app:ktlintMainSourceSetCheck SKIPPED :app:collectKtlintMainViolations SKIPPED :app:ktlintStandardDebugSourceSetCheck SKIPPED :app:collectKtlintStandardDebugViolations SKIPPED :app:ktlintStandardSourceSetCheck SKIPPED :app:collectKtlintStandardViolations SKIPPED :app:collectKtlintStandardDebugVariantViolations SKIPPED

However, it's not failing properly. E.g., when I run detekt manually via ./gradlew detekt, there are a lot of errors. The config is pretty standard with the failure config like this:

staticAnalysis {
    penalty {
        maxErrors = 0
        maxWarnings = 0
    }
tasomaniac commented 4 years ago

I just had a look at our integration tests for Detekt. We don't have it for recent 1.x versions. All I can think of is that if Detekt changed their XML output format and we are failing to parse errors.

tasomaniac commented 4 years ago

Just extended the test suite to include Detekt 1.3.1. Let's see what happens https://github.com/novoda/gradle-static-analysis-plugin/pull/222

Is this the version we use at Wayfair?

tasomaniac commented 4 years ago

Wait, now I had a closer look at your dry run output. It only includes ktlint tasks. Not detekt!

kenyee commented 4 years ago

yep, Wayfair. You're right..detekt isn't in the dry run list :-( The static-analysis.gradle file looks like it's configuring it:

    detekt {
        config = files(configFile('quality/detekt/detekt-config.yml'))
        baseline = configFile('quality/detekt/detekt-baseline.xml')
    }

the docs here claim it needs another profile lambda: https://github.com/novoda/gradle-static-analysis-plugin/blob/master/docs/advanced-usage.md but adding that causes gradle to fail with: Could not find method profile() for arguments [main, static_analysis_azg1haq43bq1kq7hni2bf37ok$_run_closure1$_closure7$_closure12@1946e804] on object of type io.gitlab.arturbosch.detekt.extensions.DetektExtension.

tasomaniac commented 4 years ago

We updated detekt docs but missed this one. Profile is very very old function in detekt Gradle plugin. You basically should configure it by following Detekt tutorials. The important thing is that it should be inside staticAnalysis { }

kenyee commented 4 years ago

it's definitely inside staticAnalysis {}

So next question is: how do I debug why the detekt task is not added?

tasomaniac commented 4 years ago

For some reason the opened PR didn't trigger the CI.

The best would be to try if it can be reproduced in the sample project in this repository (using the same Detekt version).

You can debug by adding static analysis plugin as a composite build. Sample uses that approach. By doing that Gradle will replace the dependency with the sources.

tasomaniac commented 4 years ago

Putting the configuration inside afterEvaluate fixed this issue. Especially if the tools like Detekt or Ktlint start to use afterEvaluate themselves, we may need to use it internally too.

Just putting afterEvaluate inside this plugin would fix it. But I would like to investigate the root cause of this and try to find a better solution if possible.

Leaving this open for now. It would be good to find a way to reproduce this.