metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

chore(docs): Update ruby install instructions in environment setup readme

Open MajorLift opened this issue 1 year ago • 7 comments

Description

The current ruby install instructions in the "Environment Setup" readme are vague, and may cause gem to be run with the system root installation.

Screenshot 2024-10-09 at 9 13 29 AM

https://consensys.slack.com/archives/C02U025CVU4/p1728496388117329

This commit updates the instructions with commands that fix this issue.

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

  • [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

MajorLift avatar Oct 09 '24 18:10 MajorLift

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions[bot] avatar Oct 09 '24 18:10 github-actions[bot]

It is expected that a MetaMask contributor picks a version manager of their choice, and follows the version manager documentation to install and configure it on their system.

rbenv is mentioned as an example of a ruby version manager

We should keep from being prescriptive on how to configure tools that are not maintained by us.

joaoloureirop avatar Oct 17 '24 15:10 joaoloureirop

@joaoloureirop Would it help to add language emphasizing that the rbenv commands are examples? Or perhaps we could add more links to sections within the rbenv readme?

The primary target of this readme section probably isn't developers who have already set up ruby version managers themselves and don't need the instructions. Shouldn't it be a priority to unblock non-users of ruby by using the most obvious and unambiguous language possible?

MajorLift avatar Oct 17 '24 17:10 MajorLift

FWIW there is also the option of the repository Dockerfile:

# build image with bundled node/ruby/etc
$ podman build --build-arg UID=$(id -u) --build-arg GID=$(id -g) -t localhost/mm-mobile-builder -f scripts/docker/Dockerfile .

# run shell in container
$ podman run --rm -it --user $(id -u):$(id -g) --userns=keep-id:uid=$(id -u),gid=$(id -g) -v "$(pwd)":/app -w /app -v ~/.yarn:/home/node/.yarn -v ~/.cache/yarn:/home/node/.cache/yarn mm-mobile-build:debian bash

# run ruby directly
$ podman run --rm -it --user $(id -u):$(id -g) --userns=keep-id:uid=$(id -u),gid=$(id -g) -v "$(pwd)":/app -w /app mm-mobile-build:debian ruby --version

(taken to the next level: https://github.com/legobeat/l7-devenv)

Shouldn't it be a priority to unblock non-users of ruby by using the most obvious and unambiguous language possible?

I think it should also be a priority to steer developers towards secure solution and setups they can control. (No shade on rbenv or using it the proposed way)

I do think there are risks with the approach, though, specifically considering the undeterministic mess that CocoaPods have become.. By steering towards a specific blessed setup, we kind of take responsibility for that being Good Enough.

So kind of agree with @joaoloureirop

Would it help to add language emphasizing that the rbenv commands are examples? Or perhaps we could add more links to sections within the rbenv readme?

Both sound like nice improvements to me!

legobeat avatar Oct 17 '24 20:10 legobeat

@joaoloureirop Would it help to add language emphasizing that the rbenv commands are examples? Or perhaps we could add more links to sections within the rbenv readme?

We could expand this section to offer better guidance to new contributors unfamiliar with the tool.

The primary target of this readme section probably isn't developers who have already set up ruby version managers themselves and don't need the instructions. Shouldn't it be a priority to unblock non-users of ruby by using the most obvious and unambiguous language possible?

The concern is with new contributors getting familiar with this tool since it will require intervention every time our .ruby-version changes.

We want to help new contributors as much as possible, and in this case, the best help is redirecting them to the official documentation.

For example, if a new contributor decides to use the version manager mentioned in our docs they can follow the hyperlink provided and go through the installation guide, which is only two steps when using a package manager (brew install rbenv && rbenv init)

The most important part that is often overlooked is to set up the shell to load the version manager.

Down the road when intervention is needed (installing a new ruby version, rehashing, removing...), the official documentation is the best help they can get.

joaoloureirop avatar Oct 18 '24 12:10 joaoloureirop

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 54.54%. Comparing base (b0ef1a7) to head (385a936). Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11714      +/-   ##
==========================================
+ Coverage   54.27%   54.54%   +0.27%     
==========================================
  Files        1711     1743      +32     
  Lines       38712    39317     +605     
  Branches     4738     4870     +132     
==========================================
+ Hits        21010    21446     +436     
- Misses      16253    16376     +123     
- Partials     1449     1495      +46     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 18 '24 14:10 codecov-commenter

Closing as wont-fix.

MajorLift avatar Nov 13 '24 18:11 MajorLift