-
Notifications
You must be signed in to change notification settings - Fork 96
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 macports support #404
Add macports support #404
Conversation
@@ -6,6 +6,11 @@ install_osx() { | |||
vde cmake gnu-getopt coreutils zlib | |||
} | |||
|
|||
install_macports() { | |||
sudo port install pkgconfig pcre libpng libedit libsdl2 freetype libsdl2_ttf \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it's done for various other platforms, but I wonder if it is a good idea to have "sudo" in our scripts. It seems better to tell people to do the install their way, or tell them to run the script under sudo. That makes it explicit, rather than surprising people with doing privileged actions under the covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was reasonably harmless, as other architectures, such as Arch Linux and Debian also use sudo in their scripts. On the README.md file the instructions already tell the user that the deps.sh script should be invoked with sudo. If the user fails to invoke it with sudo, the script will call the installer using sudo and will prompt the user for a password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. It feels wrong, and I would view the existing "sudo" occurrences in scripts as defects to be fixed rather than conventions to apply in new work. Still, that can be treated as a separate task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can submit a patch later to fix all of them. I believe it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now issue #420 so if you want to take that on, that would be great.
Adds MacPorts as an option in the travis/deps.sh