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

Exclude i386 from valid architectures when building with Hermes on iOS #30592

Closed
wants to merge 3 commits into from
Closed

Exclude i386 from valid architectures when building with Hermes on iOS #30592

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
c9416da
exclude i386 from architectures
grabbou Dec 15, 2020
ea664b1
remove extra changes
grabbou Dec 15, 2020
5954bf8
chore: tweaks
grabbou Dec 15, 2020
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
21 changes: 18 additions & 3 deletions scripts/react_native_pods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ def use_flipper!(versions = {}, configurations: ['Debug'])
pod 'FlipperKit/FlipperKitNetworkPlugin', versions['Flipper'], :configurations => configurations
end

def has_pod(installer, name)
installer.pods_project.pod_group(name) != nil
end

# Post Install processing for Flipper
def flipper_post_install(installer)
installer.pods_project.targets.each do |target|
Expand All @@ -109,23 +113,34 @@ def flipper_post_install(installer)
end
end

def react_native_post_install(installer)
def exclude_architectures(installer)
projects = installer.aggregate_targets
.map{ |t| t.user_project }
.uniq{ |p| p.path }
.push(installer.pods_project)

arm_value = `/usr/sbin/sysctl -n hw.optional.arm64 2>&1`.to_i

# Hermes does not support `i386` architecture
excluded_archs_default = has_pod(installer, 'hermes-engine') ? "i386" : ""

projects.each do |project|
project.build_configurations.each do |config|
if arm_value == 1 then
config.build_settings.delete("EXCLUDED_ARCHS[sdk=iphonesimulator*]")
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = excluded_archs_default
else
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64"
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 " + excluded_archs_default
end
end

project.save()
end
end

def react_native_post_install(installer)
if has_pod(installer, 'Flipper')
flipper_post_install(installer)
end

exclude_architectures(installer)
end
5 changes: 0 additions & 5 deletions template/ios/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,5 @@ target 'HelloWorld' do

post_install do |installer|
react_native_post_install(installer)

# Enables Flipper.
#
# Disable the next line if you are not using Flipper.
flipper_post_install(installer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I could detect when flipper_post_install is placed in a user Podfile and print a warning that this is deprecated and one should place react_native_post_install instead.

Is it worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(just thinking about potential users that might forget to update the Podfile)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it might make some users happy to do some Podfile 🧹

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The technique would be this:

  1. Make flipper_post_install accept a second optional argument
  2. Pass second argument from within react_native_pods.rb, e.g. flipper_post_install(installer, true)
  3. When second argument is not defined, print a warning that users should replace it with react_native_post_install (second argument will not be present unless it is being run by "us")

Not sure if hacky/worth it, but figured I'll just leave it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can do this in a follow-up PR, if at all.

end
end