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

Bake prod env var into react_native_pods.rb for iOS Release build #34234

Closed
wants to merge 4 commits into from
Closed

Bake prod env var into react_native_pods.rb for iOS Release build #34234

wants to merge 4 commits into from

Conversation

leotm
Copy link
Contributor

@leotm leotm commented Jul 20, 2022

Summary

Mentioned

Close: #33764

Saw the issue ago couple wks too: leotm/react-native-template-new-architecture#757
Fixed similarly: leotm/react-native-template-new-architecture#791

Changelog

[iOS] [Changed] - Bake prod env var into react_native_pods.rb for iOS Release build

Test Plan

Everything builds and runs as expected

@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 Jul 20, 2022
@leotm leotm changed the title Allow PRODUCTION=1 pod install Allow PRODUCTION=1 pod install in template Jul 20, 2022
@leotm leotm marked this pull request as ready for review July 20, 2022 21:19
@leotm leotm changed the title Allow PRODUCTION=1 pod install in template Update Podfile for PRODUCTION=1 pod install Jul 20, 2022
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 20, 2022
@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Jul 20, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 20, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,819,885 -878
android hermes armeabi-v7a 7,212,804 -1,827
android hermes x86 8,133,685 -778
android hermes x86_64 8,111,999 -576
android jsc arm64-v8a 9,698,003 -134
android jsc armeabi-v7a 8,454,127 -142
android jsc x86 9,649,800 -144
android jsc x86_64 10,248,023 -134

Base commit: 143a0f7
Branch: main

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @leotm, thanks for this PR. I think that if we want to support the PRODUCTION env var, it is better to bake it into the react_native_pods.rb similarly to what we did with the new_arch_enabled flag.

Your fix works only for people who are using the template. The other will work for everyone using React Native.

What do you think?

@analysis-bot
Copy link

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

Base commit: 143a0f7
Branch: main

@leotm
Copy link
Contributor Author

leotm commented Jul 21, 2022

gotcha at first wasn't sure if everyone using RN meant those on diff versions or everyone internally

@leotm
Copy link
Contributor Author

leotm commented Jul 21, 2022

i couldn't tell too much from the failing tests earlier

@leotm
Copy link
Contributor Author

leotm commented Jul 21, 2022

but think i've understood right, reverted prev change and baked into react_native_pods.rb similar to new_arch_enabled

@leotm leotm requested a review from cipolleschi July 21, 2022 08:04
scripts/react_native_pods.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Unfortunately, this change can't work. Could you update it with my suggestion?

scripts/react_native_pods.rb Outdated Show resolved Hide resolved
Thanks figured ^ Appreciate the detailed follow-up

Co-authored-by: Riccardo <riccardo.cipolleschi@gmail.com>
@leotm leotm requested a review from cipolleschi July 21, 2022 08:16
@facebook-github-bot
Copy link
Contributor

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

@leotm leotm changed the title Update Podfile for PRODUCTION=1 pod install Update react_native_pods.rb for PRODUCTION=1 pod install Jul 21, 2022
@leotm leotm changed the title Update react_native_pods.rb for PRODUCTION=1 pod install Bake prod env into react_native_pods.rb for iOS Release build Jul 21, 2022
@leotm leotm changed the title Bake prod env into react_native_pods.rb for iOS Release build Bake prod env var into react_native_pods.rb for iOS Release build Jul 21, 2022
@leotm
Copy link
Contributor Author

leotm commented Jul 21, 2022

i couldn't tell too much from the failing tests earlier

  • test_windows

    • choco install yarn

      • The remote server returned an error: (503) Server Unavailable. Service Unavailable
      • The remote server returned an error: (524).

test_windows is the last, seems unrelated to this PR

@cipolleschi
Copy link
Contributor

Chocolatey was under maintenance this morning, nothing wrong on your side!

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @leotm in 77752fc.

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 Jul 22, 2022
leotm added a commit to leotm/react-native-template-new-architecture that referenced this pull request Jul 25, 2022
leotm added a commit to leotm/react-native-template-new-architecture that referenced this pull request Jul 25, 2022
kelset pushed a commit that referenced this pull request Jul 27, 2022
Summary:
### Mentioned
- pr[main]: #33882
- discussion: reactwg/react-native-releases#21 (reply in thread)
- pr[0.69-stable]: #34098

Close: #33764

Saw the issue ago couple wks too: leotm/react-native-template-new-architecture#757
Fixed similarly: leotm/react-native-template-new-architecture#791

## Changelog

[iOS] [Changed] - Update Podfile to allow `PRODUCTION=1 pod install`

[CATEGORY] [TYPE] - Message

Pull Request resolved: #34234

Test Plan: Everything builds and runs as expected

Reviewed By: cortinico

Differential Revision: D38029117

Pulled By: cipolleschi

fbshipit-source-id: bdb58200a999cb66f1043a2feb670f9037c8e463
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to build release version for iOS with the new architecture
6 participants