Skip to content
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.

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

Exclude all FlipperKit transitive dependencies from iOS Release builds #28504

Conversation

javiercr
Copy link
Contributor

@javiercr javiercr commented Apr 3, 2020

Summary

The :configuration option from pod only affects the specified pod and not its dependencies [1]. Therefore in order to avoid all transitive dependencies being linked in the resulting Release IPA we need to list them in the Podfile.

Note that this will still build Flipper's pods when doing a Release, but it won't link it in the resulting IPA.

[1] https://guides.cocoapods.org/syntax/podfile.html#pod

Fixes react-native-community/upgrade-support#28
Related CocoaPods/CocoaPods#9658

Changelog

  • [iOS] [Fixed] - Exclude Flipper from iOS Release builds

Test Plan

Create a new React Native 0.62 project, run pod install, then diff:

ProjectName/ios/Pods/Target Support Files/Pods-ProjectName/Pods-ProjectName.debug.xcconfig`

and

ProjectName/ios/Pods/Target Support Files/Pods-ProjectName/Pods-ProjectName.relaese.xcconfig

image

…ase build

The `:configuration` option from `pod` only affects the specified pod and not its dependencies [1]. Therefore in order to avoid all transitive dependencies being linked in the resulting Release IPA we need to list them in the `Podfile`.

[1] https://guides.cocoapods.org/syntax/podfile.html#pod
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 3, 2020
scripts/react_native_pods.rb Outdated Show resolved Hide resolved
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Apr 3, 2020
@analysis-bot
Copy link

analysis-bot commented Apr 3, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,607,449 0
android hermes armeabi-v7a 6,284,307 0
android hermes x86 6,948,198 0
android hermes x86_64 6,878,682 0
android jsc arm64-v8a 8,915,721 0
android jsc armeabi-v7a 8,555,924 0
android jsc x86 8,740,503 0
android jsc x86_64 9,317,240 0

Base commit: 3a11f05

@analysis-bot
Copy link

analysis-bot commented Apr 3, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 809040 n/a

Base commit: 83fee73

Copy link
Contributor

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time!

In the meantime, I’ll port this to the 0.62-stable branch and get it in today’s 0.62.1 release. Actually not going to do that, we don’t want to cherry-pick things not landed in master and the fix is easy enough for people to make locally.

@alloy
Copy link
Contributor

alloy commented Apr 3, 2020

@passy Can you get this in?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@priteshrnandgaonkar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @javiercr in e5497ca.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Apr 7, 2020
alloy pushed a commit that referenced this pull request Apr 8, 2020
#28504)

Summary:
The `:configuration` option from `pod` only affects the specified pod and not its dependencies [1]. Therefore in order to avoid all transitive dependencies being linked in the resulting Release IPA we need to list them in the `Podfile`.

Note that this will still build Flipper's pods when doing a Release, but it won't link it in the resulting IPA.

[1] https://guides.cocoapods.org/syntax/podfile.html#pod

Fixes react-native-community/upgrade-support#28
Related CocoaPods/CocoaPods#9658

* [iOS] [Fixed] - Exclude Flipper from iOS Release builds
Pull Request resolved: #28504

Test Plan:
Create a new React Native 0.62 project, run `pod install`, then diff:
```
ProjectName/ios/Pods/Target Support Files/Pods-ProjectName/Pods-ProjectName.debug.xcconfig`
```
and
```
ProjectName/ios/Pods/Target Support Files/Pods-ProjectName/Pods-ProjectName.relaese.xcconfig
```

![image](https://user-images.githubusercontent.com/855995/78337679-a3fa0280-7591-11ea-8142-6f82cbc6be58.png)

Reviewed By: passy

Differential Revision: D20894406

Pulled By: priteshrnandgaonkar

fbshipit-source-id: 680780f0f5a85fd8423b85a271a499bd12f06d00
@Ashoat
Copy link
Contributor

Ashoat commented Sep 18, 2020

Is there a reason this doesn't cover the following transitive dependencies?

  • CocoaAsyncSocket
  • CocoaLibEvent
  • OpenSSL-Universal
  • YogaKit

@alloy
Copy link
Contributor

alloy commented Sep 18, 2020

@Ashoat Not as far as I know. Can you make a PR for it?

@Ashoat
Copy link
Contributor

Ashoat commented Sep 18, 2020

My only worry is that in contrast with the other excluded transitive dependencies, which are definitely Flipper-specific, these four could be required by other pods. It appears that applying :configuration => 'Debug' will affect the given dependency wherever it appears in the dependency tree (otherwise this PR would not seem to work), which makes me worry that if another pod depends on these dependencies in release builds, it would end up breaking. So I'm reluctant to put up a PR that simply excludes them without understanding a bit more.

Ideally we would have a mechanism to exclude them from release builds only if they don't appear anywhere else in the dependency tree, but I'm not familiar enough with CocoaPods to know if something like that is possible.

@javiercr
Copy link
Contributor Author

My only worry is that in contrast with the other excluded transitive dependencies, which are definitely Flipper-specific, these four could be required by other pods. It appears that applying :configuration => 'Debug' will affect the given dependency wherever it appears in the dependency tree (otherwise this PR would not seem to work), which makes me worry that if another pod depends on these dependencies in release builds, it would end up breaking.

I think that's why I didn't include them in this PR. I could only exclude (actually include) the ones that I knew for sure were dependencies only for Flipper (basically the ones with Flipper in the name).

@alloy
Copy link
Contributor

alloy commented Sep 21, 2020

The best solution would be to take this CocoaPods PR to completion CocoaPods/CocoaPods#9066. It’s on my TODO list, but I have no idea how long it would take before I get to that, so if there are other takers please do take a shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flipper shouldn't be included in Release builds
7 participants