handle multiple application product native targets in SPM migrator #165115
Conversation
| @@ -64,18 +64,20 @@ class SwiftPackageManagerIntegrationMigration extends ProjectMigrator { | |||
| /// Existing macOS identifier for Runner PBXFrameworksBuildPhase. | |||
| static const String _macosRunnerFrameworksBuildPhaseIdentifier = '33CC10EA2044A3C60003C045'; | |||
|
|
|||
| /// Existing iOS identifier for Runner PBXNativeTarget. | |||
There was a problem hiding this comment.
The PBXNativeTarget's identifier can be different if there are multiple targets which each have the application product type. I.e. the default Runner scheme is duplicated to quickly get going for a second application.
| @@ -234,8 +230,8 @@ class SwiftPackageManagerIntegrationMigration extends ProjectMigrator { | |||
| final String schemeContent = schemeInfo.schemeContent; | |||
|
|
|||
| // The scheme should have a BuildableReference already in it with a | |||
| // BlueprintIdentifier matching the Runner Native Target. Copy from it | |||
| // since BuildableName, BlueprintName, ReferencedContainer may have been | |||
| // BlueprintIdentifier matching a Native Target. Copy from it | |||
There was a problem hiding this comment.
The BlueprintIdentifier can also change
| @@ -246,9 +242,21 @@ class SwiftPackageManagerIntegrationMigration extends ProjectMigrator { | |||
| // ReferencedContainer = "container:Runner.xcodeproj"> | |||
| // </BuildableReference> | |||
| final List<String> schemeLines = LineSplitter.split(schemeContent).toList(); | |||
| final int index = schemeLines.indexWhere( | |||
| (String line) => line.contains('BlueprintIdentifier = "$_runnerNativeTargetIdentifier"'), | |||
There was a problem hiding this comment.
Since the scheme cannot rely on the constant anymore, we have to parse the identifier.
| ParsedProjectInfo projectInfo, { | ||
| bool logErrorIfNotMigrated = false, | ||
| }) { | ||
| final bool migrated = | ||
| projectInfo.nativeTargets | ||
| .where( | ||
| (ParsedNativeTarget target) => | ||
| target.identifier == _runnerNativeTargetIdentifier && |
There was a problem hiding this comment.
Instead of relying on the fixed identifier that only applies for the target that is created using the project template,
I updated this to work for all application targets.
| @@ -557,69 +565,87 @@ $newContent | |||
| .toList() | |||
| .isNotEmpty; | |||
| if (logErrorIfNotMigrated && !migrated) { | |||
| logger.printError('PBXNativeTarget was not migrated or was migrated incorrectly.'); | |||
| logger.printError('Some PBXNativeTargets were not migrated or were migrated incorrectly.'); | |||
There was a problem hiding this comment.
Not sure if we should list the names of the affected targets? I think for this fix we can keep it without the names and improve on this later?
| final int packageProductDependenciesIndex = lines.indexWhere( | ||
| (String line) => line.trim().contains('packageProductDependencies'), | ||
| runnerNativeTargetStartIndex, | ||
| lines.insertAll(nativeTargetStartIndex + 1, newContent); |
There was a problem hiding this comment.
I used an early return to avoid the else here, hence the odd diff.
|
|
||
| if (applicationNativeTargets.isEmpty) { | ||
| throw Exception( | ||
| 'Unable to find parsed PBXNativeTargets for ${_xcodeProject.hostAppProjectName} project.', |
There was a problem hiding this comment.
The old code had target. at the end, but that is a bit misleading. It's the Runner project that has N Targets.
Because the migrator now handles multiple Targets, it gets more confusing so I updated it here to rectify that.
| if (packageProductDependenciesIndex == -1 || | ||
| packageProductDependenciesIndex > endSectionIndex) { | ||
| throw Exception( | ||
| 'Unable to find packageProductDependencies for ${_xcodeProject.hostAppProjectName} PBXNativeTarget.', |
There was a problem hiding this comment.
Should we use the name of the affected target? I also saw that there isn't an existing test for this throw statement. I guess this cannot really happen since the nativeTarget.packageProductDependencies are not null at this point?
| throw Exception( | ||
| 'Unable to find PBXNativeTarget for ${_xcodeProject.hostAppProjectName} target.', |
There was a problem hiding this comment.
Added the name of the affected target + clarifying that hostAppProjectName is the project
| if (packageProductDependenciesIndex == -1 || | ||
| packageProductDependenciesIndex > endSectionIndex) { | ||
| throw Exception( | ||
| 'Unable to find packageProductDependencies for $pbxTargetName in ${_xcodeProject.hostAppProjectName} project.', |
There was a problem hiding this comment.
Added the name of the affected target + clarifying that hostAppProjectName is the project
| /// - The migrator is rerun through the flutter CLI upon executing `flutter run` | ||
| /// - The migrator should skip sections like 'PBXBuildFile' or 'XCSwiftPackageProductDependency', | ||
| /// since they are already migrated | ||
| void _ensureNewIdentifiersNotUsed(List<String> lines) { |
There was a problem hiding this comment.
When adding the missing check for already migrated targets back, I encountered an issue:
The _ensureNewIdentifiersNotUsed is too conservative with its checks.
So I had to fix this by allowing the identifiers in the sections where these might occur after the migrator ran once. This enables the migrator to run again when additional application targets are added.
I'm not sure if there is a better solution to this issue?
| /// Returns false if the [identifier] does not occur in the given [lines]. | ||
| /// Returns false if the [identifier] only occurs within the sections denoted by the [sectionNames]. | ||
| /// Returns true otherwise. | ||
| bool _isIdentifierOutsideAllowedSections( |
There was a problem hiding this comment.
This is used by the new duplicate identifier check
| @@ -671,6 +744,57 @@ void main() { | |||
| expect(testLogger.traceText, contains('project.pbxproj already migrated. Skipping...')); | |||
| }); | |||
|
|
|||
| testWithoutContext('skips PBXNativeTarget migration if all targets are migrated', () async { | |||
There was a problem hiding this comment.
There was no "skip" test for when the Runner target was already migrated. This is one of several new tests to cover that.
| @@ -149,8 +149,13 @@ abstract class XcodeBasedProject extends FlutterProjectPlatform { | |||
| /// Checks if FlutterGeneratedPluginSwiftPackage has been added to the | |||
| /// project's build settings by checking the contents of the pbxproj. | |||
| bool get flutterPluginSwiftPackageInProjectSettings { | |||
There was a problem hiding this comment.
The new tests for skipping migrated PBXNativeTargets was failing at first. It turns out that this getter is a bit conservative, so it would also abort the migrator when the pbxproj would for example have FlutterGeneratedPluginSwiftPackage in the local swift package section even if the active target didn't have the migration. I updated all the relevant getters that were affected.
The new implementation now checks for a line in the PBXBuildFile section.
| @@ -2980,15 +3272,16 @@ String migratedNativeTargetSection( | |||
| SupportedPlatform platform, { | |||
| bool missingPackageProductDependencies = false, | |||
| bool withOtherDependency = false, | |||
| String? otherNativeTarget, | |||
There was a problem hiding this comment.
I updated this method to allow for inserting an additional PBXNativeTarget. To keep the readability for the Runner target, I went with a StringBuffer, to allow for splitting it up, while preserving the sections.
| /// A variant of [migratedNativeTargetSection] | ||
| /// that includes an additional PBXNativeTarget, "OtherTarget", that is not migrated. | ||
| String migratedRunnerWithUnmigratedOtherTargetSection(SupportedPlatform platform) { | ||
| final String otherTarget = <String>[ |
There was a problem hiding this comment.
I only added this target for an iOS specific test. Not sure if we need a MacOS variant also? The only real difference is the identifiers being slightly different.
There was a problem hiding this comment.
Hi @navaronbracke! This is great work, but unfortunately I don't think this is something we want to adopt. When custom targets get involved it gets much more complicated and altering the pbxproj file is already a little precarious.
I think for a majority of Flutter users, the defaults are probably sufficient. I would be open to accepting a PR that detects a custom target is being used and is missing the migrations and throws a warning/error with a link to the documentation on how to add manually.
|
Makes sense to do that instead. I'll look into repurposing this PR to let it add that to the migrator. |
This PR updates the Swift Package Manager project migrator to handle multiple PBXNativeTargets that are application products, instead of only the default Runner target. Other types of targets are not affected (i.e. App Clips or extensions)
Fixes #164099
Several tests were added and some other migrator specific tests were modified to clarify some things between the "project" and the potential many "targets"
This was also manually tested by doing the repro steps from the linked issue to verify that the migration still works for the default Runner target / scheme and an additional application target with its own scheme.
If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].
Pre-launch Checklist
///).