Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(shorebird_cli): use aot-tools.dill for linking if available #1596

Merged
merged 7 commits into from
Jan 2, 2024
Merged

fix(shorebird_cli): use aot-tools.dill for linking if available #1596

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
880a683
fix(shorebird_cli): use aot-tools.dill for linking if available
bryanoltman Dec 21, 2023
575f127
update comment
bryanoltman Dec 21, 2023
1851bf3
fix tests
bryanoltman Dec 21, 2023
bb268bd
update aot-tools cache artifact to download a shared kernel file
bryanoltman Jan 2, 2024
183384a
add isExecutable to CachedArtifact
bryanoltman Jan 2, 2024
9a5e299
cleanup
bryanoltman Jan 2, 2024
adf32e2
update comment
bryanoltman Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions packages/shorebird_cli/lib/src/cache.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,15 @@ abstract class CachedArtifact {

String get storageUrl;

List<String> get executables => [];
String get fileName;

bool get isExecutable;

bool get required => true;

Future<void> extractArtifact(http.ByteStream stream, String outputPath) {
final file = File(p.join(outputPath, name))..createSync(recursive: true);
final file = File(p.join(outputPath, fileName))
..createSync(recursive: true);
return stream.pipe(file.openWrite());
}

Expand Down Expand Up @@ -179,12 +182,10 @@ allowed to access $storageUrl.''',

await extractArtifact(response.stream, location.path);

if (platform.isWindows) return;

for (final executable in executables) {
if (!platform.isWindows && isExecutable) {
final result = await process.start(
'chmod',
['+x', p.join(location.path, executable)],
['+x', p.join(location.path, fileName)],
);
await result.exitCode;
}
Expand All @@ -197,8 +198,13 @@ class AotToolsArtifact extends CachedArtifact {
@override
String get name => 'aot-tools';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this to executableName to avoid confusion? I don't think it's obvious what the difference between name and fileName is at first glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add some comments. I changed executables to executable (because our artifacts only ever have one file) to fileName (because our artifacts aren't always executable).


// Not technically an executable, but this is where the file should end up and
// is how it will be consumed.
@override
List<String> get executables => ['aot-tools'];
String get fileName => 'aot-tools.dill';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why couldn't we change name to be aot-tools.dill and avoid introducing a new fileName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the artifact path is [name]/[fileName] (or, in the case of aot-tools, [name]/[flutterRev]/[fileName]). We could use fileName for both, but this felt cleaner.


@override
bool get isExecutable => false;

/// The aot-tools are only available for revisions that support mixed-mode.
@override
Expand All @@ -213,18 +219,8 @@ class AotToolsArtifact extends CachedArtifact {
);

@override
String get storageUrl {
var artifactName = 'aot-tools-';
if (platform.isMacOS) {
artifactName += 'darwin-x64';
} else if (platform.isLinux) {
artifactName += 'linux-x64';
} else if (platform.isWindows) {
artifactName += 'windows-x64';
}

return '${cache.storageBaseUrl}/${cache.storageBucket}/shorebird/${shorebirdEnv.shorebirdEngineRevision}/$artifactName';
}
String get storageUrl =>
'${cache.storageBaseUrl}/${cache.storageBucket}/shorebird/${shorebirdEnv.shorebirdEngineRevision}/aot-tools.dill';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use fileName instead of the string literal aot-tools.dill?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could ¯_(ツ)_/¯

}

class PatchArtifact extends CachedArtifact {
Expand All @@ -234,7 +230,10 @@ class PatchArtifact extends CachedArtifact {
String get name => 'patch';

@override
List<String> get executables => ['patch'];
String get fileName => 'patch';

@override
bool get isExecutable => true;

@override
Future<void> extractArtifact(
Expand Down Expand Up @@ -268,6 +267,12 @@ class BundleToolArtifact extends CachedArtifact {
@override
String get name => 'bundletool.jar';

@override
String get fileName => 'bundletool.jar';

@override
bool get isExecutable => false;

@override
String get storageUrl {
return 'https://github.com/google/bundletool/releases/download/1.15.6/bundletool-all-1.15.6.jar';
Expand Down
3 changes: 2 additions & 1 deletion packages/shorebird_cli/lib/src/executables/aot_tools.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ class AotTools {

// This enables us to run Dart scripts directly, as is needed when running
// with a local engine.
if (p.extension(executable) == '.dart') {
final ext = p.extension(executable);
if (ext == '.dart' || ext == '.dill') {
return process.run(
shorebirdEnv.dartBinaryFile.path,
[executable, ...command],
Expand Down
15 changes: 14 additions & 1 deletion packages/shorebird_cli/lib/src/shorebird_artifacts.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ enum ShorebirdArtifact {
/// The analyze_snapshot executable.
analyzeSnapshot,

/// The aot_tools executable.
/// The aot_tools executable or kernel file.
aotTools,

/// The gen_snapshot executable.
Expand Down Expand Up @@ -73,6 +73,19 @@ class ShorebirdCachedArtifacts implements ShorebirdArtifacts {

File get _aotToolsFile {
const executableName = 'aot-tools';
final kernelFile = File(
p.join(
cache.getArtifactDirectory(executableName).path,
shorebirdEnv.shorebirdEngineRevision,
'$executableName.dill',
),
);
if (kernelFile.existsSync()) {
return kernelFile;
}

// We shipped aot-tools as an executable in the past, so we return that if
// no kernel file is found.
return File(
p.join(
cache.getArtifactDirectory(executableName).path,
Expand Down
20 changes: 7 additions & 13 deletions packages/shorebird_cli/test/src/cache_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ class TestCachedArtifact extends CachedArtifact {
@override
String get name => 'test';

@override
String get fileName => 'test';

@override
bool get isExecutable => true;

@override
String get storageUrl => 'test-url';
}
Expand Down Expand Up @@ -142,18 +148,6 @@ void main() {
});
});

group('CachedArtifact', () {
late CachedArtifact artifact;

setUp(() {
artifact = TestCachedArtifact(cache: cache, platform: platform);
});

test('has empty executables by default', () {
expect(artifact.executables, isEmpty);
});
});

group('clear', () {
test('deletes the cache directory', () async {
final shorebirdCacheDirectory = runWithOverrides(
Expand Down Expand Up @@ -217,7 +211,7 @@ void main() {
(invocation) async {
final request =
invocation.positionalArguments.first as http.BaseRequest;
if (request.url.path.endsWith('aot-tools-darwin-x64')) {
if (request.url.path.endsWith('aot-tools.dill')) {
return http.StreamedResponse(
const Stream.empty(),
HttpStatus.notFound,
Expand Down
3 changes: 2 additions & 1 deletion packages/shorebird_cli/test/src/commands/patch/patch_ios_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,8 @@ Please re-run the release command for this version or create a new release.'''),
// Verify that we switch back to the original revision once we're done.
verifyInOrder([
() => shorebirdFlutter.useRevision(
revision: preLinkerRelease.flutterRevision),
revision: preLinkerRelease.flutterRevision,
),
() => shorebirdFlutter.useRevision(revision: otherRevision),
]);

Expand Down
58 changes: 55 additions & 3 deletions packages/shorebird_cli/test/src/executables/aot_tools_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void main() {
).thenReturn(aotToolsPath);
});

test('completes when linking exits with code 0', () async {
test('links and exits with code 0', () async {
when(
() => process.run(
any(),
Expand Down Expand Up @@ -139,7 +139,59 @@ void main() {
});
});

group('when using local aot_tools', () {
group('when aot-tools is a kernel file', () {
const aotToolsPath = 'aot_tools.dill';

setUp(() {
when(
() => shorebirdArtifacts.getArtifactPath(
artifact: ShorebirdArtifact.aotTools,
),
).thenReturn(aotToolsPath);
});

test('links and exits with code 0', () async {
when(
() => process.run(
any(),
any(),
workingDirectory: any(named: 'workingDirectory'),
),
).thenAnswer(
(_) async => const ShorebirdProcessResult(
exitCode: 0,
stdout: '',
stderr: '',
),
);
await expectLater(
runWithOverrides(
() => aotTools.link(
base: base,
patch: patch,
analyzeSnapshot: analyzeSnapshot,
workingDirectory: workingDirectory.path,
),
),
completes,
);
verify(
() => process.run(
dartBinaryFile.path,
[
aotToolsPath,
'link',
'--base=$base',
'--patch=$patch',
'--analyze-snapshot=$analyzeSnapshot',
],
workingDirectory: any(named: 'workingDirectory'),
),
).called(1);
});
});

group('when aot_tools is a dart file', () {
const aotToolsPath = 'aot_tools.dart';

setUp(() {
Expand All @@ -150,7 +202,7 @@ void main() {
).thenReturn(aotToolsPath);
});

test('completes when linking exits with code 0', () async {
test('links and exits with code 0', () async {
when(
() => process.run(
any(),
Expand Down
74 changes: 60 additions & 14 deletions packages/shorebird_cli/test/src/shorebird_artifacts_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'mocks.dart';

void main() {
group(ShorebirdCachedArtifacts, () {
const engineRevision = 'engine-revision';
late Cache cache;
late Directory flutterDirectory;
late Directory artifactDirectory;
Expand All @@ -31,8 +32,11 @@ void main() {

setUp(() {
cache = MockCache();
flutterDirectory = Directory('flutter');
artifactDirectory = Directory('artifacts');
final tmpDir = Directory.systemTemp.createTempSync();
flutterDirectory = Directory(p.join(tmpDir.path, 'flutter'))
..createSync(recursive: true);
artifactDirectory = Directory(p.join(tmpDir.path, 'artifacts'))
..createSync(recursive: true);
shorebirdEnv = MockShorebirdEnv();
artifacts = const ShorebirdCachedArtifacts();

Expand All @@ -41,21 +45,63 @@ void main() {
).thenReturn(artifactDirectory);
when(() => shorebirdEnv.flutterDirectory).thenReturn(flutterDirectory);
when(() => shorebirdEnv.shorebirdEngineRevision)
.thenReturn('engine-revision');
.thenReturn(engineRevision);
});

group('getArtifactPath', () {
test('returns correct path for aot tools', () {
expect(
runWithOverrides(
() => artifacts.getArtifactPath(
artifact: ShorebirdArtifact.aotTools,
),
),
equals(
p.join(artifactDirectory.path, 'engine-revision', 'aot-tools'),
),
);
group('aot-tools', () {
const aotToolsKernel = 'aot-tools.dill';
const aotToolsExe = 'aot-tools';
late String aotToolsKernelPath;
late String aotToolsExePath;

setUp(() {
aotToolsKernelPath = p.join(
artifactDirectory.path,
engineRevision,
aotToolsKernel,
);
aotToolsExePath = p.join(
artifactDirectory.path,
engineRevision,
aotToolsExe,
);
});

group('when kernel and executable are present', () {
setUp(() {
File(aotToolsKernelPath).createSync(recursive: true);
File(aotToolsExePath).createSync(recursive: true);
});

test('returns path to kernel file', () async {
expect(
runWithOverrides(
() => artifacts.getArtifactPath(
artifact: ShorebirdArtifact.aotTools,
),
),
equals(aotToolsKernelPath),
);
});
});

group('when only executable is present', () {
setUp(() {
File(aotToolsExePath).createSync(recursive: true);
});

test('returns path to executable file', () {
expect(
runWithOverrides(
() => artifacts.getArtifactPath(
artifact: ShorebirdArtifact.aotTools,
),
),
equals(aotToolsExePath),
);
});
});
});

test('returns correct path for gen_snapshot', () {
Expand Down