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

Move use_flipper logic inside use_react_native and simplify the Flipper dependencies logic #33882

Closed
wants to merge 11 commits into from
Closed

Move use_flipper logic inside use_react_native and simplify the Flipper dependencies logic #33882

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c05cb76
Move use flipper logic inside use_react_native
f-meloni May 20, 2022
c3d8783
Add configuration for flipper
f-meloni May 20, 2022
fdd92c8
Fix tests
f-meloni May 20, 2022
387d626
Merge branch 'main' of https://github.com/facebook/react-native into …
f-meloni May 23, 2022
0bc843f
Disable flipper if USE_FRAMEWORK is enabled
f-meloni May 23, 2022
7be8a3e
Add production flag
f-meloni May 23, 2022
3e9545f
Update template
f-meloni May 23, 2022
543f1d8
Fix typo
f-meloni May 23, 2022
b2d71d5
Fix space from previous commit
f-meloni May 23, 2022
b3a0bc6
Add warning for use_flipper
f-meloni May 23, 2022
dc8ad2e
Merge branch 'main' of https://github.com/facebook/react-native into …
f-meloni May 24, 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
5 changes: 2 additions & 3 deletions packages/rn-tester/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ def pods(options = {})
path: @prefix_path,
fabric_enabled: fabric_enabled,
hermes_enabled: hermes_enabled,
flipper_configuration: USE_FRAMEWORKS ? FlipperConfiguation.disabled : FlipperConfiguation.enabled,
f-meloni marked this conversation as resolved.
Show resolved Hide resolved
app_path: "#{Dir.pwd}",
config_file_dir: "#{Dir.pwd}/node_modules",
production: !ENV['PRODUCTION'].nil?
)
pod 'ReactCommon/turbomodule/samples', :path => "#{@prefix_path}/ReactCommon"

Expand All @@ -48,9 +50,6 @@ end

target 'RNTester' do
pods()
if !USE_FRAMEWORKS
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
use_flipper!
end
end

target 'RNTesterUnitTests' do
Expand Down
13 changes: 2 additions & 11 deletions scripts/cocoapods/__tests__/flipper-test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,15 @@ def setup
# =========================== #
# TEST - Install Dependencies #
# =========================== #
def test_installFlipperDependencies_whenProductionIsFalse_installDependencies
def test_installFlipperDependencies_installDependencies
# Act
install_flipper_dependencies(false, '../..')
install_flipper_dependencies('../..')

# Assert
assert_equal($podInvocationCount, 1)
assert_equal($podInvocation['React-Core/DevSupport'][:path], "../../" )
end

def test_installFlipperDependencies_whenProductionIsTrue_skipDependencies
# Act
install_flipper_dependencies(true, '../..')

# Assert
assert_equal($podInvocationCount, 0)
assert_true($podInvocation.empty?)
end

# ======================= #
# TEST - Use Flipper Pods #
# ======================= #
Expand Down
26 changes: 22 additions & 4 deletions scripts/cocoapods/flipper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
#
# @parameter production: a boolean that indicates whether we are in production or not.
# @parameter pathToReactNative: the path to the React Native installation
def install_flipper_dependencies(production, pathToReactNative)
unless production
pod 'React-Core/DevSupport', :path => "#{pathToReactNative}/"
end
def install_flipper_dependencies(pathToReactNative)
pod 'React-Core/DevSupport', :path => "#{pathToReactNative}/"
end


Expand Down Expand Up @@ -92,3 +90,23 @@ def flipper_post_install(installer)
end
end
end

class FlipperConfiguation
f-meloni marked this conversation as resolved.
Show resolved Hide resolved
attr_reader :flipper_enabled
attr_reader :configurations
attr_reader :versions

def initialize(flipper_enabled, configurations, versions)
@flipper_enabled = flipper_enabled
@configurations = configurations
@versions = versions
end

def self.enabled(configurations = ["Debug"], versions = {})
return FlipperConfiguation.new(true, configurations, versions)
f-meloni marked this conversation as resolved.
Show resolved Hide resolved
end

def self.disabled
return FlipperConfiguation.new(false, [], {})
f-meloni marked this conversation as resolved.
Show resolved Hide resolved
end
end
10 changes: 8 additions & 2 deletions scripts/react_native_pods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def use_react_native! (options={})
# Include Hermes dependencies
hermes_enabled = options[:hermes_enabled] ||= false

flipper_configuration = options[:flipper_configuration] ||= FlipperConfiguation.disabled
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
f-meloni marked this conversation as resolved.
Show resolved Hide resolved

if `/usr/sbin/sysctl -n hw.optional.arm64 2>&1`.to_i == 1 && !RUBY_PLATFORM.include?('arm64')
Pod::UI.warn 'Do not use "pod install" from inside Rosetta2 (x86_64 emulation on arm64).'
Pod::UI.warn ' - Emulated x86_64 is slower than native arm64'
Expand All @@ -59,8 +61,6 @@ def use_react_native! (options={})
pod 'React-RCTVibration', :path => "#{prefix}/Libraries/Vibration"
pod 'React-Core/RCTWebSocket', :path => "#{prefix}/"

install_flipper_dependencies(production, prefix)

pod 'React-bridging', :path => "#{prefix}/ReactCommon/react/bridging"
pod 'React-cxxreact', :path => "#{prefix}/ReactCommon/cxxreact"
pod 'React-jsi', :path => "#{prefix}/ReactCommon/jsi"
Expand Down Expand Up @@ -108,6 +108,11 @@ def use_react_native! (options={})
pod 'libevent', '~> 2.1.12'
end

if flipper_configuration.flipper_enabled && !production
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary workaround that should fix #33764
If production is true the flipper pods are not added at all (differently than what CocoaPods does that compiles them, but then doesn't copy the frameworks), this avoids Flipper to be compiled when releasing, hence avoids the error discussed in the issue.
The downside of this workaround is that before making a release, the user needs to call pod install again

Copy link
Contributor

Choose a reason for hiding this comment

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

The downside of this workaround is that before making a release, the user needs to call pod install again

Correct me if I'm wrong, but it used to have to call it anyway, passing production = false, right?

I would also specify, perhaps also adding some doc, that it's still the case. When a user wants to build for release, it has to run PRODUCTION=1 pod install

Copy link
Contributor Author

@f-meloni f-meloni May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That is correct, however if there wasn't the mentioned issue we could only rely on CocoaPods' configurations logic to do esclude Flipper from production (it would anyway compile flipper but wouldn't copy it) and remove the production flag.
The PRODUCTION=1 pod install is how this is set up on RNTester, final users could potentially set their own way to define if is production or not depending on their needs and their workflow.
If we think setting the PRODUCTION env variable is a good default to express that we want the production flag true I can add that to the template as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment about that here? I think this can be easily missed since it's such a weird quirk of CocoaPods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry I missed this comment, will make another PR to fix those two comments as well

Copy link
Contributor Author

@f-meloni f-meloni May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Opened #33902

install_flipper_dependencies(prefix)
use_flipper_pods(flipper_configuration.versions, :configurations => flipper_configuration.configurations)
end

pods_to_update = LocalPodspecPatch.pods_to_update(options)
if !pods_to_update.empty?
if Pod::Lockfile.public_instance_methods.include?(:detect_changes_with_podfile)
Expand All @@ -122,6 +127,7 @@ def get_default_flags()
flags = {
:fabric_enabled => false,
:hermes_enabled => false,
:flipper_configuration => FlipperConfiguation.disabled
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
f-meloni marked this conversation as resolved.
Show resolved Hide resolved
}

if ENV['RCT_NEW_ARCH_ENABLED'] == '1'
Expand Down