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

Improve install instructions for macOS #414

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Conversation

rgov
Copy link
Contributor

@rgov rgov commented Dec 8, 2023

Summary

Several improvements to the install instructions for macOS including,

  • Using curl instead of wget, which is not built-in.
  • Unifying environment variable settings between Intel and Apple Silicon Macs.
  • Instructing the user how to install the Xcode Command Line Tools.
  • Directing users to the Homebrew documentation rather than duplicating it.
  • Prioritizing Zsh (the default shell) over Bash.
  • Partially addresses Change references from OS X to macOS  #106.

I've only made the changes to Harmonic so far, but once approved they can be applied to Ionic as well.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@rgov rgov requested a review from mabelzhang as a code owner December 8, 2023 04:19
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Dec 8, 2023
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! As you said, there are a lot of different changes in this pull request, some of which I am ready to approve immediately, but others will require more discussion. In particular, I am ready to immediately approve the following changes:

if you want to split these out into a separate pull request and propagate the changes to other documentation files, I'll be happy to review it

# qt@5
export CMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH}:/opt/homebrew/opt/qt@5
export CMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH:+$CMAKE_PREFIX_PATH:}`brew --prefix qt@5`
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you changed the syntax to ${CMAKE_PREFIX_PATH:+$CMAKE_PREFIX_PATH:}. I think it's easier to read the original syntax. I also think it's a little easier to read the $(...) instead of backticks

Suggested change
export CMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH:+$CMAKE_PREFIX_PATH:}`brew --prefix qt@5`
export CMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH}:$(brew --prefix qt@5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

${CMAKE_PREFIX_PATH:+$CMAKE_PREFIX_PATH:} is the unfortunate shell (Bash, Zsh) syntax for "append a : delimiter after the variable if it has a value". Otherwise if CMAKE_PREFIX_PATH is not set and you write ${CMAKE_PREFIX_PATH}:/opt/homebrew you get :/opt/homebrew.

@azeey azeey requested a review from scpeters June 10, 2024 18:40
@scpeters
Copy link
Member

@j-rivero I'll let you merge this since it may conflict with #451

@azeey azeey requested a review from j-rivero July 22, 2024 18:43
@azeey azeey enabled auto-merge (squash) August 9, 2024 16:25
@azeey azeey merged commit c7f945a into gazebosim:master Aug 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants