Speed up and harden the android-gradle-dependencies toolchain task - #4445
Conversation
|
I do intend to explore porting the "Check the packaged repositories against the inventory the enumeration pass leaves behind" commit upstream eventually. Seems like a much more robust way to achieve what that check was there to do. |
bfeb3dc to
056e9c8
Compare
|
Saves 12-13min on the gradle-dependencies run and gives us a most robust cache in the end (already confirmed that the DAGP 3.18.0 update PR is unblocked on top of this stack). |
|
Queued — the merge queue status continues in this comment ↓. |
ncalexan
left a comment
ncalexan
left a comment
There was a problem hiding this comment.
This looks sensible. Pity about the (arch dependent, IIRC!) aapt2 nonsense.
Merge Queue Status
This pull request spent 10 minutes 30 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #4445 has been dequeued Queue conditions are not satisfied:
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
ncalexan
left a comment
ncalexan
left a comment
There was a problem hiding this comment.
I'm not a big fan of Gradle properties for this, but fine.
|
|
||
| GRADLE_FLAGS=( | ||
| --no-daemon | ||
| # Overrides the daemon=false the base image puts in GRADLE_OPTS. |
There was a problem hiding this comment.
Why not fix the base image?
There was a problem hiding this comment.
GRADLE_OPTS in the base image applies to every gradlew task in the repo, and each of those invokes Gradle exactly once (job.py builds a single ./gradlew command per task). A daemon there is pure overhead: nothing reuses it, and it sits on its heap for the remainder of the task. This toolchain task is the only one that runs Gradle twice back to back, so it's the only place the daemon pays for itself. The second pass went from 40s to 25s in CI once it could reuse a warm one, which is also why the script stops the daemon before after.sh starts copying and compressing.
| # AGP resolves the aapt2 binary while its tasks run, so the pass above misses it. | ||
| ./gradlew "${GRADLE_FLAGS[@]}" :app:processDebugResources | ||
|
|
||
| # Don't leave a 4GB heap sitting there while `after.sh` packages everything up. |
There was a problem hiding this comment.
Huh, maybe we should do this upstream? Although... we might not be daemon-ing in a-g-d.
There was a problem hiding this comment.
Yeah, upstream runs without a daemon.
…e tasks Ported from bug 1977709 in mozilla-central (D257608). Rather than building everything in order to pull dependencies into the local Nexus, ask Gradle to enumerate and download the dependencies of every resolvable configuration, and use --dry-run to stop the requested tasks from actually running. Unlike mozilla-central, one real task execution is still needed. AGP resolves the aapt2 binary through a detached configuration while its tasks are running, so the enumeration never sees it, and downstream tasks are given the packaged repository as their only google() repo, so a missing aapt2 is a hard resolution failure for them. :app:processDebugResources is the cheapest task that forces the download. mozilla-central gets away without this because its builds resolve from GRADLE_MAVEN_REPOSITORIES, which normally points at the live remote repos.
Replace `assemble assembleAndroidTest bundle test lint ktlint detekt` with `detekt ktlint lint buildHealth build`, which is what the build, test and lint kinds run. buildHealth was missing from the old list entirely. This does not change what gets downloaded. With --write-verification-metadata, Gradle resolves every resolvable configuration in every project regardless of which tasks were requested, so the task list no longer drives coverage. Both lists were checked to enumerate an identical set of 915 components.
Matches what mozilla-central does now. xz is single threaded on both ends and this artifact is unpacked by the build, build-bundle, test and lint kinds. Two additions to the base image are needed for this. `tar -a` shells out to the zstd binary, which is priority optional on Ubuntu and so is not installed (libzstd1 is present, but that is only the shared library). And run-task's fetch-content decompresses .tar.zst with the zstandard Python package, failing with "zstandard Python package not available" without it; the taskcluster package we already install does not pull it in. The base image covers both sides, since android-build and ui-tests derive from it and every kind that fetches this artifact uses it. No taskgraph change is needed: fetch-content already treats any *.tar.* as a tar and sniffs the zstd magic to pick a decompressor. Verified by extracting a `tar cavf` archive with the pinned taskgraph's fetch-content, which fails without the zstandard package and succeeds with it.
buildSrc declares a bare mavenCentral(), so unlike the root project it ignores the centralRepo property that CI points at the local Nexus. Its classpath is therefore fetched straight from Maven Central and never lands in the android-gradle-dependencies artifact, even though the artifact is otherwise meant to be the only Maven Central the downstream tasks see. Mirror what the root project does, and add buildSrc to the toolchain task's resources. buildSrc/build.gradle.kts didn't match any of them, `*.gradle` not covering `.kts`, so with buildSrc's classpath now part of the artifact a change to it would otherwise silently reuse a stale one.
Now that this task runs Gradle twice in a row, running each one without a daemon makes the second pay for a cold JVM again. Measured locally, the second pass drops from 37s to 13s when it can reuse a warm one. Dropping --no-daemon is not enough on its own: the base image puts -Dorg.gradle.daemon=false in GRADLE_OPTS, which a first attempt at this showed still forks a single-use daemon per invocation in CI. Pass --daemon explicitly to override it, and stop the daemon once both passes are done rather than leaving it holding its heap while after.sh copies, hashes and compresses the artifact.
Project dependencies already resolve through the local Nexus, but plugins did not, and that left the packaged artifact incomplete in a way that is impossible to distinguish from the packaging having dropped something. Two separate holes. pluginManagement in settings.gradle declared google() and mavenCentral() outright, and Gradle contributes those to the buildscript classpath alongside the proxies, so resolution could take a component's pom through Nexus and its jar from the unproxied twin. buildSrc resolves its own plugins separately again, from the plugin portal, which nothing pointed at Nexus either. A CI run packaged com.google.errorprone's annotations as a pom with no jar, which either hole would explain. nexus.xml already had a gradle-plugins proxy for the portal, unused until now, so this only has to point at it and package its storage. buildSrc needs its own settings.gradle.kts because the root one doesn't govern it, and the Kotlin DSL plugin it applies is published to the portal and not to Maven Central. The two Mozilla repositories are still not proxied, but they serve nothing outside org.mozilla, so they can only ever account for a component being absent in full, never for a component that is packaged with a file missing.
ncalexan
left a comment
ncalexan
left a comment
There was a problem hiding this comment.
AI slop is slop, but there's nothing worrying me here. A few nits. This would be nice to clean up and lift to mozilla-central; it's better than what we have.
| parser.add_argument( | ||
| "--unproxied", | ||
| nargs="*", | ||
| default=["org/mozilla"], |
There was a problem hiding this comment.
The default shouldn't be org/mozilla; this should be passed on the command line.
There was a problem hiding this comment.
Done. The default is gone and after.sh passes --unproxied org/mozilla explicitly, with a comment there tying it to the unproxied Mozilla repositories in build.gradle. I also switched it to action="append" so it takes one prefix per flag, which avoids needing a -- separator before the repository arguments. Omitting the flag now makes every absence fatal rather than silently tolerated, which seems like the right direction to fail.
| f"components; {len(unpackaged)} are not packaged, coming from a " | ||
| "repository we don't proxy" | ||
| ) | ||
| for path in unpackaged[:10]: |
There was a problem hiding this comment.
What? No, show them all. Or accept a verbose flag.
There was a problem hiding this comment.
Fixed, it prints all of them now. In practice that is 98 lines on a successful run, all of them org/mozilla.
There was a problem hiding this comment.
We can revisit adding a --verbose flag when upstreaming after I've had a chance to see what the output looks like there.
|
|
||
| if problems: | ||
| print(f"FATAL ERROR: {len(problems)} absent, missing or corrupt:") | ||
| for problem in problems[:50]: |
There was a problem hiding this comment.
Same, no truncation.
Picked up from bug 1953671 in mozilla-central. This task has intermittently produced artifacts with files missing, which only surfaces much later as a confusing resolution failure in a downstream task. mozilla-central checks a single hardcoded artifact as a canary. We can do better: the enumeration pass leaves behind an inventory of every artifact Gradle downloaded, with a sha256 for each, so check all three packaged repositories against it. Two wrinkles. Downstream tasks are handed all of them, so an artifact only has to appear in one. And the inventory records the file name from the module metadata, which for Kotlin Multiplatform is not the name the repository publishes the file under, so where the recorded name isn't on disk we look for its checksum among the files that are. Whatever the caller names in --unproxied is expected to be absent, which for us is what the unproxied Mozilla repositories serve. Anything else absent is a fault, as is any packaged component with a file missing, which is the shape bug 1953671 describes. Failing on those is the point of this. Verified against fixtures for a complete tree, a Kotlin Multiplatform component laid out the way a repository lays it out, a component split across two repositories, and a plugin marker packaged under gradle-plugins; and that it fails on a missing file, a corrupt file, a component absent from outside the unproxied prefixes, and a missing or empty inventory.
c043061 to
9b7a63c
Compare
Merge Queue Status
This pull request spent 16 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
3 participants
The
android-gradle-dependenciestoolchain task currently builds the whole project just to populate the dependency cache. This stops it doing that, then closes the gaps that change exposed.The first two commits port bug 1977709 / D257608 from mozilla-central. The rest picks up later changes to the upstream scripts that we never followed, plus two that are specific to this repo.
ce3c15a3de9403b126d6d865d21af2555506c867f65cb2f49b7a63cdWhy one Gradle task still runs
mozilla-central gets away with running no Gradle tasks at all. We cannot. AGP resolves the aapt2 binary through a detached configuration while its tasks run, so the enumeration pass never sees it, and our downstream tasks are handed the packaged repository as their only
google(). That makes a missing aapt2 a hard resolution failure. mozilla-central is unaffected because its builds resolve fromGRADLE_MAVEN_REPOSITORIES, which normally points at the live remote repos.:app:processDebugResourcesis the cheapest task that pulls aapt2 down. I confirmed it is both necessary and sufficient by deleting aapt2 from the Gradle cache and re-running the committed command.Why plugin resolution had to move too
Project dependencies already resolved through the proxies. Plugins did not, which left the artifact incomplete in a way that cannot be told apart from the packaging having dropped a file.
There were two holes.
pluginManagementinsettings.gradledeclaredgoogle()andmavenCentral()outright, and Gradle contributes those to the buildscript classpath alongside the proxies, so resolution could take a component's pom through Nexus and its jar from the unproxied twin. buildSrc then resolves its own plugins separately again, from the plugin portal, which nothing pointed at Nexus.This matters for the last commit. Until both holes were closed, the completeness check could not distinguish a real fault from a file that legitimately came from elsewhere, and would have had to tolerate both.
nexus.xmlalready carried an unusedgradle-pluginsproxy for the portal, so this only had to point at it and package its storage.For reviewers
zstdis needed fortar -aand is priority optional on Ubuntu.zstandardis what run-task'sfetch-contentuses to unpack.tar.zst. Bothandroid-buildandui-testshaveparent: base, so one image covers producer and consumers..tar.zst. That name is part of the toolchain digest, so the toolchain regenerates and nothing reuses a cached.tar.xz.buildSrc/**joins the task'sresources. buildSrc's classpath is part of the artifact now, and*.gradledoes not match.kts.Testing
Green in CI, including every task that consumes the artifact:
build-debug,test-debug, the four lint tasks andsigning-debug.build-debuglogs__plugin_repository__MavenCentral, so downstream really does resolve plugins through the packaged repository.verified 1430 artifacts across 825 of 923 components; 98 are not packaged. All 98 areorg/mozilla, which is what the two unproxied Mozilla repositories account for. Anything else absent, or any packaged component missing a file, fails the task.Pull Request checklist