Skip to content

feat: set Swift active compilation conditions build setting to DEBUG - #42330

Closed
okwasniewski wants to merge 1 commit into
react:mainfrom
okwasniewski:feat/swift-active-compilation-condtions
Closed

feat: set Swift active compilation conditions build setting to DEBUG #42330
okwasniewski wants to merge 1 commit into
react:mainfrom
okwasniewski:feat/swift-active-compilation-condtions

Conversation

@okwasniewski

Copy link
Copy Markdown
Contributor

Summary:

This PR adds cocoapods utility to set SWIFT_ACTIVE_COMPILATION_CONDITIONS to DEBUG, which is set to this value by default (when generating a new native Xcode project).

This allows to use the #if DEBUG compilator directive in Swift to work out of the box, without any changes on user's side:

overridefunc bundleURL()->URL?{#if DEBUGRCTBundleURLProvider.sharedSettings().jsBundleURL(forBundleRoot:"index")#elseBundle.main.url(forResource:"main", withExtension:"jsbundle")#endif}

Changelog:

[IOS] [ADDED] - Set SWIFT_ACTIVE_COMPILATION_CONDITIONS to DEBUG

Test Plan:

Run bundle exec pod install and check if the active compilation flags are populated:

CleanShot 2024-01-17 at 13 11 03@2x

@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Jan 17, 2024
@okwasniewskiokwasniewski changed the title feat: add set_swift_compilation_conditions utility to set Swift compilation conditionsfeat: set SWIFT_ACTIVE_COMPILATION_CONDITIONS build setting to DEBUG Jan 17, 2024
@okwasniewskiokwasniewski changed the title feat: set SWIFT_ACTIVE_COMPILATION_CONDITIONS build setting to DEBUG feat: set Swift active compilation conditions build setting to DEBUG Jan 17, 2024
@facebook-github-botfacebook-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 Jan 17, 2024
Comment threadpackages/react-native/scripts/cocoapods/utils.rb Outdated
@analysis-bot

analysis-bot commented Jan 17, 2024

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a16,803,066-435,494
androidhermesarmeabi-v7an/a--
androidhermesx86n/a--
androidhermesx86_64n/a--
androidjscarm64-v8a20,190,312-410,683
androidjscarmeabi-v7an/a--
androidjscx86n/a--
androidjscx86_64n/a--

Base commit: b41a33e
Branch: main

@okwasniewski

Copy link
Copy Markdown
ContributorAuthor

Hey @cipolleschi, anything I can do to help you get this one merged?

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I completely missed this PR. 😭

The changes look good. Could you add a test in utils-test.rb file to verify that the build settings is actually added? In this way we can make sure not to miss changes in the set_swift_compilation_conditions method

@okwasniewski
okwasniewskiforce-pushed the feat/swift-active-compilation-condtions branch from df39513 to 69f4840CompareFebruary 12, 2024 16:15
@okwasniewski
okwasniewskiforce-pushed the feat/swift-active-compilation-condtions branch from 69f4840 to d821fefCompareFebruary 12, 2024 16:24
@okwasniewski

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi I've refactored the method to set_build_setting which accepts necessary arguments to set proper build settings. This method can remove set_use_hermes_build_setting and set_node_modules_user_settings methods (less code to maintain). I've also changed the tests to only test the utility

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job, thanks for refactoring!

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase - this command rebase the pr on top of main automatically

@okwasniewski

Copy link
Copy Markdown
ContributorAuthor

Hey @cipolleschi, is there anything more we need to get done in order to merge this PR?

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase - this comment automatically rebase on top of main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Jun 6, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in 756f89a.

@github-actions

Copy link
Copy Markdown

This pull request was successfully merged by @okwasniewski in 756f89a.

When will my fix make it into a release? | How to file a pick request?

kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
…eact#42330)
Summary:
This PR adds cocoapods utility to set `SWIFT_ACTIVE_COMPILATION_CONDITIONS` to DEBUG, which is set to this value by default (when generating a new native Xcode project).
This allows to use the `#if DEBUG` compilator directive in Swift to work out of the box, without any changes on user's side:
```swift
override func bundleURL() -> URL? {
#if DEBUG
RCTBundleURLProvider.sharedSettings().jsBundleURL(forBundleRoot: "index")
#else
Bundle.main.url(forResource: "main", withExtension: "jsbundle")
#endif
}
```
## Changelog:
[IOS] [ADDED] - Set SWIFT_ACTIVE_COMPILATION_CONDITIONS to DEBUG
Pull Request resolved: react#42330
Test Plan:
Run `bundle exec pod install` and check if the active compilation flags are populated:
![CleanShot 2024-01-17 at 13 11 03@2x](https://github.com/facebook/react-native/assets/52801365/68f119a6-af47-41bc-a1f1-9085fe4df6a0)
Reviewed By: cortinico
Differential Revision: D58188103
Pulled By: cipolleschi
fbshipit-source-id: 64746f3c7bfbdf47c2dea5e5e8cb2962635b719b
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.p: CallstackPartner: CallstackPartnerShared with MetaApplied 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.

4 participants

@okwasniewski@analysis-bot@cipolleschi@facebook-github-bot