ADFA-5068: Fix javac-services dependency scope without breaking AGP's consistency check - #1643
Conversation
… consistency check
javac-services declared kotlin-stdlib, guava, :common, and :logger as
implementation even though all are resident, leaking androidx.core's
2.87MB of dead resources and duplicate guava/kotlin-stdlib bytecode
into java-compiler-carrier.apk. Switching those to compileOnly broke
AGP's compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)
check one level up in java-compiler-impl: removing :common's implicit
implementation edge meant nothing but AGP's own low-pinned viewbinding
was left providing androidx.annotation/kotlin-stdlib/org.jetbrains:
annotations on the runtime classpath, while compileOnly deps (sora-
editor, appcompat, material, lsp:indexing) still demanded much higher
versions on the compile side.
Relocated ReflectUtils (the one :common symbol javac-services actually
uses; VMUtils and ILogger already lived in :shared/:logger) there, and
added a constraints{} block in java-compiler-impl pinning the three
conflicting artifacts to the versions already used everywhere else in
the project -- harmonizing both classpaths without re-adding a real
dependency edge. guava was dropped outright: javac-services' own code
never references it directly.
Verified via a from-scratch java-compiler-carrier release build:
32.4MB -> 10.2MB, resources.arsc 2.27MB -> 40 bytes (empty), dex
9.1+8.1+10.5+2.6MB across 5 files -> 8.4+2.1MB across 2.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Moving this test to :shared drops all 9 ReflectUtils tests from CI.
:shared is a plain java-library -- it has a test task, not testV8DebugUnitTest. Nothing in CI runs it:
jacocoAggregateReport(build.gradle.kts:459-467) depends only on each subproject'stestV8DebugUnitTest.github/workflows/analyze.yml:108reaches unit tests solely throughsonarqube->jacocoAggregateReport
So these tests passed locally, then silently stopped being executed anywhere. Concretely, the stripFinalModifier final-field regression that ADFA-4649 added these tests to guard is now unguarded.
This is the same flavorless-module coverage gap that already hides plugin-api from the nightly report, so the fix is worth making general: have the aggregate task also pick up test on flavorless modules. Otherwise the test has to stay where it actually runs.
| // (ILogger) are all already loaded by the parent classloader -- implementation here would | ||
| // duplicate their bytecode (and :common's androidx/guava graph, previously reached via | ||
| // projects.common) into the isolated carrier dex. compileOnly for the same reason as the | ||
| // block below. libs.google.guava was dropped entirely: this module's own code never |
There was a problem hiding this comment.
guava is still duplicated into the carrier -- this comment claims otherwise.
Dropping the libs.google.guava line changed nothing about what ships. Resolving the carrier's actual runtime classpath:
flox activate -d flox/local -- ./gradlew :subprojects:java-compiler-carrier:dependencies \
--configuration v8ReleaseRuntimeClasspath
com.google.guava:guava:33.4.0-android is still on it, reached two independent ways:
com.github.javaparser:javaparser-symbol-solver-core:3.25.8, animplementationdep of:lsp:java-compiler-impl:build-deps:google-java-format
Neither goes through :common, so neither was ever affected by this change. That means the invariant compileOnly(libs.google.guava) in java-compiler-impl exists to uphold -- guava must never be duplicated into the isolated carrier -- is not actually held today.
Cutting it for real needs an exclude group: "com.google.guava" on those two edges, not a scope change here. As written, the next person reads "dropped entirely", believes guava is handled, and skips the fix that would actually do it.
| compileOnly(libs.common.kotlin) | ||
| compileOnly(projects.shared) | ||
| compileOnly(projects.logger) |
There was a problem hiding this comment.
Missing compileOnly(libs.androidx.annotation).
Three sources in this module import androidx.annotation:
partial/DiagnosticListenerImpl.java:20util/ReparserUtils.java:20compiler/JavacFlowListener.java:20
Dropping implementation(projects.common) removed the last declared provider of that package. :shared does declare androidx.annotation, but as implementation on a java-library, so it is not exported to consumers' compile classpath.
The module compiles anyway only because AndroidModuleConf.kt:273 turns on buildFeatures.viewBinding for every module, and viewbinding drags in androidx.annotation:1.0.0 transitively. That is an accident of an unrelated build feature, and it's the same class of incidental anchoring this PR exists to remove.
Concrete failure: turn viewBinding off for this module (it has no layouts -- see the ADR thread) and compilation breaks with package androidx.annotation does not exist.
Please declare it explicitly:
compileOnly(libs.androidx.annotation)| - 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 AGP's own unconditionally-injected `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`. |
There was a problem hiding this comment.
viewbinding is not "unconditionally-injected" by AGP -- this repo injects it.
AGP adds androidx.databinding:viewbinding only when buildFeatures.viewBinding is on. It is on for every module here because our own convention plugin sets it:
composite-builds/build-logic/plugins/src/main/java/com/itsaky/androidide/plugins/conf/AndroidModuleConf.kt:273
This isn't a nitpick about attribution -- it changes what the recommended fix should be. Framed as an AGP-imposed constraint, a constraints {} block is the only move left, and the 17 lines in java-compiler-impl become permanent. Framed accurately, the cheaper fix is available: neither lsp/java-compiler-impl nor subprojects/javac-services has layouts, so setting
android { buildFeatures { viewBinding = false } }on those two removes the low version pins at the source, and the constraints block along with them.
Worth at least evaluating before enshrining the workaround as the documented rule for the next person.
2 participants
Summary
Stacked on #1638 (ADFA-5053) -- this needs its
java-compiler-carriermodule and ADR 0012.Follow-up from ADFA-5053's APK size audit (ADFA-5068).
javac-servicesdeclared kotlin-stdlib, guava,:common, and:loggerasimplementationeven though all four are resident, leaking androidx.core's ~2.87MB of dead resources plus duplicate guava/kotlin-stdlib bytecode intojava-compiler-carrier.apk. Switching those tocompileOnlybroke AGP'scompileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)check one level up injava-compiler-impl, once:common's androidx graph stopped incidentally anchoring a high-enough version against AGP's own low-pinnedviewbindinginjection.ReflectUtils(the one:commonsymbol javac-services actually uses --VMUtils/ILoggeralready lived in:shared/:logger) from:commonto:shared.javac-services:common.kotlin/projects.common/projects.logger->compileOnly(shared)/compileOnly(logger)/compileOnly(kotlin); dropped theguavaline entirely (confirmed dead -- javac-services' own code never references it).constraints {}block injava-compiler-implpinningandroidx.annotation/kotlin-stdlib/org.jetbrains:annotationsto the versions already used elsewhere in the project. A constraint only rescopes an edge already reachable via AGP's injected viewbinding, so it doesn't reintroduce the duplication this fix removes -- just a version bump on a few KB of annotation classes.compileOnlyrule.Verified
From-scratch
java-compiler-carrier:assembleV8Release:resources.arsc: 2.27MB -> 40 bytes (empty; all androidx.coreres/entries gone)Test plan
:shared:test,:subprojects:javac-services:testV8DebugUnitTest,:lsp:java-compiler-impl:testV8DebugUnitTest,:common:testV8DebugUnitTestall green:subprojects:java-compiler-carrier:assembleV8Releasesucceeds and byte sizes confirmed abovespotlessApplyclean🤖 Generated with Claude Code