opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
800 stars 323 forks source link

Linux and MacOS Build Script Issue #3928

Closed alexbeattie42 closed 1 month ago

alexbeattie42 commented 1 month ago

Both the Linux and MacOS build scripts contain the following install step:

echo 'export PATH=~/opensim-core/bin:$PATH' >> ~/.bashrc
source ~/.bashrc

Problem

This causes the following problems:

  1. It assumes the person is using bash (which does not make sense on mac since the default shell is zsh).
  2. It will append the path update line to the .bashrc file every time the build script is run.

Solution

  1. Attempt to detect the shell the user is executing the script from or do what homebrew does and post a message about updating your shell .env script
  2. Check for the existence of this line in the specific .env file before attempting to add it

I am happy to submit a pull request for the script update if you would find it helpful.

nickbianco commented 1 month ago

@alexbeattie42, sure, feel free to submit a PR if you have some time!

alexbeattie42 commented 1 month ago

I submitted a PR that contains my idea for solving this. If the approach is suitable I can add zsh and test on mac as well.

halleysfifthinc commented 1 month ago

I think this issue is more indicative of the unconventional choices in the build script that require this brittle path modification. Ideally on Linux, opensim should be installed in a more traditional location (i.e. following FHS it should be /opt/ or /usr/local/), and then a link to the opensim-cmd binary placed in /usr/local/bin. I'm not sure what this should conventionally look like on MacOS, but I suspect installing to the home folder is also unusual regardless of OS.

(Also, putting a link to the opensim binary somewhere already on the PATH and installing opensim to a more traditional location are independent, so a simpler immediate fix would be to just replace the PATH modification with a link.)

alexbeattie42 commented 1 month ago

I agree. I have updated the PR to invoke the install script that exists in the install directory which creates the symbolic links. Does that look better?