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.

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

Jump to bottom

#34214 for main #34271

Closed
wants to merge 3 commits into from
Closed

#34214 for main #34271

Show file tree
Hide file tree
Changes from all commits
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
10 changes: 3 additions & 7 deletions scripts/cocoapods/__tests__/new_architecture-test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,8 @@ def prepare_CXX_Flags_build_configuration(name)
end

def prepare_pod_target_installation_results_mock(name, configs)
return PodTargetInstallationResultsMock.new(
:name => name,
:native_target => TargetMock.new(name, configs)
)
target = TargetMock.new(name, configs)
return TargetInstallationResultMock.new(target, target)
end

def prepare_installer_for_cpp_flags(xcconfigs, build_configs)
Expand All @@ -232,8 +230,6 @@ def prepare_installer_for_cpp_flags(xcconfigs, build_configs)
[
AggregatedProjectMock.new(:xcconfigs => xcconfigs_map, :base_path => "a/path/")
],
:target_installation_results => TargetInstallationResultsMock.new(
:pod_target_installation_results => pod_target_installation_results_map
)
:pod_target_installation_results => pod_target_installation_results_map
)
end
50 changes: 35 additions & 15 deletions scripts/cocoapods/__tests__/test_utils/InstallerMock.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,23 @@ class InstallerMock
attr_reader :aggregate_targets
attr_reader :target_installation_results

def initialize(pods_project = PodsProjectMock.new, aggregate_targets = [AggregatedProjectMock.new], target_installation_results: [])
InstallationResults = Struct.new(:pod_target_installation_results, :aggregate_target_installation_results)

def initialize(pods_project = PodsProjectMock.new, aggregate_targets = [AggregatedProjectMock.new],
pod_target_installation_results: {},
aggregate_target_installation_results: {})
@pods_project = pods_project
@aggregate_targets = aggregate_targets
@target_installation_results = target_installation_results

@target_installation_results = InstallationResults.new(pod_target_installation_results, aggregate_target_installation_results)
aggregate_targets.each do |aggregate_target|
aggregate_target.user_project.native_targets.each do |target|
@target_installation_results.pod_target_installation_results[target.name] = TargetInstallationResultMock.new(target, target)
end
end
pods_project.native_targets.each do |target|
@target_installation_results.pod_target_installation_results[target.name] = TargetInstallationResultMock.new(target, target)
end
end

def target_with_name(name)
Expand Down Expand Up @@ -168,20 +181,27 @@ def initialize(name, build_settings = {})
end
end

class TargetInstallationResultsMock
attr_reader :pod_target_installation_results

def initialize(pod_target_installation_results: {})
@pod_target_installation_results = pod_target_installation_results
end
end

class PodTargetInstallationResultsMock
attr_reader :name
class TargetInstallationResultMock
attr_reader :target
attr_reader :native_target

def initialize(name: "", native_target: TargetMock.new())
@name = name
attr_reader :resource_bundle_targets
attr_reader :test_native_targets
attr_reader :test_resource_bundle_targets
attr_reader :test_app_host_targets
attr_reader :app_native_targets
attr_reader :app_resource_bundle_targets

def initialize(target = TargetMock, native_target = TargetMock,
resource_bundle_targets = [], test_native_targets = [],
test_resource_bundle_targets = {}, test_app_host_targets = [],
app_native_targets = {}, app_resource_bundle_targets = {})
@target = target
@native_target = native_target
@resource_bundle_targets = resource_bundle_targets
@test_native_targets = test_native_targets
@test_resource_bundle_targets = test_resource_bundle_targets
@test_app_host_targets = test_app_host_targets
@app_native_targets = app_native_targets
@app_resource_bundle_targets = app_resource_bundle_targets
end
end
54 changes: 54 additions & 0 deletions scripts/cocoapods/__tests__/utils-test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,60 @@ def test_fixLibrarySearchPaths_correctlySetsTheSearchPathsForAllProjects
assert_equal(pods_projects_mock.save_invocation_count, 1)
end

# ============================================= #
# Test - Fix React-bridging Header Search Paths #
# ============================================= #

def test_fixReactBridgingHeaderSearchPaths_correctlySetsTheHeaderSearchPathsForAllTargets
# Arrange
first_target = prepare_target("FirstTarget")
second_target = prepare_target("SecondTarget")
third_target = TargetMock.new("ThirdTarget", [
BuildConfigurationMock.new("Debug", {
"HEADER_SEARCH_PATHS" => '$(inherited) "${PODS_ROOT}/Headers/Public" '
}),
BuildConfigurationMock.new("Release", {
"HEADER_SEARCH_PATHS" => '$(inherited) "${PODS_ROOT}/Headers/Public" '
}),
], nil)

user_project_mock = UserProjectMock.new("a/path", [
prepare_config("Debug"),
prepare_config("Release"),
],
:native_targets => [
first_target,
second_target
]
)
pods_projects_mock = PodsProjectMock.new([], {"hermes-engine" => {}}, :native_targets => [
third_target
])
installer = InstallerMock.new(pods_projects_mock, [
AggregatedProjectMock.new(user_project_mock)
])

# Act
ReactNativePodsUtils.fix_react_bridging_header_search_paths(installer)

# Assert
first_target.build_configurations.each do |config|
assert_equal(config.build_settings["HEADER_SEARCH_PATHS"].strip,
'$(inherited) "$(PODS_ROOT)/Headers/Private/React-bridging/react/bridging" "$(PODS_CONFIGURATION_BUILD_DIR)/React-bridging/react_bridging.framework/Headers"'
)
end
second_target.build_configurations.each do |config|
assert_equal(config.build_settings["HEADER_SEARCH_PATHS"].strip,
'$(inherited) "$(PODS_ROOT)/Headers/Private/React-bridging/react/bridging" "$(PODS_CONFIGURATION_BUILD_DIR)/React-bridging/react_bridging.framework/Headers"'
)
end
third_target.build_configurations.each do |config|
assert_equal(config.build_settings["HEADER_SEARCH_PATHS"].strip,
'$(inherited) "${PODS_ROOT}/Headers/Public" "$(PODS_ROOT)/Headers/Private/React-bridging/react/bridging" "$(PODS_CONFIGURATION_BUILD_DIR)/React-bridging/react_bridging.framework/Headers"'
)
end
end

# ================================= #
# Test - Apply Mac Catalyst Patches #
# ================================= #
Expand Down
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
Collaborator

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
Collaborator

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
Contributor Author

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