PodFile Multiple targets break Flipper support

TheSolly commented 4 years ago

Environment

System:
    OS: macOS 10.15.4
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 24.69 MB / 16.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 10.13.0 - ~/.nvm/versions/node/v10.13.0/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.4.1 - ~/.nvm/versions/node/v10.13.0/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.9.1 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: iOS 13.4, DriverKit 19.0, macOS 10.15, tvOS 13.4, watchOS 6.2
    Android SDK: Not Found
  IDEs:
    Android Studio: Not Found
    Xcode: 11.4/11E146 - /usr/bin/xcodebuild
  Languages:
    Java: 10.0.2 - /usr/bin/javac
    Python: 2.7.17 - /usr/local/bin/python
  npmPackages:
    @react-native-community/cli: Not Found
    react: 16.11.0 => 16.11.0
    react-native: 0.62.2 => 0.62.2
  npmGlobalPackages:
    *react-native*: Not Found

Upgrading version

from=0.61.5 --> to=0.62.2

Description

I am Having multiple target in the PodFile, and according to the upgrade helper process i should put this code inside the my target for Flipper.

target 'RnDiffApp' do
...
add_flipper_pods!
post_install do |installer|
  flipper_post_install(installer)
end
...
end

Trying to add the same piece of code into my other target Prod like this:

target 'RnDiffAppProd' do
...
add_flipper_pods!
post_install do |installer|
  flipper_post_install(installer)
end
...
end

When running pod install i got the following error:

[!] Invalid `Podfile` file: [!] Specifying multiple `post_install` hooks is unsupported

According to this issue 6172 CocoaPods doesn't support multiple post_install hooks.

To solve this issue, i had to rewrite the code as below:

target 'RnDiffApp' do
....
end
target 'RnDiffAppProd' do
....
end
add_flipper_pods!
post_install do |installer|
  flipper_post_install(installer)
end

So far both my target build successfully in debug mode with flipper support.

IMO, this should be the default way to insert the post-install hook for Flipper in the upgrade helper.

lucasbento commented 4 years ago

@TheSolly I have moved your issue from upgrade-helper as this is the proper repository to report it, can you please edit it to follow the issue template?

TheSolly commented 4 years ago

@lucasbento Done, thanks 🙏

lucasbento commented 4 years ago

@TheSolly I put the Question label but this should be a Solution, right?

TheSolly commented 4 years ago

@lucasbento Exactly, although this is my proposed solution. Am sure open to improvement.

lucasbento commented 4 years ago

@TheSolly cool!

cc @priteshrnandgaonkar & @alloy, any thoughts on this?

matt-oakes commented 4 years ago

Technically, I think the add_flipper_pods! should be inside each target which you want it to be used by. Moving it outside all targets will add it to all targets which, if you have a "Test" target, might not be the desired behaviour.

If your targets share the same set of pods, I would recommend defining a method to "share" the pod list:

def shared_pods
  # Core React Native libraries
  pod 'FBLazyVector', :path => "../node_modules/react-native/Libraries/FBLazyVector"
  pod 'FBReactNativeSpec', :path => "../node_modules/react-native/Libraries/FBReactNativeSpec"
  ... etc ...

  # Auto-linked pods
  use_native_modules!

  # Flipper pods
  add_flipper_pods!
end

target 'RnDiffApp' do
  shared_pods
end
target 'RnDiffAppProd' do
  shared_pods
end

post_install do |installer|
  flipper_post_install(installer)
end

alloy commented 4 years ago

What @matt-oakes said. Also note that CocoaPods has the notion of a ‘abstract target’ to share pods, but either solution will work for most cases.

TheSolly commented 4 years ago

Thanks for info, applied @matt-oakes methodology and everything is working smoothly 🙏 I think it would be helpful to add this comment/info in the upgrade helper 🤔

matt-oakes commented 4 years ago

I didn’t know about abstract targets. Good to know!

I don’t think it needs to be covered in the upgrade helper as it’s a bit more “advanced” than most users will need.