Implement base Gradle StaticAnalysis classes for Gradle Plugin

Mauin commented 6 years ago

This is still WIP but publishing for feedback.

As described in Issues #580 and #540 this PR makes the Detekt Gradle Plugin implement the base StaticAnalysis classes SourceTask and AbstractCodeQualityPlugin

This work will result in breaking API changes of the Detekt Gradle Plugins API as the Plugin will now behave more like the PMD, Checkstyle, Findbugs and similar static analysis plugins.

This work will also result in detekt running as part of ./gradlew check and therefore ./gradlew build if the plugin is added to the project.

I had to make minor changes to account for the new way to have inputs based on source sets and not explicitly giving an input path in the API. With these changes the Gradle Plugin will automatically generate tasks for running detekt for each sourceSet: detektMain, detektTest, etc. Those sourceSets will be the inputs for the detekt CLI.

Due to the change how output reports are defined in the same manner as other tools like PMD and Checkstyle do in their Gradle API there are some more changes needed to detekt-cli and detekt-core to account for enabling disabling certain reports and changing their output paths with the CLI arguments.

The changes to the API are visible in the top-level build.gradle file in this PR. Essentially I believe this will make the concept of profiles go away as it is really easy to define a second detekt task with some special parameters (see the detektFailfast task in the build.gradle file).

Missing:

[x] Update documentation when implementation is finished
[x] ~~Decide on profile handling.~~ Removed
[x] Implement detektCreateBaseline and detektGenerateConfig tasks.
[x] Implement idea tasks.
[x] Create detektCheck task which delegates to the main detekt task to provide backwards compatibility

arturbosch commented 6 years ago

Looking forward to review this later! :) :+1:

Mauin commented 6 years ago

As for the reports. Currently we configure them via the config.yml. However the checkstyle reports in the Gradle Plugin would allow us to independently enable/disable and change the path for the different XML/HTML reports. To fully comply with this we need to find some way in -cli to set the path for the XML/HTML report directly. We can also leave the old output parameters in the CLI. However then CLI and Gradle Plugin would diverge a bit in how they are used. I'm open for suggestions and improvements.

Mauin commented 6 years ago

Just realized that the default CodeQualityExtension has a toolsVersion field. That's also the way Checkstyle and friends set the version through the Gradle Plugin. So I also removed our custom version field to instead use toolVersion.

arturbosch commented 6 years ago

I agree that we should be compliant with the reports feature. How about we enable custom reports only through the configuration file? Like:

output-reports:
  active: true
  MyReport:
    active: true
    path: reports/myreport.txt

We can do this after this PR then. What do you think?

Mauin commented 6 years ago

@arturbosch So effectively leaving the way I currently implemented it in this PR with the --report-xml and --report-html arguments and removing the handling of Reports via the config? I'll get right on that.

arturbosch commented 6 years ago

Yes, I will be able to test everything on the WE when I have internet again.

Mauin commented 6 years ago

Okay, feature-wise I think I have everything covered. Before updating all the documentation I'd love some feedback on how the Gradle Plugin API will look like after these changes.

See: https://github.com/arturbosch/detekt/pull/771/files#diff-c197962302397baf3a4cc36463dce5eaR41

Mauin commented 6 years ago

Just pinging @tasomaniac and @rock3r on this as well to get some feedback on the new behavior and DSL.

rock3r commented 6 years ago

CC @tobiasheine for planning evolution of the Novoda Static Analysis plugin

vanniktech commented 6 years ago

I'd like to review this. Would it be possible to have one last review round once this is no longer WIP?

Mauin commented 6 years ago

I'll go over the comments in the coming days and update everything. Thanks for all the feedback!

mkobit commented 6 years ago

FYI, if you wanted to add some tests (only briefly glanced over this) is to use the Gradle Test Kit which is fairly easy to use with the Gradle Plugin Development Plugin

Mauin commented 6 years ago

Updated the PR with all the feedback and using the Gradle Property API.

Mauin commented 6 years ago

Currently seeing 2 issues when running the detektMain task:

detekt-api cannot find the Yaml class. (NoClassDefFoundError)
detekt-gradle-plugin throws NoSuchMethodError on constructor.visibilityModifierType() in EmptyDefaultConstructor rule.

Might be my local config. I'm still looking but if anyone has seen these issues before, let me know 🙃

arturbosch commented 6 years ago

heyho, if you rebase everything, I can take a look at the two remaining issues too :). Lets do a freeze until this is done (except for the analysis projects ticket I created ;))

Mauin commented 6 years ago

@arturbosch I'll try to quickly rebase the branch tomorrow and fix the conflicts. As for the issues themselves I still haven't gotten closer to identifying the issue yet.

markov commented 6 years ago

@Mauin Thanks for doing this.

As for the NoSuchMethodError - I think it is something that changed in Kotlin between v1.2.21 and v1.2.31.

I am still using detekt RC5-6 and if I force everything in my project to use the latest kotlin by adding this to my root build.gradle file:

configurations.all {
    resolutionStrategy {
        eachDependency { details ->
            switch(details.requested.group) {
                case 'org.jetbrains.kotlin':
                    details.useVersion '1.2.31'
                    break
            }
        }
    }
}

Then I see the same error.

rock3r commented 6 years ago

Hey there, is there any updates on this? I understand it's been a very busy couple of months for all of us but I'd like to see this moved forward so we can properly support Detekt again in our static analysis plugin, which is currently broken. Let me know if we can assist in any way!

Mauin commented 6 years ago

I'm still chipping away at this but have run into multiple issues on the Gradle level recently. As we switched to using the Kotlin DSL in detekt I noticed multiple issues with the Plugin rework that I had to address and also found blockers in Testing the plugin with Gradle v4.7 and am waiting for a Gradle v4.8 release to continue the work in testing the Plugin properly. I'm looking at pushing an updated version of the re-work soon(TM) but the iteration process on the Plugin is suuuper slow unfortunately...

rock3r commented 6 years ago

I totally feel your pain @Mauin — let me know if there's any way I can get Novoda people working on this too, I think we maaay be able to squeeze part of someone's time into things like these. Maybe. No promises but I really want to get this sorted one way or the other 💃

3flex commented 6 years ago

@Mauin you mentioned some work that's partly done but not pushed yet - is that in a different branch? I'd like to help with this, particularly on the Gradle side (I started down this path some time back but got held up when I started looking at the changes required on the detekt side to make it work properly. But a lot of what I did could be migrated here since it looks like you've made the necessary changes in detekt which is fantastic!). I found it a real pain to work with initially, Gradle is a beast.

Good news on Gradle's side is there's a 4.8-rc-1 out now with Gradle TestKit support for projects using Kotlin DSL. You mentioned problems with testing, perhaps this will help?

Let me know how I can assist. It would be great to get this one over the line!

Mauin commented 6 years ago

Pushed an update to the branch with now a Kotlin DSL compliant DSL including some Tests for it. Groovy Gradle DSL Tests are still missing. This also uses the Gradle v4.8-rc-1, which makes the Kotlin DSL testing possible, but we're still suffering from the classpath issue described in #913.

3flex commented 6 years ago

This is great! I hope I didn't miss something major related to the classpath issues you told me about, because it seems I have it working with some very minor changes in build.gradle.kts (as long as we ignore the Windows path separator issue for now!):

Remove this line: classpath("io.gitlab.arturbosch.detekt:detekt-gradle-plugin:$usedDetektGradleVersion")
Add this line to plugins{} block: id("io.gitlab.arturbosch.detekt") version "1.0.0.RC7-MH" (change the version as appropriate)

This is based on this documentation from the Kotlin DSL project, which says:

When using the Gradle Kotlin DSL it is heavily recommended to apply Gradle plugins declaratively using the plugins {} block. This will enable type-safe extension accessors you will use to configure plugins.

Also, because I'd pushed to mavenLocal, I had to add this in settings.gradle.kts, because the Gradle Plugin repo is the only one used by default for items in the plugins {} block:

pluginManagement {
    repositories {
        gradlePluginPortal()
        mavenLocal()
    }
}

See https://github.com/3flex/detekt/tree/mr/gradle_classes for working example.

arturbosch commented 6 years ago

Please make sure to rebase this on top of current master as we may have two commits here which I dropped on master (gradle 4.7).

Mauin commented 6 years ago

I merged that latest changes from Master. Now running gradle detektMain passes successfully! Thanks for all the work @3flex. Now I need to finish looking at the tests. But they seem to fail for valid reasons now. Looks like we need to generate more temporary files in the test build.

3flex commented 6 years ago

@Mauin thoughts on how to get tests working in CI? gradle-plugin resolves the dependency at runtime using a module dependency, so the cli artifact has to be published to a repository.

I think two options make sense:

Publish SNAPSHOT builds of detekt, and have gradle-plugin consume them during testing (already requested, see https://github.com/arturbosch/detekt/issues/512)
Add a publishToMavenLocal step on detekt and add a mavenLocal repository when building the Gradle plugin

Thoughts? 2 can be done very cleanly in CI using a Gradle init script. If you're good with that I'll submit a PR to master; this would be useful in CI anyway so we'd know the plugin and latest detekt work well together.

sschuberth commented 6 years ago

Wouldn't it be possible on CI to conditionally use dependency substitution to replace the module dependency with a project dependency?

3flex commented 6 years ago

Maybe... this project now uses a Gradle composite build, so to detekt-gradle-plugin, detekt-cli is not part of the project so detekt-cli can't be treated as a project dependency from gradle-plugin. Does dependency substitution work if substituting with a file instead of a project dependency? Was unclear from the docs. I'll test locally to confirm since that should be the cleanest solution.

This might work too.

Mauin commented 5 years ago

Closing this as it is succeeded by #1017

detekt / detekt

Implement base Gradle StaticAnalysis classes for Gradle Plugin #771