Add usage event for failed iOS project migration #51879
Conversation
jmagman
added
the
tool
jmagman
changed the title
| @@ -95,6 +95,7 @@ Future<XcodeBuildResult> buildXcodeProject({ | |||
|
|
|||
| for (final IOSMigrator migrator in migrators) { | |||
| if (!migrator.migrate()) { | |||
| UsageEvent('ios-migration', '${migrator.usageEventAction}-failure').send(); | |||
There was a problem hiding this comment.
I think this would be a bit easier to navigate in GA as:
UsageEvent('ios-migration', migrator.usageEventAction, label: 'failure').send();
|
|
||
| /// iOS project is generated from a template on Flutter project creation. | ||
| /// Sometimes (due to behavior changes in Xcode, CocoaPods, etc) these files need to be altered | ||
| /// from the original template. | ||
| class IOSMigrator { | ||
| IOSMigrator(this.logger); | ||
| abstract class IOSMigrator { |
There was a problem hiding this comment.
Why does processFileLines need to take a callback? Could it call an abstract method provided by the subclasses?
There was a problem hiding this comment.
The migration can handle multiple files with different pattern matching per file:
void run() {
_migrateXcodeProjectInfoFile();
_migratePodfile();
}
void _migrateXcodeProjectInfoFile() {
_processFileLines(_xcodeProjectInfoFile, (String line) {
if (line.contains('3B80C3941E831B6300D905FE')) {
return null;
}
...
});
}
void _migratePodfile() {
_processFileLines(_podfile, (String line) {
if (line.contains('Prevent Cocoapods from embedding a second Flutter framework and causing an error with the new Xcode build system.')) {
return null;
}
...
});
}Original version of RemoveFrameworkLinkAndEmbeddingMigration (Podfile migration since deleted):
https://github.com/jmagman/flutter/blob/e70ed529c41b1dbfe6392c45f84b6cfb165fb419/packages/flutter_tools/lib/src/ios/ios_project_migration.dart
| @@ -54,3 +56,20 @@ class IOSMigrator { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| class IOSMigration { | |||
There was a problem hiding this comment.
Especially for a migration that might touch multiple files, this will have to be treated like a database transaction. We don't want to leave a tree in a partially migrated state, for example. Not for this PR (unless we already have multi-file migrations), but maybe add a TODO and file an issue.
|
|
||
| bool run() { | ||
| for (final IOSMigrator migrator in migrators) { | ||
| if (!migrator.migrate()) { |
There was a problem hiding this comment.
How should this code handle exceptions from migrator.migrate()
| @@ -2,26 +2,32 @@ | |||
| // Use of this source code is governed by a BSD-style license that can be | |||
| // found in the LICENSE file. | |||
|
|
|||
| import 'package:flutter_tools/src/base/common.dart'; | |||
There was a problem hiding this comment.
import '../../base/common.dart'There was a problem hiding this comment.
Ugh Android Studio always does that without me noticing. Turning off import code folding...
Description
Add usage event for failed iOS project migrations.
Move
flutterUsageto globals as part of #47161.Related Issues
Fixes #51817
See also #51453
Feedback followup from #51453 (comment).
Tests
Added
IOSMigrationtests.Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change