Skip to content

Declare byte-buddy as 'implementation' dependency instead of shading. #4266

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

Merged
merged 9 commits into from
Nov 18, 2024
1 change: 0 additions & 1 deletion benchmarks/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import org.jetbrains.kotlin.gradle.tasks.*
import org.jetbrains.kotlin.gradle.dsl.JvmTarget

plugins {
id("com.github.johnrengelman.shadow")
id("me.champeau.jmh")
}

Expand Down
3 changes: 0 additions & 3 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ buildscript {
classpath("ru.vyarus:gradle-animalsniffer-plugin:${version("animalsniffer")}") // Android API check
classpath("org.jetbrains.kotlin:atomicfu:${version("kotlin")}")
classpath("org.jetbrains.kotlinx:kover-gradle-plugin:${version("kover")}")

// JMH plugins
classpath("gradle.plugin.com.github.johnrengelman:shadow:${version("shadow")}")
}

with(CacheRedirector) { buildscript.configureBuildScript(rootProject) }
Expand Down
4 changes: 0 additions & 4 deletions buildSrc/src/main/kotlin/version-file-conventions.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,6 @@ import org.gradle.api.tasks.bundling.*
configure(subprojects.filter { !unpublished.contains(it.name) && it.name !in sourceless }) {
val project = this
val jarTaskName = when {
project.name == "kotlinx-coroutines-debug" -> {
project.apply(plugin = "com.github.johnrengelman.shadow")
"shadowJar"
}
isMultiplatform -> "jvmJar"
else -> "jar"
}
Expand Down
2 changes: 0 additions & 2 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ jna_version=5.9.0
# Gradle
jdk_toolchain_version=11
animalsniffer_version=1.7.1
shadow_version=7.1.2
#shadow_version=8.1.1 For Gradle 8, not compatible with Gradle 7

# Android versions
android_version=4.1.1.4
Expand Down
9 changes: 3 additions & 6 deletions kotlinx-coroutines-debug/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,9 @@ dependency of `kotlinx-coroutines-test`) may fail with `DuplicateRelativeFileExc

The problem is that Android merges the resources of all its dependencies into a single directory and complains about
conflicts, but:
* `kotlinx-coroutines-debug` transitively depends on JNA and JNA-platform, both of which include license files in their
META-INF directories. Trying to merge these files leads to conflicts, which means that any Android project that
depends on JNA and JNA-platform will experience build failures.
* Additionally, `kotlinx-coroutines-debug` embeds `byte-buddy-agent` and `byte-buddy`, along with their resource files.
Then, if the project separately depends on `byte-buddy`, merging the resources of `kotlinx-coroutines-debug` with ones
from `byte-buddy` and `byte-buddy-agent` will lead to conflicts as the resource files are duplicated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this change, the list above only has a single bullet point (so it's strange that it's still a list), and also, in addition to JNA and JNA-platform mentioned above, kotlinx-coroutines-debug now depends on byte-buddy and byte-buddy-agent.

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, I think one of two ways is more efficient than ping-ponging in this review.
Either we merge the contribution as is and you can quickly edit the readme to your liking in a new commit, or you might just want to edit the commit directly pre merge.

WDYT?

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Nov 13, 2024

Choose a reason for hiding this comment

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

There's also a third way, the one we usually take in this library: the contributor carefully looks at the places affected by the proposed change, considers the consequences, and then introduces sweeping, coordinated edits. In other words, instead of making a small isolated change, they consider the bigger picture. A reviewer's job then boils down to highlighting the affected areas the contributor hasn't thought of / didn't know about (without enumerating every specific line of code to change), after which the contributor goes back to researching what the proposed change entails and introduces further sweeping edits. No ping-ponging results, as the list of such areas to consider is typically limited.

Although the third way is preferred, both of the options you've proposed are okay: we're also glad and thankful to accept incomplete contributions if the contributor says: "I'm no longer interested in working on this, you do it." In this scenario, the credit still goes to the contributor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed an update, please provide feedback

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, thanks! I've also left a comment with my review, it's possible that you've missed it:

With this change, we are no longer shading anything, so could you please also remove all mentions of the shadow plugin from our library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

`kotlinx-coroutines-debug` transitively depends on JNA and JNA-platform, byte-buddy and byte-buddy-agent, all of them include license files in their
META-INF directories. Trying to merge these files leads to conflicts, which means that any Android project that
depends on JNA and JNA-platform will experience build failures.

One possible workaround for these issues is to add the following to the `android` block in your gradle file for the
application subproject:
Expand Down
63 changes: 13 additions & 50 deletions kotlinx-coroutines-debug/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -1,22 +1,12 @@
import com.github.jengelman.gradle.plugins.shadow.tasks.*
import java.net.*
import java.nio.file.*
import org.gradle.api.JavaVersion
import org.gradle.api.file.DuplicatesStrategy
import org.gradle.api.tasks.bundling.Jar
import org.gradle.api.tasks.testing.Test

plugins {
id("com.github.johnrengelman.shadow")
id("org.jetbrains.kotlinx.kover") // apply plugin to use autocomplete for Kover DSL
}

configurations {
val shadowDeps by creating
compileOnly.configure {
extendsFrom(shadowDeps)
}
runtimeOnly.configure {
extendsFrom(shadowDeps)
}
}

val junit_version by properties
val junit5_version by properties
val byte_buddy_version by properties
Expand All @@ -28,8 +18,8 @@ dependencies {
compileOnly("org.junit.jupiter:junit-jupiter-api:$junit5_version")
testImplementation("org.junit.jupiter:junit-jupiter-engine:$junit5_version")
testImplementation("org.junit.platform:junit-platform-testkit:1.7.0")
add("shadowDeps", "net.bytebuddy:byte-buddy:$byte_buddy_version")
add("shadowDeps", "net.bytebuddy:byte-buddy-agent:$byte_buddy_version")
implementation("net.bytebuddy:byte-buddy:$byte_buddy_version")
implementation("net.bytebuddy:byte-buddy-agent:$byte_buddy_version")
compileOnly("io.projectreactor.tools:blockhound:$blockhound_version")
testImplementation("io.projectreactor.tools:blockhound:$blockhound_version")
testImplementation("com.google.code.gson:gson:2.8.6")
Expand All @@ -50,21 +40,7 @@ tasks.withType<Test>().configureEach {
}
}

val jar by tasks.existing(Jar::class) {
enabled = false
}

val shadowJar by tasks.existing(ShadowJar::class) {
// Shadow only byte buddy, do not package kotlin stdlib
configurations = listOf(project.configurations["shadowDeps"])
relocate("net.bytebuddy", "kotlinx.coroutines.repackaged.net.bytebuddy")
/* These classifiers are both set to `null` to trick Gradle into thinking that this jar file is both the
artifact from the `jar` task and the one from `shadowJar`. Without this, Gradle complains that the artifact
from the `jar` task is not present when the compilaton finishes, even if the file with this name exists. */
archiveClassifier.convention(null as String?)
archiveClassifier = null
archiveBaseName = jar.flatMap { it.archiveBaseName }
archiveVersion = jar.flatMap { it.archiveVersion }
tasks.named<Jar>("jar") {
manifest {
attributes(
mapOf(
Expand All @@ -74,30 +50,17 @@ val shadowJar by tasks.existing(ShadowJar::class) {
)
)
}

// add module-info.class to the META-INF/versions/9/ directory.
dependsOn(tasks.compileModuleInfoJava)
doLast {
// We can't do that directly with the shadowJar task because it doesn't support replacing existing files.
val zipPath = [email protected]()
val zipUri = URI.create("jar:${zipPath.toUri()}")
val moduleInfoFilePath = tasks.compileModuleInfoJava.get().outputs.files.asFileTree.matching {
include("module-info.class")
}.singleFile.toPath()
FileSystems.newFileSystem(zipUri, emptyMap<String, String>()).use { fs ->
val moduleInfoFile = fs.getPath("META-INF/versions/9/module-info.class")
Files.copy(moduleInfoFilePath, moduleInfoFile, StandardCopyOption.REPLACE_EXISTING)
}
from(tasks.compileModuleInfoJava.get().outputs.files.asFileTree.matching {
include("module-info.class")
}) {
this.duplicatesStrategy = DuplicatesStrategy.INCLUDE
into("META-INF/versions/9")
}
}

configurations {
// shadowJar is already part of the `shadowRuntimeElements` and `shadowApiElements`, but the other subprojects
// that depend on `kotlinx-coroutines-debug` look at `runtimeElements` and `apiElements`.
artifacts {
add("apiElements", shadowJar)
add("runtimeElements", shadowJar)
}
}

kover {
reports {
Expand Down