diff --git a/build.gradle.kts b/build.gradle.kts index b0f6438ace..5721ea3934 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -459,11 +459,16 @@ tasks.named("sonarqube") { tasks.register("jacocoAggregateReport") { val excludedProjects = emptySet() - // Depend only on testV8DebugUnitTest tasks in subprojects + // Android modules run testV8DebugUnitTest; flavorless ones (plain java-library: :shared, + // :logger, :plugin-api, :eventbus ...) have a plain `test` and no such task. Depending only on + // the former meant nothing in CI ever ran their tests -- sonarqube reaches unit tests solely + // through this task -- so moving a test into one of those modules silently stopped running it, + // which is how ADFA-4649's ReflectUtils regression tests went unguarded (found in review of + // ADFA-5068). dependsOn( subprojects .filterNot { it.name in excludedProjects } - .mapNotNull { it.tasks.findByName("testV8DebugUnitTest") }, + .mapNotNull { it.tasks.findByName("testV8DebugUnitTest") ?: it.tasks.findByName("test") }, ) reports { @@ -480,7 +485,9 @@ tasks.register("jacocoAggregateReport") { "**/*Test*.*", ) - // Collect kotlin and java class directories for v8Debug and v8DebugUnitTest variant + // Collect kotlin and java class directories for v8Debug and v8DebugUnitTest variant, plus the + // flavorless layout (build/classes/{kotlin,java}/main) so a java-library module's coverage is + // reported rather than merely executed. val classDirs = subprojects .filterNot { it.name in excludedProjects } @@ -498,6 +505,12 @@ tasks.register("jacocoAggregateReport") { fileTree(subproj.layout.buildDirectory.dir("intermediates/javac/v8DebugUnitTest/classes")) { exclude(fileFilter) }, + fileTree(subproj.layout.buildDirectory.dir("classes/kotlin/main")) { + exclude(fileFilter) + }, + fileTree(subproj.layout.buildDirectory.dir("classes/java/main")) { + exclude(fileFilter) + }, ) } @@ -511,9 +524,13 @@ tasks.register("jacocoAggregateReport") { val execFiles = subprojects .filterNot { it.name in excludedProjects } - .map { subproj -> - subproj.layout.buildDirectory.file( - "outputs/unit_test_code_coverage/v8DebugUnitTest/testV8DebugUnitTest.exec", + .flatMap { subproj -> + listOf( + subproj.layout.buildDirectory.file( + "outputs/unit_test_code_coverage/v8DebugUnitTest/testV8DebugUnitTest.exec", + ), + // Where a plain java-library's `test` task writes its coverage. + subproj.layout.buildDirectory.file("jacoco/test.exec"), ) } diff --git a/docs/adr/0015-lazy-load-javac-via-dexclassloader.md b/docs/adr/0015-lazy-load-javac-via-dexclassloader.md index 27a84fde47..fb9dcd796f 100644 --- a/docs/adr/0015-lazy-load-javac-via-dexclassloader.md +++ b/docs/adr/0015-lazy-load-javac-via-dexclassloader.md @@ -52,6 +52,9 @@ Investigation found this coupling narrower than it first looked: none of `CacheF - First `.java`-file interaction in a session now pays a one-time synchronous latency spike (asset extraction on first run + `DexClassLoader` construction + `JavaCompilerService`/`SourceFileManager` bootstrap) on top of ADFA-5052's own deferred-reset cost. - A third resident/isolated classloader boundary to reason about (after Kotlin's and the plugin system's). The same rule as ADR 0011 applies and now has two worked examples of getting it wrong: an `api` dependency anywhere in a vendored composite build's *own* `build.gradle.kts` propagates to every consumer's runtime classpath regardless of how the consumer declares its dependency — `compileOnly` has to be applied at the source of the leak, not just where it's consumed. A second, distinct rule this ADR adds: every resident member the isolated fork calls across the boundary must be `public` — `protected`/package-private access throws `IllegalAccessError` at runtime even when both classes share a package name, since ART checks classloader identity, not just the package string, and this has no build-time or unit-test signal at all. - The debugger's breakpoint/stack-frame source-path resolution (`JavaDebugAdapter`/`ModelUtils.asLspLocation`) took on a narrow, real dependency on `JavaCompilerProvider`/`SourceFileObject` that the isolation boundary can't ignore; it now resolves through `IJavaCompilerSession.findSourceFilePath` (returning a plain path, not the isolated `SourceFileObject` type) instead, and degrades to filename-only when no session exists yet. +- A third rule (ADFA-5068): converting a resident dependency from `implementation` to `compileOnly` can trip AGP's `compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)` check for any `com.android.library` module, if that dependency was incidentally anchoring a high-enough transitive version (of e.g. `androidx.annotation`, `kotlin-stdlib`, `org.jetbrains:annotations`) against `androidx.databinding:viewbinding`, which pins those same artifacts much lower on the runtime side once nothing else pulls them in. The fix isn't to re-add the dependency (that reintroduces the duplication this rule exists to avoid) but a `constraints {}` block on `implementation` bumping just the conflicting artifact(s) to the version already used everywhere else — a constraint, unlike a dependency, only rescopes an edge that's already reachable (here, via viewbinding), so it doesn't add anything new to the carrier beyond a version bump on a few KB of annotation classes. See `lsp/java-compiler-impl/build.gradle.kts`'s `constraints` block and `subprojects/javac-services/build.gradle.kts`. + + Two corrections to how this was first written (hal, #1643). **viewbinding is not AGP-imposed**: AGP adds it only when `buildFeatures.viewBinding` is on, and it is on for every module here because our own convention plugin sets it (`AndroidModuleConf.kt`). That matters because it suggests a cheaper fix — turn viewBinding off for the modules that have no layouts, removing the low pins at their source. **That fix was tried and is not sufficient at module scope**: with `viewBinding = false` on both `lsp/java-compiler-impl` and `subprojects/javac-services` and the constraints removed, `v8Release` still fails with `androidx.annotation:{strictly 1.0.0}`, because the pin arrives through the runtime graph from the other modules that still enable viewBinding. Removing these constraints for good means turning viewBinding off by default in the convention plugin and opting the modules with layouts back in — a project-wide change worth its own ticket, not a per-module tweak. ## Alternatives considered diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index eb8d9b0336..0f857cefd1 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -176,6 +176,8 @@ common-javaparser = { module = "com.github.javaparser:javaparser-symbol-solver-c common-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.14.0" } common-io = { module = "commons-io:commons-io", version = "2.15.1" } common-kotlin = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" } +common-kotlin-stdlib = { module = "org.jetbrains.kotlin:kotlin-stdlib", version.ref = "kotlin" } +common-jetbrains-annotations = { module = "org.jetbrains:annotations", version = "24.1.0" } common-kotlin-coroutines-core = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "kotlin-coroutines" } common-kotlin-coroutines-android = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-android", version.ref = "kotlin-coroutines" } common-jkotlin = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" } diff --git a/lsp/java-compiler-impl/build.gradle.kts b/lsp/java-compiler-impl/build.gradle.kts index 77f7c52f06..3b09238b85 100644 --- a/lsp/java-compiler-impl/build.gradle.kts +++ b/lsp/java-compiler-impl/build.gradle.kts @@ -40,6 +40,29 @@ kapt { } dependencies { + // javac-services' (and this module's own) compileOnly androidx-heavy deps (sora-editor, + // appcompat, material, lsp:indexing, etc.) pull androidx.annotation/kotlin-stdlib/ + // org.jetbrains:annotations at high versions onto the compile classpath, but being compileOnly + // they are absent from the runtime classpath -- which sees viewbinding's much lower transitive + // pins for the same artifacts instead. AGP's compile/runtime consistency check cannot reconcile + // the two. Constraining these three (not adding a dependency: each is already reachable via + // viewbinding, just at the wrong version) harmonizes both classpaths at the version used + // everywhere else, without adding an edge. All three are a few KB of interfaces and annotations + // with no resources, unlike the androidx.core duplication this compileOnly effort exists to + // avoid -- see ADFA-5068. + // + // viewbinding is not AGP's doing: our own convention plugin turns buildFeatures.viewBinding on + // for every module (AndroidModuleConf.kt). Turning it off here and in javac-services -- neither + // has a layout -- was tried and is not sufficient: v8Release still fails with + // androidx.annotation:{strictly 1.0.0}, because the pin arrives through the runtime graph from + // the modules that still enable viewBinding. Removing these constraints needs that project-wide, + // not per-module. + constraints { + implementation(libs.androidx.annotation) + implementation(libs.common.kotlin.stdlib) + implementation(libs.common.jetbrains.annotations) + } + kapt(projects.annotationProcessors) // Resident (bundled in the main app dex via editor/editor-api/lsp:java/etc.) -- like the diff --git a/common/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt b/shared/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt similarity index 100% rename from common/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt rename to shared/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt diff --git a/common/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt b/shared/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt similarity index 100% rename from common/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt rename to shared/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt diff --git a/subprojects/javac-services/build.gradle.kts b/subprojects/javac-services/build.gradle.kts index ab698259d4..4819c6f54f 100644 --- a/subprojects/javac-services/build.gradle.kts +++ b/subprojects/javac-services/build.gradle.kts @@ -13,13 +13,39 @@ android { isMinifyEnabled = false } } + + // This module has no layouts, and the convention plugin turns viewBinding on for every module + // (AndroidModuleConf.kt). Leaving it on pulled androidx.databinding:viewbinding in, whose + // transitive androidx.annotation:1.0.0 was the only thing making the imports below compile -- + // and whose low version was what java-compiler-impl's constraints block existed to harmonize. + buildFeatures { + viewBinding = false + } } dependencies { - implementation(libs.common.kotlin) - implementation(libs.google.guava) - implementation(projects.common) - implementation(projects.logger) + // Resident (see docs/adr/0012): kotlin-stdlib, :shared (ReflectUtils, VMUtils) and :logger + // (ILogger) are all already loaded by the parent classloader -- implementation here would + // duplicate their bytecode into the isolated carrier dex. compileOnly for the same reason as + // the block below. libs.google.guava was dropped because this module's own code never + // references it directly; it only ever arrived transitively through :common's api(guava). + // + // That does *not* mean guava stays out of the carrier. It is still on + // :subprojects:java-compiler-carrier's v8ReleaseRuntimeClasspath, reached through + // :build-deps:google-java-format and javaparser-symbol-solver-core -- neither of which goes via + // :common, so neither was affected by this change (hal, #1643). Keeping it out for real means + // deciding what to do about those two edges, and google-java-format needs guava at runtime, so + // it is not a scope change. The compileOnly(libs.google.guava) in java-compiler-impl upholds an + // invariant the build does not currently hold. + compileOnly(libs.common.kotlin) + compileOnly(projects.shared) + compileOnly(projects.logger) + + // Declared, not inherited: three sources here import androidx.annotation, and dropping + // implementation(projects.common) removed the last declared provider. It compiled anyway only + // because viewBinding (now off, above) dragged in androidx.annotation:1.0.0 transitively -- + // an accident of an unrelated build feature. + compileOnly(libs.androidx.annotation) // The actual javac fork this module wraps -- bundled with this module wherever it ends up // (isolated carrier, per ADFA-5053).