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

Simplify SETUP.md for MacOS #57528

Merged
merged 15 commits into from
May 7, 2024
Merged

Simplify SETUP.md for MacOS #57528

merged 15 commits into from
May 7, 2024

Conversation

snickell
Copy link
Contributor

@snickell snickell commented Mar 26, 2024

Its probably easiest to read this PR with the markdown rendered, possibly as side-by-side before/after blocks:
Before | After

Changes:

  1. Don't suggest using Rosetta for Apple Silicon (not needed anymore, much faster)
  2. Shorter + less complex + visually clearer = easier to follow
  3. Use multi-line code blocks for macOS setup commands, as these are easier to single-click copy-paste, and perhaps less visually overwhelming:
    image
  4. Suggest the repo be checked out BEFORE doing platform-specific setup. Why? While this complicates git lfs slightly (requires an explicit git lfs pull), it means the instructions are actually valid / they work... since nvm install and rbenv install don't actually work until the repo is checked out (as they use .nvmrc and .ruby-version).
  5. brew install packages in one big block at the start (just like we do for apt-get install on ubuntu), rather than having to tediously paste in 20 brew install commands one by one as you come to each.
  6. Remove now unnecessary build hacks from "Apple Silicon (M1) or Intel Mac bundle install steps" section from the end.
  7. Move bundle exec rake install tips (equally relevant to all OSes) out of the linux-only section
  8. Install brew pdftk-java (which automatically pulls in a cpu-correct jdk) rather than complex instructions to install intel-only jdk8
  9. More consistently use the very nice <summary></summary> formatting for troubleshooting tips

Testing:

  • Instructions work on a fresh user account on an apple silicon macbook running macOS 14
    - [ ] Instructions work on a fresh user account on an x86 macbook (preferably running macOS 13) Update setup.md for an M3 mac #58321 removed x86 as supported
  • pdftk can still generated curriculum pdfs (how to test this?)

1. Clone the repo, which also may take a while.
- Note you should have `git lfs --version` >= 3.0 installed prior to cloning, see OS-specific install steps referenced above.
- The simplest option is to clone via SSH with: `git clone git@github.com:code-dot-org/code-dot-org.git`
- The fastest option is to clone via HTTP with: `git clone https://github.com/code-dot-org/code-dot-org.git`. Although faster than SSH, this option requires you to reauthenticate every time you want to update. You will therefore probably want to switch to SSH after the initial clone with `git remote set-url origin git@github.com:code-dot-org/code-dot-org.git`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now suggest cloning the repo before doing os-specific steps. While this requires adding a git lfs pull below, it means that nvm install and rbenv install actually work in the OS specific sections. i.e.: this ordering is actually correct!

Copy link
Contributor

Choose a reason for hiding this comment

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

should we list git lfs in the dependency check instructions above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

1. `gem install bundler -v 2.3.22`

1. `rbenv rehash`
1. `gem install bundler -v 2.3.22 && rbenv rehash`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Collapse gem install bundler and rbenv rehash together: they're really one command, as the rbenv rehash is there to make the new bundler command "take".

<summary>Troubleshoot: `rake aborted! Gem::LoadError: You have already activated...` </summary>

- If you have issue `"rake aborted! Gem::LoadError: You have already activated rake 12.3.0, but your Gemfile requires rake 11.3.0."`, make sure you add `bundle exec` in front of the `rake install:hooks` command
</details>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bundle exec is now explicitly present in the command (in an older version SETUPers were left to add the bundle exec, so this troubleshooting tip was relevant if you forgot)

If, for any reason, you are forced to interrupt the `bundle exec rake install` command before it completes,
cd into dashboard and run `bundle exec rake db:drop` before trying `bundle exec rake install` again.
`bundle exec rake install` must always be called from the local project's root directory, or it won't work.
</details>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section comes from the linux-specific instructions. Its a good tip, but its not just linux-specific, so here we make it available to all platforms.

1. Install necessary plugins described in the [Editor configuration](#editor-configuration) section below.

After setup, read about our [code styleguide](./STYLEGUIDE.md), our [test suites](./TESTING.md), or find more docs on [the wiki](https://github.com/code-dot-org/code-dot-org/wiki/For-Developers).
After setup, [configure your editor](#editor-configuration), read about our [code styleguide](./STYLEGUIDE.md), our [test suites](./TESTING.md), or find more docs on [the wiki](https://github.com/code-dot-org/code-dot-org/wiki/For-Developers).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its kind of ugly to have a not-very-clear "setup your editor" section included as a step: this step is unlike the other explicit commands, and it detracts from the "SETUP.md got dashboard running!" nice ending to the steps. I think its more of a follow-on, and its not really required anyway.

SETUP.md Outdated
@@ -104,102 +101,86 @@ External contributors can supply alternate placeholder values for secrets normal

### macOS

These steps are for Apple devices running **macOS Monterey and Ventura**, including those running on [Apple Silicon (M1|M2)](https://en.wikipedia.org/wiki/Apple_silicon#M_series).
Setup steps for Apple devices running **macOS Ventura and Sonoma**, including those running on [Apple Silicon](https://en.wikipedia.org/wiki/Apple_silicon#M_series) and Intel x86:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Include last two current macOS versions, explicitly mention that these instructions should (still) work on an x86 mac.

cd into dashboard and run `bundle exec rake db:drop` before trying `bundle exec rake install` again
1. `bundle exec rake install` must always be called from the local project's root directory, or it won't work.
1. Finally, don't worry if your versions don't match the versions in the overview if you're following this method; the installation should still work properly regardless
1. Return to the [overview](#overview) and continue setup.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This note was not linux specific, but its a great tip: its been moved to the shared setup instructions as a troubleshooting tip for bundle exec rake install

```


### ImageMagick with Pango
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started fresh, cleared my ~/.rbenv, and I'm pretty sure this is no longer necessary. Brew was messing up the openssl include and library path before (couple years past), but its straightened out now. Explicitly updating ffi is no longer required, our Gemfile.lock version is plenty fresh.



### ImageMagick with Pango
#### ImageMagick with Pango
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use the same #### column indent as other sections, so they don't become subsections of imagemagick with pango by accident

SETUP.md Outdated
@@ -104,102 +101,86 @@ External contributors can supply alternate placeholder values for secrets normal

### macOS
Copy link
Contributor Author

@snickell snickell Mar 26, 2024

Choose a reason for hiding this comment

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

macOS section

For the macOS specific section, the changes are extensive, and I think its much easier to look at the markdown rendered versions before and after side by side, rather than trying to stare through the diff noise:
Before | After

@snickell snickell marked this pull request as ready for review May 4, 2024 02:56
@snickell snickell merged commit 6dd379f into staging May 7, 2024
2 checks passed
@snickell snickell deleted the seth/no-rosetta branch May 7, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants