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

Fix codegen compilation. #1213

Closed
wants to merge 1 commit into from
Closed

Fix codegen compilation. #1213

wants to merge 1 commit into from

Conversation

Tolriq
Copy link
Contributor

@Tolriq Tolriq commented Sep 4, 2020

  • Fix ShadowJar configuration to properly update source code to point to relocated package
  • Update Shadow plugin to 6.0.0

@@ -44,6 +44,7 @@ dependencies {
exclude(group = "org.jetbrains.kotlin", module = "kotlin-stdlib")
}
implementation(Dependencies.KotlinPoet.kotlinPoet)
kapt(Dependencies.KotlinPoet.kotlinPoet)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? The only processors on the processorpath are auto service and incap helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually seems it's not needed. As said in both issues this is not a part I usually use and master.
During first attempt adding this did embed kotlinpoet in the jar so I assumed it was necessary and part of the fix.

Just tested and just properly binding the shadowJar task at proper timing is enough. PR updated.

- Fix ShadowJar configuration to properly update source code to point to relocated package
- Update Shadow plugin to 6.0.0
@@ -109,3 +109,5 @@ afterEvaluate {
}
}
}

tasks.named("assemble") { dependsOn(tasks.named("shadowJar")) }
Copy link
Collaborator

@ZacSweers ZacSweers Sep 4, 2020

Choose a reason for hiding this comment

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

Why is this needed? shadowJar is already configured above this for publications

> Task :kotlin:codegen:jar
> Task :kotlin:codegen:shadowJar
> Task :kotlin:tests:kaptGenerateStubsTestKotlin
> Task :kotlin:tests:kaptTestKotlin

shadowed kotlinpoet/kotlinx-metadata classes are present too

image

The problem here appears to be that kotlinpoet itself is getting shaded

image

I tried with your changes and the same issue is still present, not sure if you tried this and saw something different?

image

The proper fix here is to figure out why this block is excluding kotlinpoet's core artifact from the pom and shading it. This PR just updates the shadow plugin dependency but doesn't fix anything. We probably missed this in the switch to gradle because the kotlin gradle plugin often just uses compiled class files from their local project's build dir directly rather than packaged jars (sometimes annoying quirk of how their incremental compilation works).

image

One quick fix is to add exclude(group = "com.squareup", module = "kotlinpoet") to the shaded kotlinpoet deps to ensure it doesn't shade the core artifact, but still leaves the runtime issue. This appears to be a long-standing issue in the shadow plugin: GradleUp/shadow#321. We can fix this with a withXml adjustment. I've opened #1214 to do these

Copy link
Contributor Author

@Tolriq Tolriq Sep 4, 2020

Choose a reason for hiding this comment

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

This is needed because it ran too late and the source code was not updated.
So the code still referenced the non shadowed one and failed.

Edit: I meant the non relocated one

Furthermore if you check https://oss.sonatype.org/content/repositories/snapshots/com/squareup/moshi/moshi-kotlin-codegen/1.11.0-SNAPSHOT/ you'll see that recent snapshots does not have the shadow nor the relocated one. (https://oss.sonatype.org/content/repositories/snapshots/com/squareup/moshi/moshi-kotlin-codegen/1.11.0-SNAPSHOT/moshi-kotlin-codegen-1.11.0-20200901.200622-3.jar)

@Tolriq Tolriq closed this Sep 5, 2020
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.

3 participants