-
Notifications
You must be signed in to change notification settings - Fork 926
use pod installed by bundler if possible #2669
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
base: main
Are you sure you want to change the base?
Conversation
export async function execaPod(args: string[], options?: execa.Options) { | ||
let podType: 'system' | 'bundle' = 'system'; | ||
try { | ||
await execa('bundle', ['exec', 'pod', '--version'], options); |
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.
It'd be quicker to run bundle show cocoapods
. That way it doesn't need to initialise cocoapods, it just asks bundler if it can resolve it.
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.
Whether it remains as an bundle exec pod --version
or becomes a bundle show cocoapods
;
Does this need to be more robust?
I'm asking because currently, it'd fail if bundle install
hasn't been run and would then fall back to the system pod even if it's meant to be using bundler.
If the first command fails you could potentially check the output of bundle check
to see if cocoapods
is listed as a missing gem?
But, this may be more complex than it needs to be? I'll defer to a project maintainer on that one. 😄
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.
I'm asking because currently, it'd fail if bundle install hasn't been run and would then fall back to the system pod even if it's meant to be using bundler.
Nice catch. This would move more users to installing cocoapods
globally, but it seems this is not the intended way since the template that is used by the cli is installing cocoapods
with bundler anyway (https://github.com/react-native-community/template/blob/main/template/Gemfile#L7).
It would be a breaking change in but I would love to have only the bundler
way.
try { | ||
await execa('pod', ['--version'], options); | ||
podType = 'system'; | ||
} catch (systemPodError) { | ||
throw new Error('cocoapods not installed'); | ||
} |
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.
we only want to fall back to pod
when bundle
is not installed, which you can check separately by trying to run bundle install
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.
I just pushed some changes. It will now try bundle install
first before falling back to pod
. Can you please have another look?
@buschco this is great start. Mind following up with a bit more robust logic please? |
e1a0ffe
to
d85ce6c
Compare
Summary
see #2668 and #2663 (should fix both).
This change ensures that local installed cocoapods (
bundle exec pod
) is used before system installedpod
.Test Plan
A: Bundled Version:
gem uninstall cocoapods
npx @react-native-community/cli@latest init MyApp
n
for cocoa pods questioncd MyApp
bundle install
Now
bundle exec pod --version
has an 0 exit code.npx react-native run-ios --force-pods
will not fail nor install cocoapods globally (pod
will have exit code > 0)B: Global Version (someone could argue this should never be supported anyway):
gem install cocoapods
npx @react-native-community/cli@latest init MyApp
n
for cocoa pods questioncd MyApp
cocoapods
fromGemfile
bundle install
Now
bundle exec pod --version
has an non 0 exit code.npx react-native run-ios --force-pods
will not fail and install cocoapods globally (pod
will have non 0 exit code)Checklist
react-native
checkout (instructions).