Skip to content

Commit

Permalink
fix(shorebird_cli): update cache artifacts on every command run, log …
Browse files Browse the repository at this point in the history
…progress (#1958)

Co-authored-by: Eric Seidel <eric@shorebird.dev>
  • Loading branch information
bryanoltman and eseidel authored Apr 23, 2024
1 parent 7652dc6 commit 764142e
Show file tree
Hide file tree
Showing 14 changed files with 82 additions and 40 deletions.
18 changes: 15 additions & 3 deletions packages/shorebird_cli/lib/src/cache.dart
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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');
}
}

Expand Down Expand Up @@ -98,7 +101,8 @@ class Cache {
);
}

final List<CachedArtifact> _artifacts = [];
@visibleForTesting
final List<CachedArtifact> artifacts = [];

String get storageBaseUrl => 'https://storage.googleapis.com';

Expand Down Expand Up @@ -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});

Expand Down
8 changes: 8 additions & 0 deletions packages/shorebird_cli/lib/src/command_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 0 additions & 3 deletions packages/shorebird_cli/lib/src/executables/aot_tools.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(
Expand Down
1 change: 0 additions & 1 deletion packages/shorebird_cli/lib/src/executables/bundletool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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),
Expand Down
38 changes: 38 additions & 0 deletions packages/shorebird_cli/test/src/cache_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -72,6 +73,7 @@ void main() {
httpClient = MockHttpClient();
logger = MockLogger();
platform = MockPlatform();
progress = MockProgress();
chmodProcess = MockProcess();
shorebirdEnv = MockShorebirdEnv();
shorebirdProcess = MockShorebirdProcess();
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 15 additions & 0 deletions packages/shorebird_cli/test/src/command_runner_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand All @@ -31,6 +33,7 @@ void main() {
return runScoped(
body,
values: {
cacheRef.overrideWith(() => cache),
loggerRef.overrideWith(() => logger),
platformRef.overrideWith(() => platform),
shorebirdEnvRef.overrideWith(() => shorebirdEnv),
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand All @@ -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),
Expand Down Expand Up @@ -190,7 +187,6 @@ void main() {
flutterBuildProcessResult = MockProcessResult();
flutterPubGetProcessResult = MockProcessResult();
httpClient = MockHttpClient();
cache = MockCache();
shorebirdEnv = MockShorebirdEnv();
shorebirdFlutter = MockShorebirdFlutter();
shorebirdProcess = MockShorebirdProcess();
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand All @@ -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()),
Expand Down Expand Up @@ -127,7 +124,6 @@ void main() {
shorebirdRoot = Directory.systemTemp.createTempSync();
projectRoot = Directory.systemTemp.createTempSync();
auth = MockAuth();
cache = MockCache();
java = MockJava();
progress = MockProgress();
logger = MockLogger();
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -15,7 +14,6 @@ import '../mocks.dart';

void main() {
group(AotTools, () {
late Cache cache;
late ShorebirdArtifacts shorebirdArtifacts;
late ShorebirdProcess process;
late ShorebirdEnv shorebirdEnv;
Expand All @@ -27,7 +25,6 @@ void main() {
return runScoped(
body,
values: {
cacheRef.overrideWith(() => cache),
processRef.overrideWith(() => process),
shorebirdArtifactsRef.overrideWith(() => shorebirdArtifacts),
shorebirdEnvRef.overrideWith(() => shorebirdEnv),
Expand All @@ -36,15 +33,13 @@ void main() {
}

setUp(() {
cache = MockCache();
process = MockShorebirdProcess();
shorebirdArtifacts = MockShorebirdArtifacts();
shorebirdEnv = MockShorebirdEnv();
dartBinaryFile = File('dart');
workingDirectory = Directory('aot-tools test');
aotTools = AotTools();

when(() => cache.updateAll()).thenAnswer((_) async {});
when(() => shorebirdEnv.dartBinaryFile).thenReturn(dartBinaryFile);
when(
() => shorebirdArtifacts.getArtifactPath(
Expand Down
Loading

0 comments on commit 764142e

Please sign in to comment.