-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
[iOS] Enable hermes debugger by configuration type instead of configuration name #48174
[iOS] Enable hermes debugger by configuration type instead of configuration name #48174
Conversation
Hi @benhandanyan! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks for doing that and thanks for adding the tests!
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -197,6 +197,10 @@ def initialize(name, build_settings = {}, is_debug: false) | |||
def debug? | |||
return @is_debug | |||
end | |||
|
|||
def type | |||
@is_debug ? :debug : :release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cipolleschi I just noticed that I didn't explictly return
here and I wanted to flag it in case you think it should be added for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its fine! i personally prefer to have an explicit return, but Ruby automatically returns the last line of a function, and many people thinks that that's idiomatic Ruby.
I prefer not to do that, because not many people know Ruby to that degree of confidence, so being explicit makes the code more readable for everyone.
@cipolleschi merged this pull request in eda4f18. |
This pull request was successfully merged by @benhandanyan in eda4f18 When will my fix make it into a release? | How to file a pick request? |
…8193) Summary: While I was [working on fixing the iOS debugger logic](#48174) based on configuration name regex match, I wanted to know if other logic was also based on configuration names. I think I found and fixed the only other configuration name-based logic in the repo in this PR. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [IOS] [CHANGED] - Use configuration type when adding ndebug flag to pods in release Pull Request resolved: #48193 Test Plan: In a fresh react-native project, I added to the Podfile: ```ruby installer.aggregate_targets.each do |aggregate_target| aggregate_target.xcconfigs.each do |config_name, config_file| is_release = aggregate_target.user_build_configurations[config_name] == :release puts "aggregate_targets #{config_name} is_release: #{is_release}" end end installer.target_installation_results.pod_target_installation_results.each do |pod_name, target_installation_result| target_installation_result.native_target.build_configurations.each do |config| is_release = config.type == :release puts "target_installation_results #{config.name} is_release: #{is_release}" end end ``` to confirm my logic. It output the following: ``` aggregate_targets Release is_release: true aggregate_targets Local is_release: false ... target_installation_results Local is_release: false target_installation_results Release is_release: true ... ``` I also updated the applicable tests I could find for this logic. Reviewed By: cortinico Differential Revision: D67025325 Pulled By: cipolleschi fbshipit-source-id: 45d68ee86e3255d843275a72916883c8c4bbc13d
… name (#48174) Summary: Fixes an [issue](#48168) where only iOS configurations with "Debug" in the name are configured to use the hermes debugger. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [IOS] [FIXED] - Enable hermes debugger by configuration type instead of configuration name Pull Request resolved: #48174 Test Plan: Added new test scenarios that all pass: ``` ruby -Itest packages/react-native/scripts/cocoapods/__tests__/utils-test.rb Loaded suite packages/react-native/scripts/cocoapods/__tests__/utils-test Started Finished in 0.336047 seconds. ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 56 tests, 149 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 166.64 tests/s, 443.39 assertions/s ``` In a personal project with the following configurations: ``` project 'ReactNativeProject', { 'Local' => :debug, 'Development' => :release, 'Staging' => :release, 'Production' => :release, } ``` I added the following to my Podfile: ``` installer.pods_project.targets.each do |target| target.build_configurations.each do |config| puts "#{config.name} is debug? #{config.type == :debug}" end end ``` To confirm that my logic is correct: ``` Local is debug? true Development is debug? false Staging is debug? false Production is debug? false ``` Reviewed By: robhogan Differential Revision: D66962860 Pulled By: cipolleschi fbshipit-source-id: 7bd920e123c9064c8a1b5d45df546ff5d2a7d8be
… name (#48174) Summary: Fixes an [issue](#48168) where only iOS configurations with "Debug" in the name are configured to use the hermes debugger. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [IOS] [FIXED] - Enable hermes debugger by configuration type instead of configuration name Pull Request resolved: #48174 Test Plan: Added new test scenarios that all pass: ``` ruby -Itest packages/react-native/scripts/cocoapods/__tests__/utils-test.rb Loaded suite packages/react-native/scripts/cocoapods/__tests__/utils-test Started Finished in 0.336047 seconds. ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 56 tests, 149 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 166.64 tests/s, 443.39 assertions/s ``` In a personal project with the following configurations: ``` project 'ReactNativeProject', { 'Local' => :debug, 'Development' => :release, 'Staging' => :release, 'Production' => :release, } ``` I added the following to my Podfile: ``` installer.pods_project.targets.each do |target| target.build_configurations.each do |config| puts "#{config.name} is debug? #{config.type == :debug}" end end ``` To confirm that my logic is correct: ``` Local is debug? true Development is debug? false Staging is debug? false Production is debug? false ``` Reviewed By: robhogan Differential Revision: D66962860 Pulled By: cipolleschi fbshipit-source-id: 7bd920e123c9064c8a1b5d45df546ff5d2a7d8be
This pull request was successfully merged by @benhandanyan in 4ca9d72 When will my fix make it into a release? | How to file a pick request? |
Summary:
Fixes an issue where only iOS configurations with "Debug" in the name are configured to use the hermes debugger.
Changelog:
[IOS] [FIXED] - Enable hermes debugger by configuration type instead of configuration name
Test Plan:
Added new test scenarios that all pass:
In a personal project with the following configurations:
I added the following to my Podfile:
To confirm that my logic is correct: