Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add automatic detekt tasks for Android Plugins #2787

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

realdadfish
Copy link
Contributor

The Gradle Plugin was missing automatic task generation for Android modules, so I tried adding them.

While this works in general, I still get a couple of compilation issues when running the task on different modules, mainly things that stem from code generation:

/DownloadApiFactory.kt:7:24: error: unresolved reference: BuildConfig
import my.module.BuildConfig
                       ^
/DownloadApiFactory.kt:28:52: error: unresolved reference: BuildConfig
        .client(downloadOkHttpFactory.createClient(BuildConfig.DEBUG, readTimeOutSeconds))

@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #2787 into master will decrease coverage by 0.20%.
The diff coverage is 51.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2787      +/-   ##
============================================
- Coverage     79.76%   79.56%   -0.21%     
- Complexity     2507     2527      +20     
============================================
  Files           427      431       +4     
  Lines          7542     7639      +97     
  Branches       1421     1441      +20     
============================================
+ Hits           6016     6078      +62     
- Misses          771      795      +24     
- Partials        755      766      +11     
Impacted Files Coverage Δ Complexity Δ
...tlab/arturbosch/detekt/DetektCreateBaselineTask.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...lin/io/gitlab/arturbosch/detekt/KotlinExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
.../io/gitlab/arturbosch/detekt/internal/DetektJvm.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../gitlab/arturbosch/detekt/internal/FileMangling.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...o/gitlab/arturbosch/detekt/internal/SharedTasks.kt 52.17% <52.17%> (ø) 0.00 <0.00> (?)
...gitlab/arturbosch/detekt/internal/DetektAndroid.kt 70.96% <70.96%> (ø) 12.00 <12.00> (?)
...kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt 83.87% <83.33%> (+30.02%) 11.00 <6.00> (+1.00)
...ab/arturbosch/detekt/extensions/DetektExtension.kt 47.36% <100.00%> (+22.36%) 6.00 <3.00> (+4.00)
...h/detekt/api/internal/DisabledAutoCorrectConfig.kt 44.44% <0.00%> (-33.34%) 5.00% <0.00%> (-2.00%)
.../arturbosch/detekt/core/tooling/ParsingStrategy.kt 27.27% <0.00%> (-11.19%) 0.00% <0.00%> (ø%)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1712c0d...39f36fc. Read the comment docs.

@realdadfish
Copy link
Contributor Author

Hrm... I still seem to be doing something else wrong. When this task is run, my custom rule in my project fails to execute, because it cannot determine the absolute path of the incoming KtFile, i.e. ktFile.getUserData(Key<String>("absolutePath")) returns null. Any idea here?

@realdadfish
Copy link
Contributor Author

I eventually opened https://issuetracker.google.com/issues/158777988 for the BuildConfig issue above, in case I did not oversee anything in the existing AGP 4.0 API how to retrieve the generated source classpath into Detekt.

@arturbosch
Copy link
Member

Hrm... I still seem to be doing something else wrong. When this task is run, my custom rule in my project fails to execute, because it cannot determine the absolute path of the incoming KtFile, i.e. ktFile.getUserData(Key<String>("absolutePath")) returns null. Any idea here?

Please try to rebase on current master.
We have moved some code for 1.10.0 which running 1.9.1 with newer extensions won't handle.
Does this fix it?

@realdadfish
Copy link
Contributor Author

realdadfish commented Jun 15, 2020

Build is broken - the new detekt-bom artefact is apparently missing and not published. Working around that.

Could not parse POM /Users/thomaskeller/.m2/repository/io/gitlab/arturbosch/detekt/detekt-gradle-plugin/1.10.0-RC1-android-gradle-plugin/detekt-gradle-plugin-1.10.0-RC1-android-gradle-plugin.pom
            > Could not find io.gitlab.arturbosch.detekt:detekt-bom:1.10.0-RC1-android-gradle-plugin.

It's part of the locally-published POM:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <!-- This module was also published with a richer model, Gradle metadata,  -->
  <!-- which should be used instead. Do not delete the following line which  -->
  <!-- is to indicate to Gradle or any Gradle module metadata file consumer  -->
  <!-- that they should prefer consuming it instead. -->
  <!-- do_not_remove: published-with-gradle-metadata -->
  <modelVersion>4.0.0</modelVersion>
  <groupId>io.gitlab.arturbosch.detekt</groupId>
  <artifactId>detekt-gradle-plugin</artifactId>
  <version>1.10.0-RC1-android-gradle-plugin</version>
  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>io.gitlab.arturbosch.detekt</groupId>
        <artifactId>detekt-bom</artifactId>
        <version>1.10.0-RC1-android-gradle-plugin</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>
  ...

@realdadfish
Copy link
Contributor Author

Ok, I manually added the missing POM file for detekt-bom and got it working:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>
  <groupId>io.gitlab.arturbosch.detekt</groupId>
  <artifactId>detekt-bom</artifactId>
  <version>1.10.0-RC1</version>
  <name>detekt</name>
  <packaging>pom</packaging>
  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>com.beust</groupId>
        <artifactId>jcommander</artifactId>
        <version>1.78</version>
      </dependency>
      <dependency>
        <groupId>org.yaml</groupId>
        <artifactId>snakeyaml</artifactId>
        <version>1.26</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlinx</groupId>
        <artifactId>kotlinx-html-jvm</artifactId>
        <version>0.7.1</version>
      </dependency>
    </dependencies>
  </dependencyManagement>
</project>

@realdadfish
Copy link
Contributor Author

Please try to rebase on current master.
We have moved some code for 1.10.0 which running 1.9.1 with newer extensions won't handle.
Does this fix it?

This solved the issue with my custom rule not finding the KtFile's path. Analysis runs through in general, just that I still see a lot of compilation errors beforehand (that don't break the task). I don't know how exactly Detekt's type resolution works, but I guess issues that stem from synthetic imports won't be resolvable, because the compiler plugin generating code for those doesn't place that generated code anywhere visible.

> Task :ui:detektDebug
/BarcodeScannerDialogFragment.kt:24:24: error: unresolved reference: synthetic
import kotlinx.android.synthetic.main.dialog_fragment_barcode_scanner.camera_container

@arturbosch
Copy link
Member

Ok, I manually added the missing POM file for detekt-bom and got it working:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>
  <groupId>io.gitlab.arturbosch.detekt</groupId>
  <artifactId>detekt-bom</artifactId>
  <version>1.10.0-RC1</version>
  <name>detekt</name>
  <packaging>pom</packaging>
  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>com.beust</groupId>
        <artifactId>jcommander</artifactId>
        <version>1.78</version>
      </dependency>
      <dependency>
        <groupId>org.yaml</groupId>
        <artifactId>snakeyaml</artifactId>
        <version>1.26</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlinx</groupId>
        <artifactId>kotlinx-html-jvm</artifactId>
        <version>0.7.1</version>
      </dependency>
    </dependencies>
  </dependencyManagement>
</project>

The bom will be published with #2814

Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does android need the synthetic props/files while running detekt or for a sourceset?

Does it help if you explicitly add them to DetektExtension.input?

@realdadfish
Copy link
Contributor Author

Does android need the synthetic props/files while running detekt or for a sourceset?

Can't think of any, no. Just the source files should be enough IMHO.

@realdadfish
Copy link
Contributor Author

I'll come back to this in a few weeks, right now it's release crunching time here and I have no more time to spent on this PR. I'll keep it open until then and then rebase against the recent master.

@realdadfish
Copy link
Contributor Author

Would be happy if somebody could re-review this. Thanks!

Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me.
Could you elaborate why we technically need two Gradle plugins to support Android?

Shouldn't project.plugins.withId("kotlin-android") {...} to enough to not run into android specific code?

@realdadfish
Copy link
Contributor Author

The code looks good to me.
Could you elaborate why we technically need two Gradle plugins to support Android?

Shouldn't project.plugins.withId("kotlin-android") {...} to enough to not run into android specific code?

I'm not sure if I understand the question. The original reviewer's comment was that the Android-specific things should not become a burden for non-Android projects, so no Android-specific dependencies should be in the original plugin's code. I understood that and followed along. I figured however that there was no other easy and maintainable way to query the Android Gradle Plugin's API except for referencing it, the alternative would have be to write a lot of class loading magic.

The other thing is in many projects Android modules and pure JVM modules are intermixed. My aim was to have a single plugin (the new android plugin) to handle both, without code and configuration duplication. That's why I also created those aggregation tasks, detektMain and detektTest, to make it easy to run detekt on a codebase that contained either module type.

@realdadfish
Copy link
Contributor Author

@arturbosch I opened up a separate, experimental sub PR to figure out how feasible it is to generate a baseline file with type resolution enabled, but I stumbled upon a weird issue. Could you comment there? realdadfish#1

@realdadfish
Copy link
Contributor Author

Issue solved in a separate PR here. #2879

@realdadfish
Copy link
Contributor Author

So, the TR-enabled baseline creation is working nicely alongside checking. I will add a couple of more integration tests to check whether the appropriate tasks are actually created once I got some feedback on the implementation.

I kind of "hacked" the configuration of the baseline file and dynamically extend it's name with the sourceSet or variant name. I don't like this approach all too much; the alternative would have been to not configure a single baseline file, but to provide a directory where these files reside (configure module/baselines, plugin reads / writes module/baselines/{main,test,debug,release}.xml, while falling back to a common module/baselines/detekt.xml if the specific baseline file is not found).

Feedback is welcome!

@arturbosch
Copy link
Member

Issue solved in a separate PR here. #2879

Thanks for the fix!

It will take me some time to review this PR. I will come back to you in a few days.

@realdadfish
Copy link
Contributor Author

@arturbosch Have you had time to look at the PR yet? I'm working with a fork right now and would really like to get this "upstream-ready" :)

@arturbosch
Copy link
Member

arturbosch commented Aug 4, 2020

@arturbosch Have you had time to look at the PR yet? I'm working with a fork right now and would really like to get this "upstream-ready" :)

Sorry for the delay. Unfortunately no, not yet. I will take time for this PR this week. Thanks for the reminder and the patience.

@arturbosch
Copy link
Member

arturbosch commented Aug 9, 2020

@arturbosch Have you had time to look at the PR yet? I'm working with a fork right now and would really like to get this "upstream-ready" :)

Hey, sorry for the wait!
When trying to build locally I get:

com.android.builder.errors.EvalIssueException: SDK location not found. Define location with an ANDROID_SDK_ROOT environment variable or by setting the sdk.dir path in your project's local properties file at '/tmp/gradle17916022041907269827projectDir/local.properties'.

Any idea why this is happening? I can successfully build ktlint-gradle without needing the android sdk.

@arturbosch
Copy link
Member

The code looks good to me.
Could you elaborate why we technically need two Gradle plugins to support Android?
Shouldn't project.plugins.withId("kotlin-android") {...} to enough to not run into android specific code?

I'm not sure if I understand the question. The original reviewer's comment was that the Android-specific things should not become a burden for non-Android projects, so no Android-specific dependencies should be in the original plugin's code. I understood that and followed along. I figured however that there was no other easy and maintainable way to query the Android Gradle Plugin's API except for referencing it, the alternative would have be to write a lot of class loading magic.

I meant that if you protect code which uses Android specific methods/references with project.plugins.withId("kotlin-android") {...} it won't crash for non-android users.
I'm sorry that I don't follow now :). Which original comment do you mean?
If we can combine the task generation for plain jvm and android users, we should definitely do this in just one plugin.
Gradle plugin tests are expensive and cost us most of the time in our build.
Having a separation through different files/functions is good enough I would say. The ktlint-gradle plugin proves that this is possible.

@realdadfish
Copy link
Contributor Author

@arturbosch Have you had time to look at the PR yet? I'm working with a fork right now and would really like to get this "upstream-ready" :)

Hey, sorry for the wait!
When trying to build locally I get:

com.android.builder.errors.EvalIssueException: SDK location not found. Define location with an ANDROID_SDK_ROOT environment variable or by setting the sdk.dir path in your project's local properties file at '/tmp/gradle17916022041907269827projectDir/local.properties'.

Any idea why this is happening? I can successfully build ktlint-gradle without needing the android sdk.

Can you give me the full stacktrace? Then I can investigate why the Gradle plugin asks for the SDK at this stage. I don't know the ktlint plugin enough to answer your second question.

@realdadfish
Copy link
Contributor Author

realdadfish commented Aug 9, 2020

I'm not sure if I understand the question. The original reviewer's comment was that the Android-specific things should not become a burden for non-Android projects, so no Android-specific dependencies should be in the original plugin's code. I understood that and followed along. I figured however that there was no other easy and maintainable way to query the Android Gradle Plugin's API except for referencing it, the alternative would have be to write a lot of class loading magic.

I meant that if you protect code which uses Android specific methods/references with project.plugins.withId("kotlin-android") {...} it won't crash for non-android users.

Ok, had to re-read on class-loading, because my original assumption was that if I want to be truly "compatible" I'd have to use a lot of reflection in my code, but apparently popular JVMs have some loopholes and it might be enough to move the Android-specific code in separate classes, eventually even only into a separate method (cf. https://stackoverflow.com/a/34263422/305532). I try to come up with a better solution then.

I'm sorry that I don't follow now :). Which original comment do you mean?

It was given by schalkms on the Kotlin Slack at #detekt:

The detekt-gradle-plugin is the wrong place to do it. A detekt user with a web app doesn't want to depend on Android stuff.
Thanks for the contribution! I think this should be done in a separate module if the community wants to maintain this stuff.

If we can combine the task generation for plain jvm and android users, we should definitely do this in just one plugin.
Gradle plugin tests are expensive and cost us most of the time in our build.

Well, the amount of tests would stay the same if I'd merge both, but in any case one could speed up the tests in-module with parallelism (higher fork-count) eventually if this is an issue.

Having a separation through different files/functions is good enough I would say. The ktlint-gradle plugin proves that this is possible.

Again, don't know the innards of ktlint-gradle, but I try my best.

@realdadfish
Copy link
Contributor Author

@arturbosch I rebased to current master and merged the implementation back into the main plugin. Let me know what you think.

@realdadfish
Copy link
Contributor Author

If this is ready for prime-time, I should probably document the configuration options and meta tasks / potential pitfalls as well as the configuration file fallback somewhere. What would be a good place to do so?

@BraisGabin
Copy link
Member

If this is ready for prime-time, I should probably document the configuration options and meta tasks / potential pitfalls as well as the configuration file fallback somewhere. What would be a good place to do so?

I suppose that here is the best place: https://github.com/detekt/detekt/blob/master/docs/pages/gettingstarted/gradle.md if it gets big we can split it later.

@arturbosch
Copy link
Member

arturbosch commented Aug 23, 2020

The code looks good. @BraisGabin can you check how it behaves with one of your Android projects, please?
I'm not sure how to handle the warnings about missing generated files on the classpath as many of our users do not want to see any output on the console / or disturbing warnings :/
These warnings appear when using detekt[SourceSet/Variant] only?

@realdadfish
Copy link
Contributor Author

@arturbosch These warnings appear only for detekt<Variant>, yes, they won't appear on detekt<SourceSet> on JVM modules.

Things to note: detektMain and detektTest, as well as detektBaselineMain and detektBaselineTest tasks are proxy tasks that can be called to check and eventually create baseline files for all main and all test variants of a module.

Baseline files are recognized by source set or variant name, while the latter takes precedence. That means if both, a <baseline-name>-main.xml and a <baseline>.xml exist in a JVM module, the first is used for main source sets and the latter for test source sets. Likewise, if a <baseline-name>-<variant>.xml exists, it takes precendence for the detekt<Variant> and detektBaseline<Variant> tasks before <baseline-name>.xml (if that exists).

@realdadfish realdadfish changed the title WIP: Add automatic detekt tasks for Android Plugins Add automatic detekt tasks for Android Plugins Aug 24, 2020
@realdadfish
Copy link
Contributor Author

@BraisGabin
Copy link
Member

TL;DR: Ready to merge.

I did a quick fix to make it compile. But I'm not sure if I could break something (I know nearly nothing about gradle scriptting)

Things that I found:

  • A lot of warnings like:
Ignoring a file detekt cannot handle: /Users/brais.gabin/projects/XXXXX/file.java
  • If I run ./gradlew detekt it doesn't check the flavor/buildType source sets nor the tests. (I didn't check androidTest but I suppose that it will be the same)
  • I have 2 flavors so detekt finds nearly all the issues twice because nearly all the code is in main. This have sense... but if we find an way to remove duplicates would be great.

Neigther of those seem like blockers. I think that this is a huge improvement.

We can mark the support to Android as "experimental" so we can break things when we get some feedback from the community.

@realdadfish
Copy link
Contributor Author

If I run ./gradlew detekt it doesn't check the flavor/buildType source sets nor the tests. (I didn't check androidTest but I suppose that it will be the same)

Yes, the detekt task wasn't and isn't used at all for Android - I could only add this as another "proxy" to run detekt<AllSourceSetsOrVariants> in the background, but frankly, I didn't want to confuse user's expectations what this task then would do (TR-enabled checking) than what the regular detekt task would do on a non-Android project (non-TR-enabled checking).

I have 2 flavors so detekt finds nearly all the issues twice because nearly all the code is in main. This have sense... but if we find an way to remove duplicates would be great.

This is a hard problem, because each analysis sees basically "two different versions" of the same module. What I could think about in general however is some kind of "merge" task, similar to what Jacoco also provides for it's binary format. We'd however have to separate analysis from report generation task-wise, and I don't know how feasible this is to do right now, given the fact that the development goes towards a compiler plugin.

@BraisGabin
Copy link
Member

I have a lot of false positives in the rule IgnoredReturnValue... All of them related with RxJava3 (that have sense, because RxJava uses the @CheckReturnValue annotation). I don't know if this is an issue with the rule itself or the plugin. I feel strange that no one opened an issue about this if the problem is the rule.

Then I have false positives in UnnecessarySafeCall related with the missing classes. For example:

binding.address2?.visibility = if (state.address == null) GONE else VISIBLE

The problem is that binding is a generated class so detekt can't know if address2 is or not nullable.

I really don't know if these issues are in the plugin, my setup or the rules.

@realdadfish
Copy link
Contributor Author

@BraisGabin I've recently upgraded to Rx3 (3.0.6) as well and got no additional warnings from Detekt wrt IgnoredReturnValue. Interesting...

Regarding databinding (this is databinding, and not synthetic accessors, right?), we don't use safe-calls on those, so we don't see those warnings there either.

@BraisGabin
Copy link
Member

@BraisGabin I've recently upgraded to Rx3 (3.0.6) as well and got no additional warnings from Detekt wrt IgnoredReturnValue. Interesting...

Yes... probably a problem in my setup? I have issues in code like:

Observable.just("hi")
  .observeOn(main) // This line
  .subscribe()
  .addTo(compositeDisposable)

or

return if (something) {
  Observable.just("hi") // this line
} else {
  Observable.just("hello") // and this line too
}

Regarding databinding (this is databinding, and not synthetic accessors, right?), we don't use safe-calls on those, so we don't see those warnings there either.

It is ViewBinding. With a layout for mobile/tablet with some missing views in the mobile.

But I'm wondering how detekt should behave about this errors... Manage the case that one class is not correctly solved in the rule doesn't seem rule-writter friendly... So maybe we should just don't care about it. This will be fixed with the compile plugin so add that complexity to the rules doesn't seem a good idea.

@arturbosch arturbosch added this to the 1.12.0 milestone Aug 25, 2020
@arturbosch
Copy link
Member

Cool :) lets merge this then, looks good @realdadfish thanks for the contribution!

@BraisGabin
Copy link
Member

🎉🎉🎉🎉🎉 Great contribution @realdadfish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants