From 9c0f191a057a67613be61cad52146a109b5eeac8 Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Wed, 10 May 2023 12:47:16 -0700 Subject: [PATCH 1/6] Check all dependency jars before marking unused A dependency might produce more than one jar (e.g. android targets produce a library jar and a resources jar). If one of those jars is unused, the current logic will mark the dependency as unused. This change only marks the dependency unused if none of the jars are used. --- .../kotlin/builder/tasks/jvm/JdepsMerger.kt | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt index 510d72bc6..48201325f 100644 --- a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt +++ b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt @@ -88,11 +88,22 @@ class JdepsMerger { BufferedOutputStream(File(output).outputStream()).use { it.write(rootBuilder.build().toByteArray()) } - if (reportUnusedDeps != "off") { - val unusedLabels = dependencyMap.values - .filter { it.kind == Deps.Dependency.Kind.UNUSED } - .mapNotNull { readJarOwnerFromManifest(Paths.get(it.path)).label } + val kindMap = mutableMapOf() + + // A target might produce multiple jars (Android produces _resources.jar) + // so we need to make sure wedon't mart the dependency as unused + // unless all of the jars are unused. + dependencyMap.values.forEach { + val label = readJarOwnerFromManifest(Paths.get(it.path)).label + if (label != null && kindMap.getOrDefault(label, Deps.Dependency.Kind.UNUSED) >= it.kind) { + kindMap.put(label, it.kind) + } + } + + val unusedLabels = kindMap.entries + .filter { it.value == Deps.Dependency.Kind.UNUSED } + .map { it.key } .filter { it != label } if (unusedLabels.isNotEmpty()) { From 2fc7a64ddd04d46ba8d9fb61abbef1d605cf6daa Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Wed, 10 May 2023 13:40:21 -0700 Subject: [PATCH 2/6] Fix format --- .../kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt index 48201325f..f6164004d 100644 --- a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt +++ b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt @@ -96,7 +96,8 @@ class JdepsMerger { // unless all of the jars are unused. dependencyMap.values.forEach { val label = readJarOwnerFromManifest(Paths.get(it.path)).label - if (label != null && kindMap.getOrDefault(label, Deps.Dependency.Kind.UNUSED) >= it.kind) { + if (label != null && + kindMap.getOrDefault(label, Deps.Dependency.Kind.UNUSED) >= it.kind) { kindMap.put(label, it.kind) } } From 0f797c7b75fea1121e17e5f5d4c052c8a5df05d2 Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Wed, 10 May 2023 13:49:51 -0700 Subject: [PATCH 3/6] ran ktlint --- .../kotlin/builder/tasks/jvm/JdepsMerger.kt | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt index f6164004d..9e6cb41b0 100644 --- a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt +++ b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt @@ -72,8 +72,21 @@ class JdepsMerger { val deps: Deps.Dependencies = Deps.Dependencies.parseFrom(it) deps.getDependencyList().forEach { val dependency = dependencyMap.get(it.path) - // Replace dependency if it has a stronger kind than one we encountered before. - if (dependency == null || dependency.kind > it.kind) { + if (dependency != null) { + // Replace dependency if it has a stronger kind than one we encountered before, and + // merge used classes list. + if (dependency.kind > it.kind) { + dependencyMap.put( + it.path, + it.toBuilder().addAllUsedClasses(dependency.getUsedClassesList()).build(), + ) + } else { + dependencyMap.put( + it.path, + dependency.toBuilder().addAllUsedClasses(it.getUsedClassesList()).build(), + ) + } + } else { dependencyMap.put(it.path, it) } } @@ -88,6 +101,7 @@ class JdepsMerger { BufferedOutputStream(File(output).outputStream()).use { it.write(rootBuilder.build().toByteArray()) } + if (reportUnusedDeps != "off") { val kindMap = mutableMapOf() @@ -97,8 +111,9 @@ class JdepsMerger { dependencyMap.values.forEach { val label = readJarOwnerFromManifest(Paths.get(it.path)).label if (label != null && - kindMap.getOrDefault(label, Deps.Dependency.Kind.UNUSED) >= it.kind) { - kindMap.put(label, it.kind) + kindMap.getOrDefault(label, Deps.Dependency.Kind.UNUSED) >= it.kind + ) { + kindMap.put(label, it.kind) } } From eb1a95c2ef5619303fd9dd811dc89be65b0bbad7 Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Wed, 10 May 2023 13:53:47 -0700 Subject: [PATCH 4/6] Update JdepsMerger.kt --- .../kotlin/builder/tasks/jvm/JdepsMerger.kt | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt index 9e6cb41b0..2e549f89d 100644 --- a/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt +++ b/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMerger.kt @@ -72,21 +72,8 @@ class JdepsMerger { val deps: Deps.Dependencies = Deps.Dependencies.parseFrom(it) deps.getDependencyList().forEach { val dependency = dependencyMap.get(it.path) - if (dependency != null) { - // Replace dependency if it has a stronger kind than one we encountered before, and - // merge used classes list. - if (dependency.kind > it.kind) { - dependencyMap.put( - it.path, - it.toBuilder().addAllUsedClasses(dependency.getUsedClassesList()).build(), - ) - } else { - dependencyMap.put( - it.path, - dependency.toBuilder().addAllUsedClasses(it.getUsedClassesList()).build(), - ) - } - } else { + // Replace dependency if it has a stronger kind than one we encountered before. + if (dependency == null || dependency.kind > it.kind) { dependencyMap.put(it.path, it) } } From 9c6284e5d2dcf134cdf9ff16f834bdd1fa81299c Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Thu, 11 May 2023 13:46:26 -0700 Subject: [PATCH 5/6] Add tests --- .../builder/tasks/jvm/JdepsMergerTest.kt | 128 +++++++++++++++--- 1 file changed, 109 insertions(+), 19 deletions(-) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt index db2eb3e69..acb7d64e7 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt @@ -25,7 +25,7 @@ class JdepsMergerTest { private fun jdeps( path: String, - buildDeps: (Deps.Dependencies.Builder.() -> Deps.Dependencies.Builder) + buildDeps: (Deps.Dependencies.Builder.() -> Deps.Dependencies.Builder), ): Path { val jdepsPath = Files.createDirectories(wrkDir.resolve("jdeps")).resolve(path) @@ -40,12 +40,12 @@ class JdepsMergerTest { return Files.createDirectories(wrkDir.resolve("out")).resolve(name) } - fun ktJvmLibrary(label: String): String { - val path = Files.createDirectories(wrkDir.resolve("out")).resolve("lib$label.jar") + fun ktJvmLibrary(label: String, suffix: String = ""): String { + val path = Files.createDirectories(wrkDir.resolve("out")).resolve("lib${label}$suffix.jar") JarCreator( path = path, normalize = true, - verbose = false + verbose = false, ).also { it.setJarOwner(label, "kt_jvm_library") it.execute() @@ -63,7 +63,7 @@ class JdepsMergerTest { kind = Dependency.Kind.EXPLICIT path = "/path/to/kt_dep.jar" build() - } + }, ) } @@ -73,7 +73,7 @@ class JdepsMergerTest { kind = Dependency.Kind.EXPLICIT path = "/path/to/java_dep.jar" build() - } + }, ) } @@ -89,7 +89,7 @@ class JdepsMergerTest { input(javaJdeps) flag(JdepsMergerFlags.OUTPUT, mergedJdeps) flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "off") - } + }, ) } } @@ -100,7 +100,7 @@ class JdepsMergerTest { assertThat(depsProto.ruleLabel).isEqualTo("//foo/bar:baz") assertThat(depsProto.dependencyList.map { it.path }).containsExactly( "/path/to/kt_dep.jar", - "/path/to/java_dep.jar" + "/path/to/java_dep.jar", ) } @@ -114,7 +114,7 @@ class JdepsMergerTest { kind = Dependency.Kind.UNUSED path = "/path/to/shared_dep.jar" build() - } + }, ) } @@ -124,7 +124,7 @@ class JdepsMergerTest { kind = Dependency.Kind.EXPLICIT path = "/path/to/shared_dep.jar" build() - } + }, ) } @@ -140,7 +140,7 @@ class JdepsMergerTest { input(javaJdeps) flag(JdepsMergerFlags.OUTPUT, mergedJdeps) flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "off") - } + }, ) } } @@ -163,7 +163,7 @@ class JdepsMergerTest { kind = Dependency.Kind.UNUSED path = unusedKotlinDep build() - } + }, ) } @@ -173,7 +173,7 @@ class JdepsMergerTest { kind = Dependency.Kind.EXPLICIT path = ktJvmLibrary("java_dep") build() - } + }, ) } @@ -189,7 +189,7 @@ class JdepsMergerTest { flag(JdepsMergerFlags.TARGET_LABEL, "//foo/bar:baz") flag(JdepsMergerFlags.OUTPUT, mergedJdeps) flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "warn") - } + }, ) } } @@ -200,7 +200,7 @@ class JdepsMergerTest { assertThat( depsProto.dependencyList .filter { it.kind == Dependency.Kind.UNUSED } - .map { it.path } + .map { it.path }, ).containsExactly(unusedKotlinDep) } @@ -215,7 +215,7 @@ class JdepsMergerTest { kind = Dependency.Kind.UNUSED path = unusedKotlinDep build() - } + }, ) } @@ -225,7 +225,7 @@ class JdepsMergerTest { kind = Dependency.Kind.EXPLICIT path = ktJvmLibrary("java_dep") build() - } + }, ) } @@ -242,7 +242,7 @@ class JdepsMergerTest { flag(JdepsMergerFlags.TARGET_LABEL, "//foo/bar:baz") flag(JdepsMergerFlags.OUTPUT, mergedJdeps) flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "error") - } + }, ) } } @@ -253,10 +253,100 @@ class JdepsMergerTest { assertThat( depsProto.dependencyList .filter { it.kind == Dependency.Kind.UNUSED } - .map { it.path } + .map { it.path }, ).containsExactly(unusedKotlinDep) } + @Test + fun `unused deps multiple jars for label`() { + val merger = DaggerJdepsMergerTestComponent.builder().build().jdepsMerger() + + val unusedKotlinDepA = ktJvmLibrary("kotlin_dep", "_a") + val unusedKotlinDepB = ktJvmLibrary("kotlin_dep", "_b") + val kotlinJdeps = jdeps("kt.jdeps") { + addDependency( + with(Dependency.newBuilder()) { + kind = Dependency.Kind.UNUSED + path = unusedKotlinDepA + build() + }, + ) + addDependency( + with(Dependency.newBuilder()) { + kind = Dependency.Kind.UNUSED + path = unusedKotlinDepB + build() + }, + ) + } + + val mergedJdeps = out("merged.jdeps") + + val result = WorkerContext.run { + doTask("jdepsmerge") { taskCtx -> + MergeJdeps(merger = merger).invoke( + taskCtx, + args { + input(kotlinJdeps) + flag(JdepsMergerFlags.TARGET_LABEL, "//foo/bar:baz") + flag(JdepsMergerFlags.OUTPUT, mergedJdeps) + flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "warn") + }, + ) + } + } + assertThat(result.status).isEqualTo(SUCCESS) + assertThat(result.log.out.toString()).contains("'remove deps kotlin_dep' //foo/bar:baz") + } + + @Test + fun `used deps multiple jars for label`() { + val merger = DaggerJdepsMergerTestComponent.builder().build().jdepsMerger() + + val unusedKotlinDepA = ktJvmLibrary("kotlin_dep", "_a") + val unusedKotlinDepB = ktJvmLibrary("kotlin_dep", "_b") + val kotlinJdeps = jdeps("kt.jdeps") { + addDependency( + with(Dependency.newBuilder()) { + kind = Dependency.Kind.UNUSED + path = unusedKotlinDepA + build() + }, + ) + addDependency( + with(Dependency.newBuilder()) { + kind = Dependency.Kind.EXPLICIT + path = unusedKotlinDepB + build() + }, + ) + } + + val mergedJdeps = out("merged.jdeps") + + val result = WorkerContext.run { + doTask("jdepsmerge") { taskCtx -> + MergeJdeps(merger = merger).invoke( + taskCtx, + args { + input(kotlinJdeps) + flag(JdepsMergerFlags.TARGET_LABEL, "//foo/bar:baz") + flag(JdepsMergerFlags.OUTPUT, mergedJdeps) + flag(JdepsMergerFlags.REPORT_UNUSED_DEPS, "warn") + }, + ) + } + } + assertThat(result.status).isEqualTo(SUCCESS) + + val depsProto = depsProto(mergedJdeps) + assertThat( + depsProto.dependencyList + .filter { it.kind == Dependency.Kind.EXPLICIT } + .map { it.path }, + ).containsExactly(unusedKotlinDepB) + } + private fun depsProto(mergedJdeps: Path) = Deps.Dependencies.parseFrom(BufferedInputStream(Files.newInputStream(mergedJdeps))) From 4521b2e752d8bea2e4817a5b163b44728faf8558 Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Thu, 11 May 2023 14:00:30 -0700 Subject: [PATCH 6/6] Rename variables --- .../bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt index acb7d64e7..2f4e2b187 100644 --- a/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt +++ b/src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/JdepsMergerTest.kt @@ -303,20 +303,20 @@ class JdepsMergerTest { fun `used deps multiple jars for label`() { val merger = DaggerJdepsMergerTestComponent.builder().build().jdepsMerger() - val unusedKotlinDepA = ktJvmLibrary("kotlin_dep", "_a") - val unusedKotlinDepB = ktJvmLibrary("kotlin_dep", "_b") + val unusedKotlinDep = ktJvmLibrary("kotlin_dep", "_a") + val usedKotlinDep = ktJvmLibrary("kotlin_dep", "_b") val kotlinJdeps = jdeps("kt.jdeps") { addDependency( with(Dependency.newBuilder()) { kind = Dependency.Kind.UNUSED - path = unusedKotlinDepA + path = unusedKotlinDep build() }, ) addDependency( with(Dependency.newBuilder()) { kind = Dependency.Kind.EXPLICIT - path = unusedKotlinDepB + path = usedKotlinDep build() }, ) @@ -344,7 +344,7 @@ class JdepsMergerTest { depsProto.dependencyList .filter { it.kind == Dependency.Kind.EXPLICIT } .map { it.path }, - ).containsExactly(unusedKotlinDepB) + ).containsExactly(usedKotlinDep) } private fun depsProto(mergedJdeps: Path) =