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

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

Open
evant opened this issue Apr 4, 2024 · 9 comments
Open

Comments

@evant
Copy link

evant commented Apr 4, 2024

This came out of the conversation started on saket/telephoto#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 #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
Copy link
Collaborator

JakeWharton commented Apr 4, 2024

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
Copy link
Author

evant commented Apr 4, 2024

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
Copy link
Collaborator

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
Copy link
Author

evant commented Apr 4, 2024

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
Copy link
Collaborator

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

@drewhamilton
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Owner

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?

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

No branches or pull requests

3 participants