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

Add supported Ruby versions and deprecate "sudo gem install" to system Ruby #1022

Merged
merged 11 commits into from
Feb 11, 2021

Conversation

ainame
Copy link
Contributor

@ainame ainame commented Feb 8, 2021

Closes #1018

Last year Ruby 3.0 was released and I started working on Ruby 3.0 migration project lately. I realised that not a few people pick up Ruby 3.0 as their first Ruby version to install and get errors. fastlane/fastlane#17931

Right now if you try gem install fastlane with Ruby 3.0, it results in getting version "2.54.1", which is too old. We've gotten several issues due to that. To clarify supported versions, I updated the installation guide page.

And also there is something bugging me always, which is recommending using "sudo". It's almost common sense among Ruby developers, which we should avoid using system's Ruby and should adopt some manager solution for local Ruby environment. So I tried updating that part as well.

I know Ruby environment for mobile app developers is not as important as for web developers but wanted to stop recommending system Ruby + "sudo" at least, which makes us even harder to track GitHub issues; e.g. fastlane/fastlane#15183 (comment)

In addition, Apple clearly mentioned that macOS won't include Ruby in future. We will need to admit this anyway.

Scripting language runtimes such as Python, Ruby, and Perl are included in macOS for compatibility with legacy software. Future versions of macOS won’t include scripting language runtimes by default, and might require you to install additional packages. If your software depends on scripting languages, it’s recommended that you bundle the runtime within the app. (49764202)

https://developer.apple.com/documentation/macos-release-notes/macos-catalina-10_15-release-notes

@google-cla google-cla bot added the cla: yes label Feb 8, 2021
@ainame ainame marked this pull request as ready for review February 8, 2021 06:49
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Really welcome changes! Left some copy improvements, links and some feedback you might want to consider 😊

Thanks for this ❤️

docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
ainame and others added 7 commits February 8, 2021 17:19
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
@ainame ainame requested a review from rogerluan February 8, 2021 17:23
@ainame
Copy link
Contributor Author

ainame commented Feb 8, 2021

@rogerluan Took all your suggestions and removed unnecessary whitespace🙂 Thank you for the review!

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Good stuff!! Cheers 🎉

@ainame
Copy link
Contributor Author

ainame commented Feb 9, 2021

We'll then need to replace inline messages in fastlane itself as well🙂
https://github.com/fastlane/fastlane/search?q=sudo+install

@rogerluan
Copy link
Member

Oh boy ☝️ 😅

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is a much needed change! Thanks for making this 😊

I added two small change requests to change the vibe of the sentences a little bit so let me know what you think!

Feel free to merge this on your own when ready 😊

docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
docs/includes/installing-fastlane.md Outdated Show resolved Hide resolved
ainame and others added 2 commits February 9, 2021 11:52
Co-authored-by: Josh “Now a dad so moving slower” Holtz <josh@rokkincat.com>
Co-authored-by: Josh “Now a dad so moving slower” Holtz <josh@rokkincat.com>
@ainame
Copy link
Contributor Author

ainame commented Feb 9, 2021

@joshdholtz Thank you very much for your review! I just applied your suggestions so, please ♻️

@rogerluan rogerluan dismissed joshdholtz’s stale review February 10, 2021 15:11

Requested changes were applied.

@rogerluan
Copy link
Member

rogerluan commented Feb 10, 2021

@ainame from Josh's last message I think he meant we can merge once those changes were applied 🎉 . netlify is failing to build this PR for some reason though 🤔 should we investigate that, or was it failing before as well?

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Josh's suggestions were 💯 😁

@rogerluan
Copy link
Member

Looks like the netlify issue is also present in master 😬 we can probably just merge this in then AFAIC

@joshdholtz
Copy link
Member

Yeah, Netlify isn't super important but I should look into that 😊 I think I have access to that account somewhere 😬

But merge away!

Click 👏 that 👏 button 👏

@ainame
Copy link
Contributor Author

ainame commented Feb 11, 2021

Ah sorry, I thought Josh requested change too. I'm going to merge this now and will work on the "sudo" instructions in code next 👍

@ainame ainame merged commit 440979e into master Feb 11, 2021
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.

Clarify supported Ruby versions
3 participants