-
Notifications
You must be signed in to change notification settings - Fork 30.4k
Parse common SwiftPM errors and only fetch Swift packages when project is using SwiftPM #185218
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.
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
fbf2831
0f633dc
552cf55
abaed9b
54c62da
1164c3d
532a164
b3cb95e
6c5c75b
de045a6
8ea6be9
906b817
d49164b
97a9ad4
05e6127
037399e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -403,4 +403,51 @@ class SwiftPackageManager { | |
| manifestContents.replaceFirst(oldSupportedPlatform, newSupportedPlatform), | ||
| ); | ||
| } | ||
|
|
||
| static final List<_SwiftPMPluginErrorMatcher> _errorMatchers = <_SwiftPMPluginErrorMatcher>[ | ||
| _SwiftPMPluginErrorMatcher( | ||
| // Example: target 'plugin_a' in package 'plugin_a' is outside the package root | ||
| pattern: RegExp(r"target '([^']+)' in package '[^']+' is outside the package root"), | ||
| message: (RegExpMatch match) => | ||
| 'Flutter plugin "${match.group(1)}" has an incorrectly configured Package.swift file.\n' | ||
| 'Please contact the plugin maintainers for assistance.', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also provide the details in error mesage? (same for the other matchers below)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put the onus or printing the actual error on whatever call this method. For example in if (swiftPackageManagerError != null) {
_logger.printError(stderrString);
throwToolExit(swiftPackageManagerError);
} |
||
| ), | ||
| _SwiftPMPluginErrorMatcher( | ||
| // Example: /path/to/plugin_a/Package.swift:25:17: error: expected ',' separator | ||
| pattern: RegExp(r'([^\/]+)\/Package\.swift:\d+:\d+: error:'), | ||
|
vashworth marked this conversation as resolved.
|
||
| message: (RegExpMatch match) => | ||
| 'Flutter plugin "${match.group(1)}" has an incorrectly configured Package.swift file.\n' | ||
| 'Please contact the plugin maintainers for assistance.', | ||
| ), | ||
| _SwiftPMPluginErrorMatcher( | ||
| // Example: unknown package 'some-package' in dependencies of target 'plugin_a' | ||
| pattern: RegExp(r"unknown package '[^']+' in dependencies of target '([^']+)'"), | ||
| message: (RegExpMatch match) => | ||
| 'Flutter plugin "${match.group(1)}" has an incorrectly configured Package.swift file.\n' | ||
| 'Please contact the plugin maintainers for assistance.', | ||
| ), | ||
| ]; | ||
|
|
||
| /// Parses a Swift Package Manager error message and returns a guided error | ||
| /// message if the error matches a known pattern. | ||
| static String? parsePluginError(String? message, {required List<String> pluginNames}) { | ||
| if (message == null || message.isEmpty) { | ||
| return null; | ||
| } | ||
| for (final _SwiftPMPluginErrorMatcher matcher in _errorMatchers) { | ||
| for (final RegExpMatch match in matcher.pattern.allMatches(message)) { | ||
| final String? packageName = match.group(1); | ||
| if (packageName != null && pluginNames.contains(packageName)) { | ||
|
vashworth marked this conversation as resolved.
|
||
| return matcher.message(match); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| class _SwiftPMPluginErrorMatcher { | ||
| const _SwiftPMPluginErrorMatcher({required this.pattern, required this.message}); | ||
| final RegExp pattern; | ||
| final String Function(RegExpMatch match) message; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,13 +227,35 @@ class SwiftPackageManagerIntegrationMigration extends ProjectMigrator { | |
| } | ||
| } | ||
|
|
||
| try { | ||
| // When migrating for the first time, this will be the first time packages are downloaded | ||
| // and may take a while. | ||
| await _xcodeProjectInterpreter.prefetchSwiftPackages( | ||
| _xcodeProject.hostAppRoot.path, | ||
| buildDirectory: _fileSystem.directory( | ||
| _platform.buildDirectory(config: _config, fileSystem: _fileSystem), | ||
| ), | ||
| quiet: false, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note I was able to remove the force here because now prefetchSwiftPackagesForProject only runs if the project has migrated to SwiftPM already so it doesn't get run before here. |
||
| force: true, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why need to reset & restart the process when calling this from migration.dart (but not other places?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This migrator is what adds SwiftPM integration, so when the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. Can you add a comment about this corner case? This also feels like a bit of hack/workaround since we shouldn't start pre-fetch for un-migrated projects in the first place (feel free to leave a TODO tho).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I decided to go ahead and fix. I was trying to keep this small but then I realized there was also a flaw in that |
||
| ); | ||
| } on Exception catch (e) { | ||
| throw _PrefetchSwiftPackageException(e.toString()); | ||
| } | ||
|
|
||
| // Get the project info to make sure it compiles with xcodebuild | ||
| await _xcodeProjectInterpreter.getInfo( | ||
| _xcodeProject.hostAppRoot.path, | ||
| buildDirectory: _fileSystem.directory( | ||
| _platform.buildDirectory(config: _config, fileSystem: _fileSystem), | ||
| ), | ||
| ); | ||
| } on _PrefetchSwiftPackageException catch (e) { | ||
| restoreFromBackup(schemeInfo); | ||
| throwToolExit( | ||
| 'An error occurred when adding Swift Package Manager integration:\n' | ||
| ' ${e.message}\n\n' | ||
| '$kDisableSwiftPMInstructions' | ||
| ); | ||
| } on Exception catch (e) { | ||
| restoreFromBackup(schemeInfo); | ||
| if (optionalOnly) { | ||
|
|
@@ -248,8 +270,7 @@ class SwiftPackageManagerIntegrationMigration extends ProjectMigrator { | |
| throwToolExit( | ||
| 'An error occurred when adding Swift Package Manager integration:\n' | ||
| ' $e\n\n' | ||
| 'Swift Package Manager is currently an experimental feature, please file a bug at\n' | ||
| ' https://github.com/flutter/flutter/issues/new?template=01_activation.yml \n' | ||
| 'Please file a bug at https://github.com/flutter/flutter/issues/new?template=01_activation.yml \n' | ||
| 'Consider including a copy of the following files in your bug report:\n' | ||
| ' ${_platform.name}/Runner.xcodeproj/project.pbxproj\n' | ||
| ' ${_platform.name}/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme ' | ||
|
|
@@ -1430,3 +1451,12 @@ class ParsedProject { | |
| final String identifier; | ||
| final List<String>? packageReferences; | ||
| } | ||
|
|
||
| class _PrefetchSwiftPackageException implements Exception { | ||
| _PrefetchSwiftPackageException(this.message); | ||
|
|
||
| final String message; | ||
|
|
||
| @override | ||
| String toString() => message; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.