fix(shorebird_cli): update cache artifacts on every command run, log progress (#1958)

Co-authored-by: Eric Seidel <eric@shorebird.dev>

packages/shorebird_cli/lib/src/cache.dart

@@ -1,6 +1,7 @@
 import 'dart:io' hide Platform;
 
 import 'package:http/http.dart' as http;
+import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 import 'package:platform/platform.dart';
 import 'package:scoped/scoped.dart';
@@ -49,15 +50,17 @@ class Cache {
     registerArtifact(AotToolsExeArtifact(cache: this, platform: platform));
   }
 
-  void registerArtifact(CachedArtifact artifact) => _artifacts.add(artifact);
+  void registerArtifact(CachedArtifact artifact) => artifacts.add(artifact);
 
   Future<void> updateAll() async {
-    for (final artifact in _artifacts) {
+    for (final artifact in artifacts) {
       if (await artifact.isUpToDate()) {
         continue;
       }
 
+      final progress = logger.progress('Downloading ${artifact.name} artifact');
       await artifact.update();
+      progress.complete('Downloaded ${artifact.name} artifact');
     }
   }
 
@@ -98,7 +101,8 @@ class Cache {
     );
   }
 
-  final List<CachedArtifact> _artifacts = [];
+  @visibleForTesting
+  final List<CachedArtifact> artifacts = [];
 
   String get storageBaseUrl => 'https://storage.googleapis.com';
 
@@ -207,6 +211,14 @@ class AotToolsDillArtifact extends CachedArtifact {
 
 /// For a few revisions in Dec 2023, we distributed aot-tools as an executable.
 /// Should be removed sometime after June 2024.
+///
+/// The change to use a .dill file was made in
+/// https://github.com/shorebirdtech/_build_engine/commit/babbc37d93e7a2f36e62787e47eee5a3b5458901
+/// The Flutter versions that use this are:
+///  - 3.13.9 (a3d5f7c614aa1cc4d6cb1506e74fd1c81678e68e)
+///  - 3.16.3 (b9b23902966504a9778f4c07e3a3487fa84dcb2a)
+///  - 3.16.4 (7e92b034c5dddb727cf5e802c23cddd39b325a7f)
+///  - 3.16.5 (4e8a7c746ae6f10951f3e676f10b82b21d7300a5)
 class AotToolsExeArtifact extends CachedArtifact {
   AotToolsExeArtifact({required super.cache, required super.platform});
 

packages/shorebird_cli/lib/src/command_runner.dart

@@ -5,6 +5,7 @@ import 'package:args/command_runner.dart';
 import 'package:cli_completion/cli_completion.dart';
 import 'package:mason_logger/mason_logger.dart';
 import 'package:scoped/scoped.dart';
+import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
@@ -206,6 +207,13 @@ Run ${lightCyan.wrap('shorebird upgrade')} to upgrade.''');
       exitCode = ExitCode.success.code;
     } else {
       try {
+        // Most commands need shorebird artifacts, so we ensure the cache
+        // is up to date for all commands (even ones which don't need it).  We
+        // could make a Command subclass and move the cache onto that and
+        // only update in that case, but it didn't seem worth it at the time.
+        // Ensure all cached artifacts are up-to-date before running the
+        // command.
+        await cache.updateAll();
         exitCode = await super.runCommand(topLevelResults);
       } catch (error, stackTrace) {
         logger

packages/shorebird_cli/lib/src/commands/patch/patch_aar_command.dart

@@ -9,7 +9,6 @@ import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/archive_analysis/archive_analysis.dart';
 import 'package:shorebird_cli/src/artifact_manager.dart';
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/command.dart';
 import 'package:shorebird_cli/src/commands/patch/patch.dart';
@@ -99,8 +98,6 @@ of the Android app that is using this module.''',
     final allowAssetDiffs = results['allow-asset-diffs'] == true;
     final allowNativeDiffs = results['allow-native-diffs'] == true;
 
-    await cache.updateAll();
-
     if (shorebirdEnv.androidPackageName == null) {
       logger.err('Could not find androidPackage in pubspec.yaml.');
       return ExitCode.config.code;

packages/shorebird_cli/lib/src/commands/patch/patch_android_command.dart

@@ -7,7 +7,6 @@ import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/archive_analysis/archive_analysis.dart';
 import 'package:shorebird_cli/src/archive_analysis/archive_differ.dart';
 import 'package:shorebird_cli/src/artifact_manager.dart';
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/command.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
@@ -108,8 +107,6 @@ If this option is not provided, the version number will be determined from the p
     final dryRun = results['dry-run'] == true;
     final isStaging = results['staging'] == true;
 
-    await cache.updateAll();
-
     const releasePlatform = ReleasePlatform.android;
     final flavor = results.findOption('flavor', argParser: argParser);
     final target = results.findOption('target', argParser: argParser);

packages/shorebird_cli/lib/src/executables/aot_tools.dart

@@ -6,7 +6,6 @@ import 'package:mason_logger/mason_logger.dart';
 import 'package:path/path.dart' as p;
 import 'package:pub_semver/pub_semver.dart';
 import 'package:scoped/scoped.dart';
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/extensions/version.dart';
 import 'package:shorebird_cli/src/shorebird_artifacts.dart';
@@ -86,8 +85,6 @@ class AotTools {
     List<String> command, {
     String? workingDirectory,
   }) async {
-    await cache.updateAll();
-
     // This will be a path to either a kernel (.dill) file or a Dart script if
     // we're running with a local engine.
     final artifactPath = shorebirdArtifacts.getArtifactPath(

packages/shorebird_cli/lib/src/executables/bundletool.dart

@@ -16,7 +16,6 @@ class Bundletool {
   static const jar = 'bundletool.jar';
 
   Future<ShorebirdProcessResult> _exec(List<String> command) async {
-    await cache.updateAll();
     final bundletool = p.join(cache.getArtifactDirectory(jar).path, jar);
     final javaHome = java.home;
     final javaExecutable = java.executable ?? 'java';

packages/shorebird_cli/lib/src/shorebird_release_version_mixin.dart

@@ -1,4 +1,3 @@
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/command.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 
@@ -9,8 +8,6 @@ mixin ShorebirdReleaseVersionMixin on ShorebirdCommand {
   Future<String> extractReleaseVersionFromAppBundle(
     String appBundlePath,
   ) async {
-    await cache.updateAll();
-
     final results = await Future.wait([
       bundletool.getVersionName(appBundlePath),
       bundletool.getVersionCode(appBundlePath),

packages/shorebird_cli/test/src/cache_test.dart

@@ -28,6 +28,7 @@ void main() {
     late http.Client httpClient;
     late Logger logger;
     late Platform platform;
+    late Progress progress;
     late Process chmodProcess;
     late ShorebirdEnv shorebirdEnv;
     late ShorebirdProcess shorebirdProcess;
@@ -72,6 +73,7 @@ void main() {
       httpClient = MockHttpClient();
       logger = MockLogger();
       platform = MockPlatform();
+      progress = MockProgress();
       chmodProcess = MockProcess();
       shorebirdEnv = MockShorebirdEnv();
       shorebirdProcess = MockShorebirdProcess();
@@ -86,6 +88,7 @@ void main() {
         (invocation.namedArguments[#outputDirectory] as Directory)
             .createSync(recursive: true);
       });
+      when(() => logger.progress(any())).thenReturn(progress);
       when(
         () => shorebirdEnv.shorebirdEngineRevision,
       ).thenReturn(shorebirdEngineRevision);
@@ -179,6 +182,41 @@ void main() {
     });
 
     group('updateAll', () {
+      group('progress', () {
+        group('when all artifacts are up-to-date', () {
+          setUp(() {
+            runWithOverrides(() {
+              for (final artifact in cache.artifacts) {
+                artifact.location.createSync(recursive: true);
+              }
+            });
+          });
+
+          test('does not print progress', () async {
+            await runWithOverrides(cache.updateAll);
+            verifyNever(() => logger.progress(any()));
+          });
+        });
+
+        group('when artifacts require updates', () {
+          test('prints progress', () async {
+            await runWithOverrides(cache.updateAll);
+
+            // We expect one fewer call to progress.complete than the number of
+            // artifacts. This is because [AotToolsDillArtifact] and
+            // [AotToolsExeArtifact] share a location. These artifacts will
+            // never both be present for the same engine revision. We
+            // temporarily used an precompiled executable for aot-tools before
+            // switching to a .dill file. This can be removed once we no longer
+            // need to support the precompiled executable case. See the
+            // [AotToolsExeArtifact] definition for more details.
+            final expectedUpdateCount = cache.artifacts.length - 1;
+            verify(() => logger.progress(any())).called(expectedUpdateCount);
+            verify(() => progress.complete(any())).called(expectedUpdateCount);
+          });
+        });
+      });
+
       group('patch', () {
         test('throws CacheUpdateFailure if a SocketException is thrown',
             () async {

packages/shorebird_cli/test/src/command_runner_test.dart

@@ -3,6 +3,7 @@ import 'package:mason_logger/mason_logger.dart';
 import 'package:mocktail/mocktail.dart';
 import 'package:platform/platform.dart';
 import 'package:scoped/scoped.dart';
+import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/command_runner.dart';
 import 'package:shorebird_cli/src/logger.dart' hide logger;
 import 'package:shorebird_cli/src/platform.dart';
@@ -20,6 +21,7 @@ void main() {
     const flutterRevision = 'test-flutter-revision';
     const flutterVersion = '1.2.3';
 
+    late Cache cache;
     late Logger logger;
     late Platform platform;
     late ShorebirdEnv shorebirdEnv;
@@ -31,6 +33,7 @@ void main() {
       return runScoped(
         body,
         values: {
+          cacheRef.overrideWith(() => cache),
           loggerRef.overrideWith(() => logger),
           platformRef.overrideWith(() => platform),
           shorebirdEnvRef.overrideWith(() => shorebirdEnv),
@@ -41,11 +44,13 @@ void main() {
     }
 
     setUp(() {
+      cache = MockCache();
       logger = MockLogger();
       platform = MockPlatform();
       shorebirdEnv = MockShorebirdEnv();
       shorebirdFlutter = MockShorebirdFlutter();
       shorebirdVersion = MockShorebirdVersion();
+      when(() => cache.updateAll()).thenAnswer((_) async {});
       when(() => logger.level).thenReturn(Level.info);
       when(
         () => shorebirdEnv.shorebirdEngineRevision,
@@ -272,6 +277,16 @@ Run ${lightCyan.wrap('shorebird upgrade')} to upgrade.'''),
     });
 
     group('on command failure', () {
+      test('updates cache artifacts', () async {
+        // This will fail due to the release android command missing scoped
+        // dependencies. Because the cache update should happen before the
+        // command runs, we verify that behavior here.
+        await runWithOverrides(
+          () => commandRunner.run(['release', 'android']),
+        );
+        verify(() => cache.updateAll()).called(1);
+      });
+
       test('logs a stack trace using detail', () async {
         // This will fail due to the release android command missing scoped
         // dependencies.

packages/shorebird_cli/test/src/commands/patch/patch_aar_command_test.dart

@@ -10,7 +10,6 @@ import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/archive_analysis/archive_analysis.dart';
 import 'package:shorebird_cli/src/artifact_manager.dart';
 import 'package:shorebird_cli/src/auth/auth.dart';
-import 'package:shorebird_cli/src/cache.dart' show Cache, cacheRef;
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/config/config.dart';
@@ -102,7 +101,6 @@ void main() {
     late ShorebirdProcessResult flutterBuildProcessResult;
     late ShorebirdProcessResult flutterPubGetProcessResult;
     late http.Client httpClient;
-    late Cache cache;
     late ShorebirdEnv shorebirdEnv;
     late ShorebirdFlutter shorebirdFlutter;
     late ShorebirdProcess shorebirdProcess;
@@ -115,7 +113,6 @@ void main() {
         values: {
           artifactManagerRef.overrideWith(() => artifactManager),
           authRef.overrideWith(() => auth),
-          cacheRef.overrideWith(() => cache),
           codePushClientWrapperRef.overrideWith(() => codePushClientWrapper),
           engineConfigRef.overrideWith(() => const EngineConfig.empty()),
           httpClientRef.overrideWith(() => httpClient),
@@ -190,7 +187,6 @@ void main() {
       flutterBuildProcessResult = MockProcessResult();
       flutterPubGetProcessResult = MockProcessResult();
       httpClient = MockHttpClient();
-      cache = MockCache();
       shorebirdEnv = MockShorebirdEnv();
       shorebirdFlutter = MockShorebirdFlutter();
       shorebirdProcess = MockShorebirdProcess();
@@ -318,10 +314,6 @@ void main() {
           metadata: any(named: 'metadata'),
         ),
       ).thenAnswer((_) async {});
-      when(() => cache.updateAll()).thenAnswer((_) async => {});
-      when(
-        () => cache.getArtifactDirectory(any()),
-      ).thenReturn(Directory.systemTemp.createTempSync());
       when(
         () => shorebirdFlutter.getVersionAndRevision(),
       ).thenAnswer((_) async => flutterVersionAndRevision);

packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart

@@ -8,7 +8,6 @@ import 'package:path/path.dart' as p;
 import 'package:platform/platform.dart';
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/auth/auth.dart';
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/config/config.dart';
@@ -73,7 +72,6 @@ void main() {
     late Doctor doctor;
     late Platform platform;
     late Auth auth;
-    late Cache cache;
     late Java java;
     late Logger logger;
     late OperatingSystemInterface operatingSystemInterface;
@@ -93,7 +91,6 @@ void main() {
         values: {
           authRef.overrideWith(() => auth),
           bundletoolRef.overrideWith(() => bundletool),
-          cacheRef.overrideWith(() => cache),
           codePushClientWrapperRef.overrideWith(() => codePushClientWrapper),
           doctorRef.overrideWith(() => doctor),
           engineConfigRef.overrideWith(() => const EngineConfig.empty()),
@@ -127,7 +124,6 @@ void main() {
       shorebirdRoot = Directory.systemTemp.createTempSync();
       projectRoot = Directory.systemTemp.createTempSync();
       auth = MockAuth();
-      cache = MockCache();
       java = MockJava();
       progress = MockProgress();
       logger = MockLogger();
@@ -191,10 +187,6 @@ void main() {
       when(() => argResults.wasParsed(any())).thenReturn(true);
       when(() => auth.isAuthenticated).thenReturn(true);
       when(() => auth.client).thenReturn(httpClient);
-      when(() => cache.updateAll()).thenAnswer((_) async => {});
-      when(
-        () => cache.getArtifactDirectory(any()),
-      ).thenReturn(Directory.systemTemp.createTempSync());
       when(() => logger.progress(any())).thenReturn(progress);
       when(() => logger.confirm(any())).thenReturn(true);
       when(

packages/shorebird_cli/test/src/executables/aot_tools_test.dart

@@ -4,7 +4,6 @@ import 'package:mason_logger/mason_logger.dart';
 import 'package:mocktail/mocktail.dart';
 import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
-import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/shorebird_artifacts.dart';
 import 'package:shorebird_cli/src/shorebird_env.dart';
@@ -15,7 +14,6 @@ import '../mocks.dart';
 
 void main() {
   group(AotTools, () {
-    late Cache cache;
     late ShorebirdArtifacts shorebirdArtifacts;
     late ShorebirdProcess process;
     late ShorebirdEnv shorebirdEnv;
@@ -27,7 +25,6 @@ void main() {
       return runScoped(
         body,
         values: {
-          cacheRef.overrideWith(() => cache),
           processRef.overrideWith(() => process),
           shorebirdArtifactsRef.overrideWith(() => shorebirdArtifacts),
           shorebirdEnvRef.overrideWith(() => shorebirdEnv),
@@ -36,7 +33,6 @@ void main() {
     }
 
     setUp(() {
-      cache = MockCache();
       process = MockShorebirdProcess();
       shorebirdArtifacts = MockShorebirdArtifacts();
       shorebirdEnv = MockShorebirdEnv();
@@ -44,7 +40,6 @@ void main() {
       workingDirectory = Directory('aot-tools test');
       aotTools = AotTools();
 
-      when(() => cache.updateAll()).thenAnswer((_) async {});
       when(() => shorebirdEnv.dartBinaryFile).thenReturn(dartBinaryFile);
       when(
         () => shorebirdArtifacts.getArtifactPath(

packages/shorebird_cli/test/src/executables/bundletool_test.dart

@@ -45,7 +45,6 @@ void main() {
       bundletool = Bundletool();
 
       when(() => androidSdk.path).thenReturn(androidSdkPath);
-      when(() => cache.updateAll()).thenAnswer((_) async {});
       when(
         () => cache.getArtifactDirectory(any()),
       ).thenReturn(workingDirectory);

third_party/flutter/bin/internal/shared.sh

@@ -164,8 +164,12 @@ function upgrade_shorebird () (
       mv "$SNAPSHOT_PATH" "$SNAPSHOT_PATH_OLD"
     fi
 
-    # Compile...
-    $DART_PATH --verbosity=error --disable-dart-dev --snapshot="$SNAPSHOT_PATH" --snapshot-kind="app-jit" --packages="$SHOREBIRD_CLI_DIR/.dart_tool/package_config.json" --no-enable-mirrors "$SCRIPT_PATH" > /dev/null
+    # Compile our snapshot.
+    # We invoke `$SNAPSHOT_PATH completion` to trigger the "completion" command, which
+    # avoids executing as much of our code as possible. We do this because running
+    # the script here (instead of from the compiled snapshot) invalidates a lot of
+    # assumptions we make about the cwd in the shorebird_cli tool.
+    $DART_PATH --verbosity=error --disable-dart-dev --snapshot="$SNAPSHOT_PATH" --snapshot-kind="app-jit" --packages="$SHOREBIRD_CLI_DIR/.dart_tool/package_config.json" --no-enable-mirrors "$SCRIPT_PATH" completion > /dev/null
     echo "$compilekey" > "$STAMP_PATH"
 
     # Delete any temporary snapshot path.