-
Notifications
You must be signed in to change notification settings - Fork 902
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
Conversation
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.
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
// - 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 |
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.
// 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. |
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 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'], |
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.
This change was also discussed in : #1825
Whether we need RubyGems
in Managers or Ruby
in Languages
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.
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).
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.
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.
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 TL;DR: agree, we should pull out all references to |
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.
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.
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. |
Summary:
React Native 0.72.0 requires support for a range of Ruby version.
Fixes:
gem
instead ofruby
to validate against. Moved toruby
.Gemfiles
and.ruby-version
(as a fallback) and finally the version hardcoded into doctor.Other:
Examples:
Version doesn't match Gemfile requirements
No Gemfile
No Gemfile for .ruby-version file
Test Plan:
I ran this against a local project, adjusting variations of the
Gemfile
and.ruby-version
.