flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Solve the problem that <Flutter/Flutter.h> cannot be imported when a pod transitive depends on Flutter

Open intspt opened this issue 2 years ago • 25 comments

image image If a pod transitive depends on a pod containing a framework, cocoapods will add the path of the framework to its FRAMEWORK_SEARCH_PATHS. So I modified the relevant logic in podhelper, hoping to be consistent with the behavior of cocoapods.

intspt avatar Apr 27 '23 07:04 intspt

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Apr 27 '23 07:04 google-cla[bot]

@intspt can you file an issue that describes exactly what you're seeing and how to reproduce it? I don't understand how you got into the state from those screenshots.

jmagman avatar Apr 28 '23 18:04 jmagman

@intspt can you file an issue that describes exactly what you're seeing and how to reproduce it? I don't understand how you got into the state from those screenshots.

I have provided a demo to reproduce this problem: https://github.com/intspt/bug_demo The dependency tree is the same in the normal_demo and bug_demo scenarios.

NativePod -> shared_preferences -> Flutter

In the normal_demo, cocoapods added the directory of Flutter.framework to the FRAMEWORK_SEARCH_PATHS of NativePod. Because shared_preferences dependencies will be inherited to NativePod. image

But in the bug_demo, Flutter is a fake pod. FRAMEWORK_SEARCH_PATHS is processed by podhelper, which misses the scenarios of transitive dependencies, so an error will be reported during compilation. image

intspt avatar May 04 '23 08:05 intspt

Can you file an actual GitHub issue instead of updating this PR? I am not quite sure the use case of a native pod depending on a Flutter plugin, for example, and it's better to have that conversation documented in an issue since that's generally where folks will look for duplicates, etc.

Additionally, this will need a test. Unfortunately we don't really have Ruby tests so it would need to be an integration test, see https://github.com/flutter/flutter/blob/c2022aa5b140544e103025e6995b41c872e39d2c/dev/devicelab/lib/tasks/plugin_tests.dart or https://github.com/flutter/flutter/blob/master/dev/devicelab/bin/tasks/build_ios_framework_module_test.dart for an example of where that could live.

jmagman avatar May 04 '23 17:05 jmagman

Can you file an actual GitHub issue instead of updating this PR? I am not quite sure the use case of a native pod depending on a Flutter plugin, for example, and it's better to have that conversation documented in an issue since that's generally where folks will look for duplicates, etc.

Additionally, this will need a test. Unfortunately we don't really have Ruby tests so it would need to be an integration test, see https://github.com/flutter/flutter/blob/c2022aa5b140544e103025e6995b41c872e39d2c/dev/devicelab/lib/tasks/plugin_tests.dart or https://github.com/flutter/flutter/blob/master/dev/devicelab/bin/tasks/build_ios_framework_module_test.dart for an example of where that could live.

Our app is called Xianyu. This problem was encountered in our own project, and there should be no similar issues.

The most direct solution to this problem is to make the native pod depend on Flutter. But we can't do this in our project. We put Flutter.framework, App.framework, and framework compiled by plugin into a pod called IdlefishFlutter. The purpose is to allow several scenarios such as local development of Flutter projects, local development of native parts, and remote packaging to be used normally. Therefore, in these scenarios, native pods can only rely on IdlefishFlutter. image

Of course, the root cause of this problem is that there are some flaws in the processing of podhelper. I think it is a reasonable solution to keep the processing logic of podhelper consistent with the logic of cocoapods.

intspt avatar May 06 '23 08:05 intspt

Can you file an actual GitHub issue instead of updating this PR? I am not quite sure the use case of a native pod depending on a Flutter plugin, for example, and it's better to have that conversation documented in an issue since that's generally where folks will look for duplicates, etc.

Additionally, this will need a test. Unfortunately we don't really have Ruby tests so it would need to be an integration test, see https://github.com/flutter/flutter/blob/c2022aa5b140544e103025e6995b41c872e39d2c/dev/devicelab/lib/tasks/plugin_tests.dart or https://github.com/flutter/flutter/blob/master/dev/devicelab/bin/tasks/build_ios_framework_module_test.dart for an example of where that could live.

This change is only to add the directory where Flutter.framework is located in the FRAMEWORK_SEARCH_PATHS of the pod that transitive depends on Flutter. This problem may only be encountered in the Add-to-app scenario. So it seems impossible to add this scenario in the integration test.

intspt avatar May 06 '23 09:05 intspt

@intspt as request above:

Can you file an actual GitHub issue instead of updating this PR

We have a standard process, which includes separating discussion of what the problem is (the issue tracker) and reviewing a fix (PR discussion). Following our process will make things go more smoothly

This problem was encountered in our own project, and there should be no similar issues.

Almost all problems are encountered in developers' projects. That doesn't mean that nobody can ever encounter the same issue.

stuartmorgan-g avatar May 06 '23 11:05 stuartmorgan-g

@intspt as request above:

Can you file an actual GitHub issue instead of updating this PR

We have a standard process, which includes separating discussion of what the problem is (the issue tracker) and reviewing a fix (PR discussion). Following our process will make things go more smoothly

This problem was encountered in our own project, and there should be no similar issues.

Almost all problems are encountered in developers' projects. That doesn't mean that nobody can ever encounter the same issue.

I raised an issue, https://github.com/flutter/flutter/issues/126251.

intspt avatar May 08 '23 08:05 intspt

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install.

Maybe the recursive depth should be limited

hyiso avatar May 18 '23 08:05 hyiso

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install.

Maybe the recursive depth should be limited

Recursive operations increase the time consumption very little. Taking our project as an example, there are about 320+ pods. The total time of flutter_additional_ios_build_settings is about 1s. now: image before: image

intspt avatar May 18 '23 11:05 intspt

I'm going to mark this PR as a draft for now, as it seems per the discussion on https://github.com/flutter/flutter/issues/126251 it is not clear if the issue is one that should be fixed in the Flutter SDK. I think we should resolve that in that issue first, before reviewing this PR.

christopherfujino avatar Jun 08 '23 21:06 christopherfujino

Apologies for the delay in getting back to this; looking further at the use case, this seems like a reasonable solution. For non-add-to-app cases it seems like this should have minimal impact.

stuartmorgan-g avatar Jul 25 '23 20:07 stuartmorgan-g

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install.

Maybe the recursive depth should be limited

@hyiso Are you saying you saw actual performance problems with this PR applied? What does your dependency graph look like?

stuartmorgan-g avatar Jul 25 '23 20:07 stuartmorgan-g

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install. Maybe the recursive depth should be limited

@hyiso Are you saying you saw actual performance problems with this PR applied? What does your dependency graph look like?

Yes, after applying this PR, it stuck at Generating Pods project over 10 minutes when running pod install.

The host iOS project has a lot of pod dependencies (I don't know how to count the total number).

But I'm not sure whether it is a bug of the PR or just related to the dependencies complication, as @intspt said he does not have this problem.

BTW, my current workaround is just to comment out this line next unless target.dependencies.any? { |dependency| dependency.name == 'Flutter' } in this file

hyiso avatar Jul 26 '23 14:07 hyiso

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install. Maybe the recursive depth should be limited

@hyiso Are you saying you saw actual performance problems with this PR applied? What does your dependency graph look like?

Yes, after applying this PR, it stuck at Generating Pods project over 10 minutes when running pod install.

The host iOS project has a lot of pod dependencies (I don't know how to count the total number).

But I'm not sure whether it is a bug of the PR or just related to the dependencies complication, as @intspt said he does not have this problem.

BTW, my current workaround is just to comment out this line next unless target.dependencies.any? { |dependency| dependency.name == 'Flutter' } in this file

As mentioned here https://github.com/flutter/flutter/pull/125610#discussion_r1275130159, after adding return false, the stuck disappeared.

I'll continue to try this PR to check whether the problem <Flutter/Flutter.h> not found is solved

hyiso avatar Jul 26 '23 15:07 hyiso

Tried this PR, this might cause a long time waiting at 'Generating Pods project' when running pod install. Maybe the recursive depth should be limited

@hyiso Are you saying you saw actual performance problems with this PR applied? What does your dependency graph look like?

Yes, after applying this PR, it stuck at Generating Pods project over 10 minutes when running pod install. The host iOS project has a lot of pod dependencies (I don't know how to count the total number). But I'm not sure whether it is a bug of the PR or just related to the dependencies complication, as @intspt said he does not have this problem. BTW, my current workaround is just to comment out this line next unless target.dependencies.any? { |dependency| dependency.name == 'Flutter' } in this file

As mentioned here #125610 (comment), after adding return false, the stuck disappeared.

I'll continue to try this PR to check whether the problem <Flutter/Flutter.h> not found is solved

Seems not solved in my host iOS project, I'll keep to use my workaround.

hyiso avatar Jul 30 '23 10:07 hyiso

@intspt are you still interested in working on this?

christopherfujino avatar Sep 07 '23 20:09 christopherfujino

@intspt are you still interested in working on this?

I'm sorry for not seeing the previous reply, I'm still interested. I will modify my code as soon as possible as per stuartmorgan's suggestion.

intspt avatar Sep 08 '23 11:09 intspt

LGTM, but as @stuartmorgan and @jmagman previously requested, this will need an integration test. See previous comment (#125610 (comment)) for some examples

ok, I'll add an integration test for this change. But I have been busy recently, so I will be able to provide it later.

intspt avatar Sep 21 '23 02:09 intspt

@intspt can you file an issue that describes exactly what you're seeing and how to reproduce it? I don't understand how you got into the state from those screenshots.

Hello, I don’t know how to write this integration test task. I need to create a native pod that depends on any Flutter Plugin, but it doesn't seem possible.

intspt avatar Oct 23 '23 11:10 intspt

I need to create a native pod that depends on any Flutter Plugin, but it doesn't seem possible.

Why wouldn't that be possible; isn't that exactly the case that this PR is intended to handle?

stuartmorgan-g avatar Oct 23 '23 13:10 stuartmorgan-g

I need to create a native pod that depends on any Flutter Plugin, but it doesn't seem possible.

Why wouldn't that be possible; isn't that exactly the case that this PR is intended to handle?

But I don’t know how to introduce the pod I want in the integration test. Can I write the contents of the podspec and .m file into the dart file?

intspt avatar Oct 23 '23 13:10 intspt

@vashworth if you want to hold the testing line here I totally understand since you still have this marked as changes requested. We should either approve it or close it (or write the test ourselves) since it's been hanging out so long.

jmagman avatar Jan 03 '24 02:01 jmagman

@intspt Here's how I would add it to the plugin_test_ios integration test. Feel free to use this code in this PR: https://github.com/flutter/flutter/compare/master...vashworth:flutter:flutter_transitive_dependency_test

vashworth avatar Jan 04 '24 18:01 vashworth

@intspt Here's how I would add it to the plugin_test_ios integration test. Feel free to use this code in this PR: master...vashworth:flutter:flutter_transitive_dependency_test

thx, I'll finish it this weekend

intspt avatar Jan 12 '24 09:01 intspt