Skip to content

#34214 for main #34271

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.

Sign up for GitHub

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

Closed
Kudo wants to merge 3 commits into from
Closed

#34214 for main #34271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11ee7b4
#34214 for main
Kudo Jul 25, 2022
8d58e91
Add fix_react_bridging_header_search_paths unit test
Kudo Jul 26, 2022
13dc565
fix test regression errors
Kudo Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions scripts/cocoapods/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,18 @@ def self.fix_library_search_paths(installer)
end
end

def self.fix_react_bridging_header_search_paths(installer)
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|
# For third party modules who have React-bridging dependency to search correct headers
config.build_settings['HEADER_SEARCH_PATHS'] ||= '$(inherited) '
config.build_settings['HEADER_SEARCH_PATHS'] << '"$(PODS_ROOT)/Headers/Private/React-bridging/react/bridging" '
config.build_settings['HEADER_SEARCH_PATHS'] << '"$(PODS_CONFIGURATION_BUILD_DIR)/React-bridging/react_bridging.framework/Headers" '
end
end
end

Comment on lines +96 to +107
Copy link
Copy Markdown
Collaborator

@tido64 tido64 Jul 25, 2022

Choose a reason for hiding this comment

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

@Kudo, thanks for all your hard work with this. I'm curious why this is needed on main. Did we not fix the .podspec files? This looks wrong since it adds private headers to the search path among other paths that should've been added by CocoaPods if the .podspec was fixed.

Copy link
Copy Markdown
Collaborator

@tido64 tido64 Jul 25, 2022

Choose a reason for hiding this comment

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

It seems like React-bridging.podspec does not have public_header_files set. Would that have fixed the underlying issue?

Copy link
Copy Markdown
Contributor Author

@Kudo Kudo Jul 25, 2022

Choose a reason for hiding this comment

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

@tido64 this is for third party libraries integration, e.g. react-native-vision-camera. originally, my workaround was for React-bridging errors in use_frameworks mode and i added the header search paths to ReactCommon. it turns out some third party modules have dependencies to ReactCommon or React would also need the header search paths. that's why i am proposing to add them from CocoaPods post_install.

these are all workarounds 😔 in the future, we should remove the nested header imports, e.g. #include <react/bridging/Bridging.h> to support use_frameworks mode.

def self.apply_mac_catalyst_patches(installer)
# Fix bundle signing issues
installer.pods_project.targets.each do |target|
Expand Down
1 change: 1 addition & 0 deletions scripts/react_native_pods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ def react_native_post_install(installer, react_native_path = "../node_modules/re

ReactNativePodsUtils.exclude_i386_architecture_while_using_hermes(installer)
ReactNativePodsUtils.fix_library_search_paths(installer)
ReactNativePodsUtils.fix_react_bridging_header_search_paths(installer)
ReactNativePodsUtils.set_node_modules_user_settings(installer, react_native_path)

NewArchitectureHelper.set_clang_cxx_language_standard_if_needed(installer)
Expand Down