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.

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

fix: doctor for 0.72 Ruby changes #1854

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

blakef
Copy link
Contributor

@blakef blakef commented Mar 3, 2023

Summary:

React Native 0.72.0 requires support for a range of Ruby version.

Fixes:

  • We were using the version of gem instead of ruby to validate against. Moved to ruby.
  • Supports Gemfiles and .ruby-version (as a fallback) and finally the version hardcoded into doctor.
  • Supports more error cases

Other:

  • Added unit tests
  • Added some inline documentation about why React Native has gone back to supporting system Ruby (but includes a range over versions to support).

Examples:

Version doesn't match Gemfile requirements

CleanShot 2023-03-03 at 17 39 11

No Gemfile

CleanShot 2023-03-03 at 17 22 52

No Gemfile for .ruby-version file

CleanShot 2023-03-03 at 17 22 24

Test Plan:

I ran this against a local project, adjusting variations of the Gemfile and .ruby-version.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi there, I left some comments mostly on the documentation part. I'll leave the technical review to someone who is more TS-savvy then me! :D

packages/cli-doctor/src/tools/healthchecks/ruby.ts Outdated Show resolved Hide resolved
packages/cli-doctor/src/tools/healthchecks/ruby.ts Outdated Show resolved Hide resolved
packages/cli-doctor/src/tools/healthchecks/ruby.ts Outdated Show resolved Hide resolved
// - Apple may drop support for scripting language runtimes in future version of
// macOS [4]. Ruby 2.7 is effectively EOL, which means many supporting tools and
// developer environments _may_ not support it going forward, and 3.0 is becoming
// the default in, for example, places like our CI. Some users may be unable to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// the default in, for example, places like our CI. Some users may be unable to
// the default in, for example, places like our CI. Some users may be unable to

That's not entirely true. CircleCI still supports Ruby 2.7.7... we don't know what will happens in April though.

// the default in, for example, places like our CI. Some users may be unable to
// install Ruby 2.7 on their devices as a matter of policy.
//
// - Our Codegen is extensively built in Ruby 2.7.
Copy link
Contributor

Choose a reason for hiding this comment

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

I won't callout Codegen specifically. I'd say that we have some build scripts for iOs that are written in ruby and we used Ruby 2.7 to develop them.

Managers: ['CocoaPods', 'RubyGems'],
Languages: ['Java'],
Managers: ['CocoaPods'],
Languages: ['Java', 'Ruby'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This change was also discussed in : #1825

Whether we need RubyGems in Managers or Ruby in Languages

Copy link
Contributor Author

@blakef blakef Mar 3, 2023

Choose a reason for hiding this comment

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

Thanks @arushikesarwani94, that thread gives great context: #1818 (comment)

I think this tackles some of the issues raised by focusing on the Gemfile. Interestingly I'm not sure it's useful to consider the .ruby-version at all, even as a backup.

It has given me some more to think about 🧐

Tasks

  • Can we build without a Gemfile (I don't think so)
  • We could try fetch the actual requirement from the tagged releases, but I see lots of avenues for failure here (it's just tricky finding a way to grab the correct version for the matching version of RN - 2 network requests there at a minimum).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we build without a Gemfile (I don't think so)

We can but we would not have any guarantee on the version of Cocoapod used by the users. So, if they have an old version of Cocoapods installed in the current Ruby, they will be using that which may be not compatible with React Native.

So, I highly discourage that we don't use Gemfile.

@adamTrz
Copy link
Collaborator

adamTrz commented Mar 3, 2023

Hi @blakef thanks for PR 💯
I'll try to review it shortly, one thing that comes in mind tho right now - IIRC ruby-version file will be removed in 0.72 via this PR so don't think we can care about it if doctor change is targeting 72? WDYT?

@blakef
Copy link
Contributor Author

blakef commented Mar 3, 2023

Hi @blakef thanks for PR 💯 I'll try to review it shortly, one thing that comes in mind tho right now - IIRC ruby-version file will be removed in 0.72 via this PR so don't think we can care about it if doctor change is targeting 72? WDYT?

I touched on this a little when responding to @arushikesarwani94's comment. Originally I was concerned about people using the doctor on older projects, but now thinking about it they still have the Gemfile (which typically would have just loaded the value from the .ruby-version).

TL;DR: agree, we should pull out all references to .ruby-version.

@blakef blakef requested review from cortinico and removed request for thymikee and adamTrz March 7, 2023 09:40
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Accepting to unblock you @arushikesarwani94 but there are still a couple of open points from @cipolleschi that needs to be addressed

The new release requires support for a range of Ruby version, which are
defined in the project Gemfile. This change add support for validating
against an in project Gemfile (or .ruby-version if it can't find one).
It finally falls back on the baked in version with the CLI.

There are also additional unit tests and some background comments.
@blakef
Copy link
Contributor Author

blakef commented Mar 7, 2023

I've reverted the update to the test environment, since the current release (0.71.3) pins Ruby to 2.7.6. Once 0.72.0 is released, that should be changed to 2.6.10.

@adamTrz adamTrz merged commit 43822c3 into react-native-community:main Mar 8, 2023
@blakef blakef deleted the fix-ruby-doctor branch March 8, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants