Skip to content

Detect plugins based on .flutter-plugins-dependencies; avoid refreshing twice in iOS/MacOS #157393

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

Closed
matanlurey wants to merge 2 commits into from
Closed

Detect plugins based on .flutter-plugins-dependencies; avoid refreshing twice in iOS/MacOS #157393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aad7ef5
++
matanlurey Oct 22, 2024
e2c9f10
Remove unused import.
matanlurey Oct 23, 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
2 changes: 1 addition & 1 deletion packages/flutter_tools/lib/src/flutter_plugins.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,7 +1131,7 @@ Future<void> injectPlugins(
///
/// Assumes [refreshPluginsList] has been called since last change to `pubspec.yaml`.
bool hasPlugins(FlutterProject project) {
return _readFileContent(project.flutterPluginsFile) != null;
return _readFileContent(project.flutterPluginsDependenciesFile) != null;
}

/// Resolves the plugin implementations for all platforms.
Expand Down
7 changes: 0 additions & 7 deletions packages/flutter_tools/lib/src/macos/cocoapod_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,6 @@ Future<void> processPodsIfNeeded(
if (project.usesSwiftPackageManager && !xcodeProject.podfile.existsSync() && !forceCocoaPodsOnly) {
return;
}
// Ensure that the plugin list is up to date, since hasPlugins relies on it.
await refreshPluginsList(
project,
iosPlatform: project.ios.existsSync(),
macOSPlatform: project.macos.existsSync(),
forceCocoaPodsOnly: forceCocoaPodsOnly,
Copy link
Member

@jmagman jmagman Oct 23, 2024

Choose a reason for hiding this comment

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

In #157391 (comment) I said

I don't think it was added in #146256, but the call site parameters were changed.

This was true, but I think the logic added in that PR was intended to forceCocoaPodsOnly for add-to-app, since add-to-app currently only supports CocoaPods and not Swift Package Manager.

flutter/packages/flutter_tools/lib/src/commands/build_ios_framework.dart

Lines 263 to 268 in 7504abc

await processPodsIfNeeded(
project.ios,
getIosBuildDirectory(),
buildInfo.mode,
forceCocoaPodsOnly: true,
);

flutter/packages/flutter_tools/lib/src/flutter_plugins.dart

Line 207 in 7504abc

result['swift_package_manager_enabled'] = !forceCocoaPodsOnly && project.usesSwiftPackageManager;

However, build_ios_framework_module_test (post-submit only test) would test this, so I checked this out, set flutter config --enable-swift-package-manager, and it still passed. Which makes sense because that project.usesSwiftPackageManager explicitly checks if it's an add-to-app module (!isModule).

flutter/packages/flutter_tools/lib/src/project.dart

Lines 269 to 272 in 7504abc

bool get usesSwiftPackageManager {
if (!manifest.disabledSwiftPackageManager &&
(ios.existsSync() || macos.existsSync()) &&
!isModule) {

All that to say, I still think this isn't needed, but not for the reason I said before.

@loic-sharma can you check my work here?

Copy link
Member

@jmagman jmagman Oct 23, 2024

Choose a reason for hiding this comment

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

Which makes sense because that project.usesSwiftPackageManager explicitly checks if it's an add-to-app module (!isModule).

...and I just remembered that command can be run from a normal flutter created iOS or macOS app, not just a module.

On this PR:

$ flutter config --enable-swift-package-manager
$ flutter create test_create
$ cd test_create
$ flutter pub add camera
$ flutter build ios-framework --xcframework --no-debug --no-profile
$ ls  build/ios/framework/Release/

On master that directory contains camera_avfoundation.xcframework, but on this PR it doesn't.

.flutter-plugins-dependencies should have swift_package_manager_enabled false at this point.

"swift_package_manager_enabled":true

Copy link
Member

@jmagman jmagman Oct 23, 2024

Choose a reason for hiding this comment

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

@loic-sharma this would be a good integration test to add.

Copy link
Member

@loic-sharma loic-sharma Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The flutter build ios-framework command acts unexpectedly on a non-module project if Swift Package Manager is enabled. If I do this:

flutter build ios-framework --xcframework --no-debug --no-profile

The build completes, however, this unexpectedly generates a Swift package manifest that includes plugins and then later deletes it.

Interestingly, the produced App.xframework doesn't seem to statically link against the plugins?

$ nm --extern-only --just-symbol-name build/ios/framework/Release/App.xcframework/ios-arm64/App.framework/App -arch arm64
_kDartIsolateSnapshotData
_kDartIsolateSnapshotInstructions
_kDartVmSnapshotData
_kDartVmSnapshotInstructions
dyld_stub_binder

I'll need to dig into this more

Copy link
Contributor Author

@matanlurey matanlurey Oct 23, 2024

Choose a reason for hiding this comment

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

Sounds good, thanks for looking at this!

I am not explicitly blocked, but I would like to know (can be tomorrow) if I should expect to pursue this change (that is, it should be OK if other bugs or considerations are fixed) or if I should expect to drop it (in which case I might want to just opt-out all users of processPodsIfNeeded from generating .flutter-plugins: #157393 (comment)).

Copy link
Member

@loic-sharma loic-sharma Oct 23, 2024

Choose a reason for hiding this comment

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

I suspect you should drop this for now and reassign to me. I should clean this up as part of adding add-to-app support to SwiftPM. Thoughts @jmagman?

);

// If there are no plugins and if the project is a not module with an existing
// podfile, skip processing pods
Expand Down
1 change: 1 addition & 0 deletions packages/flutter_tools/test/general.shard/macos/cocoapod_utils_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ void main() {
..writeAsStringSync(pluginYamlTemplate.replaceAll('PLUGIN_CLASS', name));
}

flutterProject.flutterPluginsDependenciesFile..createSync()..writeAsStringSync('');
packageConfigFile.writeAsStringSync(jsonEncode(packageConfig));
}

Expand Down