Add Wire Binary Compatibility plugin #3306
Conversation
Co-authored-by: Jesse Wilson <[email protected]> Co-authored-by: Hannah Greer <[email protected]>
settings.gradle.kts
Outdated
| @@ -60,7 +60,7 @@ include(":wire-schema-tests") | |||
| include(":wire-swift-generator") | |||
| include(":wire-test-utils") | |||
| include(":wire-tests") | |||
| if (startParameter.projectProperties.get("swift") != "false") { | |||
| if (false && startParameter.projectProperties.get("swift") != "false") { | |||
There was a problem hiding this comment.
Oops indeed! Let me remove that
There was a problem hiding this comment.
Do we need this file and the next?
There was a problem hiding this comment.
Yes. They hook into the Kotlin compiler to register this plugin
| import org.jetbrains.kotlin.cli.common.messages.MessageCollector | ||
| import org.jetbrains.kotlin.ir.IrStatement | ||
| import org.jetbrains.kotlin.ir.backend.js.utils.valueArguments | ||
| import org.jetbrains.kotlin.ir.builders.* |
There was a problem hiding this comment.
supernit: our style wants full imports here.
There was a problem hiding this comment.
Yes. They hook into the Kotlin compiler to register this plugin
...com/squareup/wire/binarycompatibility/kotlin/WireBinaryCompatibilityIrGenerationExtension.kt
Show resolved
Hide resolved
| override fun getPluginArtifact(): SubpluginArtifact = SubpluginArtifact( | ||
| groupId = "com.squareup.wire.binarycompatibility", | ||
| artifactId = "wire-binary-compatibility-kotlin-plugin", | ||
| version = "2.6.0-SNAPSHOT", |
There was a problem hiding this comment.
hard-coding the version here seems wrong?
There was a problem hiding this comment.
I've moved these maven coordinates to a simple BuildConfig object, and fixed the version (another copy-paste miss) -- this is the only place these values are referenced, the intent is to just keep this lightweight!
| dependencies { | ||
| implementation(kotlin("gradle-plugin-api")) | ||
| implementation(project(":wire-binary-compatibility-kotlin-plugin")) | ||
| implementation(libs.kotlin.gradlePlugin) |
There was a problem hiding this comment.
I suspect this dependency on the kotlin gradle plugin should be compileOnly to avoid polluting downstream consumers classpath. @autonomousapps do you know? Perhaps the gradle-plugin-api dependency as well?
There was a problem hiding this comment.
Updated, thank you!
There was a problem hiding this comment.
I think you got the change slightly backwards - the libs.kotlin.gradlePlugin should be compileOnly and the project("...") one should be `implementation
There was a problem hiding this comment.
with that change made I have no other concerns, thanks for doing this!
There was a problem hiding this comment.
Yes, I agree with what Kats has said. While this plugin only makes sense in the context of a Kotlin project, we don't want to force a Kotlin version update on anyone if we can avoid it, so we should use compileOnly(libs.kotlin.gradlePlugin).
There was a problem hiding this comment.
Filename is copypasta, should probably be updated
| @@ -0,0 +1,42 @@ | |||
| /* | |||
There was a problem hiding this comment.
One more name fixup needed here
…ttings.gradle Co-authored-by: Jesse Wilson <[email protected]> Co-authored-by: Hannah Greer <[email protected]>
…sion Co-authored-by: Jesse Wilson <[email protected]>
HLGreer
force-pushed
the
wire-binary-compatibility
branch
from
ecc42a4 to
b868c71
Compare
HLGreer
force-pushed
the
wire-binary-compatibility
branch
from
b868c71 to
af75cfc
Compare
| When a new field is added to the Dinosaur schema, if there are competing versions of the compiled class and the new | ||
| version is resolved at runtime, the usage above may encounter an error: | ||
| ``` | ||
| java.lang.NoSuchMethodError: 'void com.squareup.dinosaurs.Dinosaur<init>(java.lang.String, java.lang.Double, |
| */ | ||
| package com.squareup.wire.binarycompatibility.gradle | ||
|
|
||
| object BuildConfig { |
There was a problem hiding this comment.
Usually this file can be generated from gradle properties.
There was a problem hiding this comment.
I was looking at some prior art! I figured that is a good follow up unless someone felt strongly to have it generated here now
There was a problem hiding this comment.
It must be generated, at least the version component, or it will not work properly in the destination project. It's only a few lines in the Gradle build and you can copy it from Burst or Zipline probably without any changes.
There was a problem hiding this comment.
I think maybe this PR also demonstrates this: https://github.com/square/gradle-dependencies-sorter/pull/123/files
| configure<MavenPublishBaseExtension> { | ||
| configure( | ||
| GradlePlugin( | ||
| javadocJar = JavadocJar.Empty() |
There was a problem hiding this comment.
Out of curiosity, why publish an empty javadoc jar?
There was a problem hiding this comment.
It isn’t a library for external use, but Maven Central requires Javadoc
This change adds a Kotlin compiler plugin that rewrites Wire-generated callsites to be more resilient to schema changes. This first PR only rewrites uses of the generated constructor.
Why:
When a new field is added to a proto
Message, existing usages of the Wire-generated class constructor can cause class conflicts. This is often encountered this when there are duplicate or transitive dependencies pulling in different versions of the same proto, resulting in errors shaped asjava.lang.NoSuchMethodat runtime.When faced with this situation, a frequent recommendation is to refactor the codebase to use the class'
Builder. Rather than requiring individuals to update their codebases, we can adapt their Kotlin code instead. Wrapping up this solution as a plugin allows for individual opt-in and rollout without influencing the core Wire implementation.Next:
I will follow up with a second PR that will rewrite the Wire-generated
copy()method. The copy method currently uses the generated constructor.