fix(shorebird_cli): use aot-tools.dill for linking if available #1596
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1596 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 155 155
Lines 4828 4829 +1
=========================================
+ Hits 4828 4829 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -198,7 +199,10 @@ class AotToolsArtifact extends CachedArtifact { | |||
| String get name => 'aot-tools'; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| @@ -198,7 +199,10 @@ class AotToolsArtifact extends CachedArtifact { | |||
| String get name => 'aot-tools'; | |||
|
|
|||
| @override | |||
| List<String> get executables => ['aot-tools']; | |||
| String get fileName => 'aot-tools.dill'; | |||
There was a problem hiding this comment.
Why couldn't we change name to be aot-tools.dill and avoid introducing a new fileName?
There was a problem hiding this comment.
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.
| return '${cache.storageBaseUrl}/${cache.storageBucket}/shorebird/${shorebirdEnv.shorebirdEngineRevision}/$artifactName'; | ||
| } | ||
| String get storageUrl => | ||
| '${cache.storageBaseUrl}/${cache.storageBucket}/shorebird/${shorebirdEnv.shorebirdEngineRevision}/aot-tools.dill'; |
There was a problem hiding this comment.
Could this use fileName instead of the string literal aot-tools.dill?
There was a problem hiding this comment.
We could ¯_(ツ)_/¯
Description
Because the precompiled
aot-toolsexecutable will only work on architectures that match thedartexecutable used to build it, and because we build dart on arm64 macs, theaot-toolsexecutable does not work on x64 macs. This change looks for an aot-tools kernel file (.dill) and uses thedartpackaged with shorebird's flutter (which was used to produce the kernel file) to run it.Fixes #1595
Related to but not dependent on https://github.com/shorebirdtech/_build_engine/pull/56
Type of Change