Repository navigation
[flutter_tools] Fix cache download progress calculation for non-downloading artifacts - #192090
Conversation
…oading artifacts `ArtifactSet.downloadCount` defaulted to 1, causing non-downloading artifact sets (such as `LegacyCanvasKitRemover`, `PubDependencies`, and `AndroidMavenArtifacts`) to increment the progress total in `Cache.updateAll`. When running `flutter precache --web`, this led to reporting `[1/3] Web SDK` and completing without ever reporting on items 2 and 3. This change: - Sets `ArtifactSet.downloadCount` to default to 0. - Overrides `downloadCount` to return 1 in `CachedArtifact`. - Updates `Cache.updateAll` to calculate `artifactTotal` and index based only on artifacts with `downloadCount > 0`. - Adds regression unit tests for `downloadCount` defaults and `Cache.updateAll` progress context. Fixes flutter#192023
There was a problem hiding this comment.
Code Review
This pull request updates the Cache class to allow overriding the ArtifactUpdater and refactors the updateAll method to calculate progress and display status only for artifacts that require downloading (where downloadCount > 0). It also changes the default downloadCount for ArtifactSet from 1 to 0, overriding it to 1 in CachedArtifact, and adds corresponding tests. The reviewer suggested a performance improvement in Cache.updateAll to avoid allocating an intermediate list with .toList() when calculating the total number of downloadable artifacts.
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm, one question about the logic for tallying the download jobs. I think it's practically no difference but the way the code and api look to me, it may be a sleeper bug
| final int total = artifactsToUpdate | ||
| .where((ArtifactSet artifact) => artifact.downloadCount > 0) | ||
| .length; |
There was a problem hiding this comment.
Should this be
artifactsToUpdate.map(x => x.downloadCount).reduce((x, y) => x + y)?
There was a problem hiding this comment.
No, total represents the number of downloadable ArtifactSets being updated rather than the total count of individual downloaded files across all sets.
The progress display uses a two-level hierarchy:
- At the top level,
artifactTotal(total) andartifactIndex(current) track theArtifactSets being processed (e.g.,[1/3] Android SDK dependenciesor[2/3] Material fonts). - Within an
ArtifactSetthat contains multiple downloads (artifact.downloadCount > 1),downloadTotal(artifact.downloadCount) tracks the individual downloads under that set (e.g.,├─ [1/4] ...).
If we used the sum of all download counts for total, current (which increments once per ArtifactSet) would be comparing the set index against the total file download count (e.g., reaching only [3/10] when finishing the 3rd and final artifact set).
Description
Fixes an issue where
flutter precachereported an incorrect artifact progress count (e.g.[1/3] Web SDK) and terminated without downloading items 2 or 3.Root Cause
ArtifactSet.downloadCountdefaulted to 1, causing non-downloading maintenance artifacts (LegacyCanvasKitRemover,PubDependencies,AndroidMavenArtifacts) to increment the progress total inCache.updateAll.flutter precache --web,artifactsToUpdatecontainedFlutterWebSdk(downloadable) along withLegacyCanvasKitRemoverandPubDependencies(non-downloadable cleanup/resolution tasks), resulting intotal = 3.FlutterWebSdkwas tracked as[1/3], and the remaining non-downloading artifacts ran without invokingArtifactUpdater, leaving the impression of incomplete progress.Changes
ArtifactSet.downloadCountto 0.CachedArtifact.downloadCountto return 1.Cache.updateAll, computetotaland progress index based only on artifacts withdownloadCount > 0.artifactUpdaterconstructor parameter toCacheandCache.testfor dependency injection.cache_test.dartverifyingdownloadCountdefaults and progress calculation.Issues Fixed
Fixes #192023
Pre-launch Checklist
///).