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

matanlurey · 2024-10-22T23:05:33Z

Two small but interconnected changes here:

Update hasPlugins to use the presence of .flutter-plugins-dependencies, not .flutter-plugins
Update processPodsIfNeeded to rely on the implicit refreshPluginsList provided by FlutterCommand.

jmagman · 2024-10-23T04:12:17Z

packages/flutter_tools/lib/src/macos/cocoapod_utils.dart

-    project,
-    iosPlatform: project.ios.existsSync(),
-    macOSPlatform: project.macos.existsSync(),
-    forceCocoaPodsOnly: forceCocoaPodsOnly,


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?

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

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

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

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)).

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?

jmagman

Based on https://github.com/flutter/flutter/pull/157393/files#r1811810540 this will make iOS add-to-app framework stop being generated (sorry I didn't think it all through before I said to go for this, @matanlurey)

matanlurey · 2024-10-23T19:05:19Z

@jmagman:

Based on https://github.com/flutter/flutter/pull/157393/files#r1811810540 this will make iOS add-to-app framework stop being generated (sorry I didn't think it all through before I said to go for this, @matanlurey)

I'm not familiar with this command or workflow - is there a way to either (a) make the command that triggers this trigger the pub flow (that would refresh the plugin lists) or (b) explicitly call refreshPluginsList in that workflow (versus unconditionally in processPodsIfNeeded)?

matanlurey · 2024-10-23T19:22:41Z

One workaround if we don't want to land this - I could hardcode processPodsIfNeeded to always exclude .flutter-plugins from being generated (which is why I was fixing this to begin with). I don't believe that would cause any problems (only the Android embedder seems to still handle that file), but I want to give you both options.

loic-sharma · 2024-10-23T20:23:25Z

I'm not familiar with this area. Let me read up a bit and I'll loop back on the things I was tagged on! :)

jmagman · 2024-10-24T05:31:01Z

I could hardcode processPodsIfNeeded to always exclude .flutter-plugins from being generated (which is why I was fixing this to begin with)

I vote this. I meant to ask why it running twice (which is bad) is related to moving off the deprecated .flutter-plugins file.

matanlurey · 2024-10-24T16:13:29Z

Sounds good, thanks for the discussion!

I reassigned #157391 and will open a smaller scope PR for processPodsIfNeeded.

…t`. (#157527) Work towards #48918. Workaround for #157391 (see #157393 (comment)). I'll run all post-submit tasks as well on this PR using `test: all`.

matanlurey requested a review from andrewkolos October 22, 2024 23:05

This comment was marked as outdated.

Sign in to view

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 22, 2024

matanlurey changed the title ~~Use existence of .flutter-plugins-dependencies for hasPlugins.~~ Rely on FlutterCommand to refreshPluginsList; use .flutter-plugins-dependencies for hasPlugins. Oct 22, 2024

matanlurey changed the title ~~Rely on FlutterCommand to refreshPluginsList; use .flutter-plugins-dependencies for hasPlugins.~~ Detect plugins based on .flutter-plugins-dependencies; avoid refreshing twice in iOS/MacOS Oct 22, 2024

matanlurey requested a review from jmagman October 22, 2024 23:22

++

aad7ef5

matanlurey force-pushed the flutter-plugins-deps-has-plugins branch from f61fb9c to aad7ef5 Compare October 22, 2024 23:23

github-actions bot added the a: desktop Running on desktop label Oct 22, 2024

jmagman reviewed Oct 23, 2024

View reviewed changes

matanlurey requested a review from loic-sharma October 23, 2024 18:07

Remove unused import.

e2c9f10

jmagman requested changes Oct 23, 2024

View reviewed changes

matanlurey mentioned this pull request Oct 24, 2024

flutter <build> <ios> regenerates plugins multiple times? #157391

Open

matanlurey closed this Oct 24, 2024

matanlurey deleted the flutter-plugins-deps-has-plugins branch October 24, 2024 16:13

matanlurey mentioned this pull request Oct 24, 2024

Allow opting out of .flutter-plugins, opt-out in refreshPluginsList. #157527

Merged

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

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

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

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

Uh oh!

Conversation

matanlurey commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

jmagman Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

loic-sharma Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

loic-sharma Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matanlurey commented Oct 23, 2024

Uh oh!

loic-sharma commented Oct 23, 2024

Uh oh!

jmagman commented Oct 24, 2024

Uh oh!

matanlurey commented Oct 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

matanlurey commented Oct 22, 2024 •

edited

Loading

loic-sharma Oct 23, 2024 •

edited

Loading

matanlurey commented Oct 23, 2024 •

edited

Loading