Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -459,11 +459,16 @@ tasks.named("sonarqube") {
tasks.register<JacocoReport>("jacocoAggregateReport") {
val excludedProjects = emptySet<String>()

// 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 {
Expand All @@ -480,7 +485,9 @@ tasks.register<JacocoReport>("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 }
Expand All @@ -498,6 +505,12 @@ tasks.register<JacocoReport>("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)
},
)
}

Expand All @@ -511,9 +524,13 @@ tasks.register<JacocoReport>("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"),
)
}

Expand Down
3 changes: 3 additions & 0 deletions docs/adr/0015-lazy-load-javac-via-dexclassloader.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
23 changes: 23 additions & 0 deletions lsp/java-compiler-impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Comment thread
davidschachterADFA marked this conversation as resolved.
File renamed without changes.
34 changes: 30 additions & 4 deletions subprojects/javac-services/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment thread
davidschachterADFA marked this conversation as resolved.

// 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).
Expand Down
Loading