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

Refactor detekt-gradle-plugin off com.android.tools.build:gradle #3327

Closed
chao2zhang opened this issue Dec 27, 2020 · 15 comments · Fixed by #7041
Closed

Refactor detekt-gradle-plugin off com.android.tools.build:gradle #3327

chao2zhang opened this issue Dec 27, 2020 · 15 comments · Fixed by #7041
Labels
discussion gradle-plugin never gets stale Mark Issues/PRs that should not be closed as they won't get stale

Comments

@chao2zhang
Copy link
Member

Android Gradle plugin enters a new phase of aligning versions with Gradle Official announcement, this does have an impact on plugin authors. We should not depend on com.android.tools.build:gradle but com.android.tools.build:gradle-api. As Google plans to deprecate com.android.tools.build:gradle in 7.0 and remove it completely in 8.0 (This is according to the conversation between my colleague and the technical lead of Android Developer Tools, so there could be falsehood).

What I am proposing to do for detekt on Android projects:

  • Experiment with gradle-api:4.1.1. As I was told that gradle-api:4.1/4.2 is evolving and but gradle-api:4.1 should work for the basic use case.
  • Create one Detekt task per SourceSet, instead of per variant + test variant. This should not impact the behavior in terms of the generated gradle tasks but should significantly reduce the code complexity in our project.

I am open to any feedback or correction in the information I received.

@BraisGabin
Copy link
Member

  • Experiment with gradle-api:4.1.1. As I was told that gradle-api:4.1/4.2 is evolving and but gradle-api:4.1 should work for the basic use case.

I couldn't find anything about this gradle-api dependency. But for sure that we can test it if that allow us to be ready to any future change. But we should wait until we get some official documentation. Then we can start merging our "research".

  • Create one Detekt task per SourceSet, instead of per variant + test variant. This should not impact the behavior in terms of the generated gradle tasks but should significantly reduce the code complexity in our project.

I agree with you that have two tasks own per production code and other for test code is not handy. And this is not a problem just with out android compatibility. We have the same problem with pure kotlin projects too.

I think that we should rethink all our gradle tasks. My current idea is to have 4 principal tasks for 2.0:

  • detektCheck - runs detekt with type solving. It never reformat code. It will be like running detektMain and detektTest but with a single report. For android modules it will run detekt for each build variant. For android we should create a task for each build variant too (detektCheckDebug, detektCheckFlavourDebug...)
  • detektFix - runs the rules that are correctable. It never fails nor creates any report. It just fixes all the issues that it can. This could even help with Detekt crash on autoCorrect property #3250
  • detektBaseline - generates the baseline
  • detektConfiguration - generates the configuration

I think that gradle should not support checks without type solving for 2.0.

@chao2zhang
Copy link
Member Author

I couldn't find anything about this gradle-api dependency.

A lot of the interfaces in gradle has been marked as deprecated. Such as

package com.android.build.gradle.api

@Deprecated("Use  com.android.build.api.dsl.AndroidSourceDirectorySet")
interface AndroidSourceDirectorySet

My current idea is to have 4 principal tasks for 2.0
+1, this task list is much clearer

@chao2zhang
Copy link
Member Author

This would also resolve issues such as #3476

@3flex
Copy link
Member

3flex commented Mar 6, 2021

FWIW the experimental compiler plugin works well and avoids this issue (and related AGP/plugin issues) entirely because it hooks directly into the Kotlin compilation tasks after they've been configured by AGP.

I think effort would be better spent getting that production ready. I believe the only major feature missing is generation of baseline files.

@3flex
Copy link
Member

3flex commented May 20, 2021

More details on migration to Android's Gradle API artefact: https://developer.android.com/studio/releases/gradle-plugin-roadmap

@chao2zhang
Copy link
Member Author

chao2zhang commented May 20, 2021

More information from Google I/O: https://www.youtube.com/watch?v=AZBW5StgF8o

@BraisGabin
Copy link
Member

🎉 https://developer.android.com/studio/releases/gradle-plugin#7-0-0 🎉 We can start working on it.

@BraisGabin
Copy link
Member

BraisGabin commented Jul 29, 2021

And we probably should follow this path that lint is taking:

Running lint on default variant only

Running ./gradlew :app:lint now runs lint for only the default variant. In previous versions of AGP, it would run lint for all variants.

From: https://developer.android.com/studio/releases/gradle-plugin#running_lint_on_default_variant_only

@github-actions
Copy link

github-actions bot commented Nov 7, 2021

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Nov 7, 2021
@BraisGabin
Copy link
Member

We can't do this yet because of https://issuetracker.google.com/issues/158777988. More about this here: #4133 (comment)

At Droidcon London @cortinico and I talked with Wojtek Kaliciński (Android Developer Advocate @ Google) about this issue. I just remember that he told us to send him a link to see if he could help us. Here the tweet: https://twitter.com/braisgabin/status/1457310902773075973

@BraisGabin BraisGabin added never gets stale Mark Issues/PRs that should not be closed as they won't get stale and removed stale labels Nov 7, 2021
@3flex
Copy link
Member

3flex commented Jan 10, 2023

I've looked into the feasibility of doing this now. The Variant API must allow access to Kotlin sources, compile classpath and the compiled classes (both .class and JARs) for detekt to work correctly.

It seems the earliest AGP version we could migrate to is 7.3. We can get Kotlin source here, compile classpath here, and the compiled classes from here and here.

So, we could migrate, support AGP 7.3 and up (which might be a bit aggressive, but let's go with it for now) and we're done, right?

Wrong!

ALL_CLASSES_DIRS and ALL_CLASSES_JARS are deprecated in 7.4 and removed in 8.0.0-alpha09. There's a new API available from 7.4 which can be used instead and should continue working through Gradle 8 (when Variant APIs are supposed to be stabilised) and the foreseeable future.

When AGP 9 is released we will have to switch to the 8.0 API anyway, as AGP 9 drops support for the API that the plugin currently uses.

TL;DR: we can either:

  1. Do nothing until AGP 9 is released, then switch to the stable AGP 8 API. I expect we will then be compatible with AGP 7.4+.
  2. Same as above, but transition when AGP 8 is released, instead of waiting for 9.
  3. Migrate to Variant API and gradle-api now. We will only be able to support 7.3-7.4, then when 8 is released, we'll have to drop support for 7.3 and support 7.4+ only.

It would be nice to move to the new API, but the current plugin offers better backwards & forwards compatibility. Interested in what others think.

@BraisGabin
Copy link
Member

I would vote for option 2. This change is not just to make our code "easier". This should also fix issues, right? Then I think it's acceptable to drop support for earlier versions.

Other option is to drop support for 7.3 now and don't wait for AGP8. If someone can't update just yet they can keep detekt at 1.22.0.

@3flex
Copy link
Member

3flex commented Jan 13, 2023

Agree with option 2.

7.4 just released so we will likely get AGP 8 release in a few months, so if AGP 8 API is confirmed compatible with 7.4+, can make the move then and drop support for 7.3 and earlier.

@3flex
Copy link
Member

3flex commented Jan 16, 2023

Based on experiences with #5693, when we migrate to Variant API and start compiling against AGP 7.4, we'll have to compile the detekt Gradle plugin with JDK 11+, so end users will then have to run the Gradle plugin on JVM 11+, even if they're not using AGP. They'll still be able to work on projects that compile with earlier JDKs though using toolchains and other features offered by Gradle.

The only way I can think of to avoid this would be to separate the Gradle plugins so we have a detekt Android plugin for Gradle (requires JVM 11+), and another for non-Android (requires JVM 8+) but I don't think we should go down this path.

@cortinico
Copy link
Member

It would be nice to move to the new API, but the current plugin offers better backwards & forwards compatibility. Interested in what others think.

Sorry for being late to the convo.
I would vote to wait for AGP 9. I've been using the Variants API in another plugin of mine, and it's full of moving parts. A lot of significant APIs are marked as incubating and keep on changing between minor releases. I think this would add unnecessary workload for us.

The only way I can think of to avoid this would be to separate the Gradle plugins so we have a detekt Android plugin for Gradle (requires JVM 11+), and another for non-Android (requires JVM 8+) but I don't think we should go down this path.

Actually this doesn't sound that bad to be. Is having 2 distinct Gralde plugins too much of pain because of the interactions between the two or because of how complicated the build would get? I was thinking that instead it would help us to modularize a bit and also run more Gradle tests in parallel which now are the major offender in the build time.

@3flex 3flex self-assigned this Jun 13, 2023
@3flex 3flex removed their assignment Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion gradle-plugin never gets stale Mark Issues/PRs that should not be closed as they won't get stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants