search

drewhamilton / Poko

A Kotlin compiler plugin that generates equals, hashCode, and toString for plain old Kotlin objects in public APIs.
Apache License 2.0
318 stars 10 forks source link

Don't include the transitive annotation dependency if it's not needed #328

Open evant opened 6 months ago

evant commented 6 months ago

This came out of the conversation started on https://github.com/saket/telephoto/pull/79, on the jvm/android at least the annotations only need to be present at compile time, so it would be nice not to pull it in at runtime. I know there were previous issues with setting it to compileOnly before https://github.com/drewhamilton/Poko/issues/163.

One thought: have you considered having the consumer declare the dependency instead? I don't think it's too much to ask to do:

plugins {
    id("dev.drewhamilton.poko")
}

dependencies {
    compileOnly("dev.drewhamilton.poko:poko-annotations")
}

and it could be adjusted to whatever configuration is needed.

JakeWharton commented 6 months ago

The (even further) upstream issue is https://youtrack.jetbrains.com/issue/KT-64109. Once this is fixed, we can compileOnly every target automatically. It doesn't seem too far away, so I'd be hesitant to make a change now. Especially since compileOnly only works for a subset of projects, so we'd have to detail how any Kotlin/Native targets require that you use implementation instead. Plus, as you noted in Telephoto, you can also work around it today by including your own internal annotation class marker which omits the dep.

evant commented 6 months ago

Hm, based on https://youtrack.jetbrains.com/issue/KT-64109#solution it seems like they only thing that is coming soon is improved messaging, not actually supporting compileOnly on native targets though?

JakeWharton commented 6 months ago

Ah, interesting. I didn't read the full comment history since they decided to dupe my (older 😬) bug onto it: https://youtrack.jetbrains.com/issue/KT-43500. I only was notified of the most recent comment and target version updates. That's... disappointing.

evant commented 6 months ago

Another issue with switching to compileOnly in the plugin. It won't be available to test sources, this may be what you want or it may not be what you want.

JakeWharton commented 6 months ago

We can add it to the associated compile-only configuration for tests, too!

drewhamilton commented 6 months ago

Is there anything wrong with Poko automatically using compileOnly for non-native targets and implementation for native targets? It looks maybe possible by iterating through the consuming project's KotlinTargetsContainer.

JakeWharton commented 6 months ago

I believe I tried that and KGP won't let you. You can't make the common metadata target have different behavior across platform targets.

drewhamilton commented 5 months ago

Yeah, I get a circular task dependency if I try to do add it as compileOnly to commonMain and as implementation to nativeMain.

Circular dependency between the following tasks:
:mpp:allMetadataJar
+--- :mpp:compileCommonMainKotlinMetadata
|    +--- :mpp:allMetadataJar (*)
|    \--- :mpp:transformCommonMainDependenciesMetadata
|         +--- :mpp:allMetadataJar (*)
|         +--- :mpp:macosArm64MetadataJar
|         |    +--- :mpp:compileAppleMainKotlinMetadata
|         |    |    +--- :mpp:allMetadataJar (*)
|         |    |    +--- :mpp:compileCommonMainKotlinMetadata (*)
|         |    |    +--- :mpp:compileNativeMainKotlinMetadata
|         |    |    |    +--- :mpp:allMetadataJar (*)
|         |    |    |    +--- :mpp:compileCommonMainKotlinMetadata (*)
|         |    |    |    +--- :mpp:metadataCommonMainClasses
|         |    |    |    |    \--- :mpp:compileCommonMainKotlinMetadata (*)
|         |    |    |    \--- :mpp:transformNativeMainDependenciesMetadata
|         |    |    |         +--- :mpp:allMetadataJar (*)
|         |    |    |         +--- :mpp:macosArm64MetadataJar (*)
|         |    |    |         +--- :mpp:macosX64MetadataJar
|         |    |    |         |    +--- :mpp:compileAppleMainKotlinMetadata (*)
|         |    |    |         |    +--- :mpp:compileMacosMainKotlinMetadata
|         |    |    |         |    |    +--- :mpp:allMetadataJar (*)
|         |    |    |         |    |    +--- :mpp:compileAppleMainKotlinMetadata (*)
|         |    |    |         |    |    +--- :mpp:compileCommonMainKotlinMetadata (*)
|         |    |    |         |    |    +--- :mpp:compileNativeMainKotlinMetadata (*)
|         |    |    |         |    |    +--- :mpp:metadataAppleMainClasses
|         |    |    |         |    |    |    \--- :mpp:compileAppleMainKotlinMetadata (*)
|         |    |    |         |    |    +--- :mpp:metadataCommonMainClasses (*)
|         |    |    |         |    |    +--- :mpp:metadataNativeMainClasses
|         |    |    |         |    |    |    \--- :mpp:compileNativeMainKotlinMetadata (*)
|         |    |    |         |    |    \--- :mpp:transformMacosMainDependenciesMetadata
|         |    |    |         |    |         +--- :mpp:allMetadataJar (*)
|         |    |    |         |    |         +--- :mpp:macosArm64MetadataJar (*)
|         |    |    |         |    |         +--- :mpp:macosX64MetadataJar (*)
|         |    |    |         |    |         +--- :mpp:transformAppleMainDependenciesMetadata
|         |    |    |         |    |         |    +--- :mpp:allMetadataJar (*)
|         |    |    |         |    |         |    +--- :mpp:macosArm64MetadataJar (*)
|         |    |    |         |    |         |    +--- :mpp:macosX64MetadataJar (*)
|         |    |    |         |    |         |    +--- :mpp:transformCommonMainDependenciesMetadata (*)
|         |    |    |         |    |         |    \--- :mpp:transformNativeMainDependenciesMetadata (*)
|         |    |    |         |    |         +--- :mpp:transformCommonMainDependenciesMetadata (*)
|         |    |    |         |    |         \--- :mpp:transformNativeMainDependenciesMetadata (*)
|         |    |    |         |    +--- :mpp:metadataAppleMainClasses (*)
|         |    |    |         |    \--- :mpp:metadataMacosMainClasses
|         |    |    |         |         \--- :mpp:compileMacosMainKotlinMetadata (*)
|         |    |    |         \--- :mpp:transformCommonMainDependenciesMetadata (*)
|         |    |    +--- :mpp:metadataCommonMainClasses (*)
|         |    |    +--- :mpp:metadataNativeMainClasses (*)
|         |    |    \--- :mpp:transformAppleMainDependenciesMetadata (*)
|         |    +--- :mpp:compileMacosMainKotlinMetadata (*)
|         |    +--- :mpp:metadataAppleMainClasses (*)
|         |    \--- :mpp:metadataMacosMainClasses (*)
|         \--- :mpp:macosX64MetadataJar (*)
+--- :mpp:compileLinuxMainKotlinMetadata
|    +--- :mpp:allMetadataJar (*)
|    +--- :mpp:compileCommonMainKotlinMetadata (*)
|    +--- :mpp:compileNativeMainKotlinMetadata (*)
|    +--- :mpp:metadataCommonMainClasses (*)
|    +--- :mpp:metadataNativeMainClasses (*)
|    \--- :mpp:transformLinuxMainDependenciesMetadata
|         +--- :mpp:allMetadataJar (*)
|         +--- :mpp:transformCommonMainDependenciesMetadata (*)
|         \--- :mpp:transformNativeMainDependenciesMetadata (*)
+--- :mpp:compileNativeMainKotlinMetadata (*)
+--- :mpp:metadataCommonMainClasses (*)
+--- :mpp:metadataLinuxMainClasses
|    \--- :mpp:compileLinuxMainKotlinMetadata (*)
\--- :mpp:metadataNativeMainClasses (*)

(*) - details omitted (listed previously)

I'm inclined to agree with @evant's suggestion to require all consumers to declare the annotation dep (or their own), because neither default feels intuitive for all cases.

drewhamilton commented 4 months ago

Part of https://youtrack.jetbrains.com/issue/KT-64109#update-the-warning-message now says:

  • Update the message to state: ...
    • To resolve this explicitly add the compileOnly dependency as an 'api' dependency in K/N sourcesets (JVM sourcesets may continue to use compileOnly)

Which sounds similar to what I suggested above. So maybe the 2.0.20-Beta1 target version will support this?