Summary: | [GTK] Move basic dependency installation to a script | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | gustavo, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 110693 | ||||||||||||||
Attachments: |
|
Description
Martin Robinson
2013-02-23 10:43:14 PST
It might even make sense to simply include a PackageKit catalog in the repository. Created attachment 189934 [details]
Patch
Comment on attachment 189934 [details]
Patch
Some people do not use sudo as the main ways of getting something to run as root, so I think it's a good idea to support a case in which the person decides for themselves how to become root, by which I mean we should use sudo only if the script is not already running as root. I think using a SUDO variable that is set to sudo if a test like if [ "$(id -u)" != "0" ] succeeds and to the empty string otherwise should do it, otherwise it's a great idea =)
Created attachment 190017 [details]
Patch
Split to functions, Fixed some typos, Fedora support (I need to double check it on fresh install) , check if running with root rights
Comment on attachment 190017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190017&action=review r- because the root check is not running the script as root with sudo and because the error message is now outdated, also a changelog nit > Tools/ChangeLog:1 > +2013-02-25 Martin Robinson <mrobinson@igalia.com> You should add yourself as co-author, here! > Tools/gtk/install-dependencies:6 > +if [[ $UID -ne 0 ]]; then > + echo "$0 must be run as root" > + exit 1 > +fi The script should use sudo to run itself if it's not yet running as root IMO. So you'd have sudo $0 inside this if. It should not have two of each of [ and ], too. > Tools/gtk/install-dependencies:11 > + echo "Currently this script only works for distributions supporting apt-get." > + echo "Please add support for your distribution: http://webkit.org/b/110693" Need to add yum here now =) Created attachment 190054 [details]
Repaired patch
Fixed proposed changes from Gustavo, added new dependencies for Fedora.
Comment on attachment 190054 [details] Repaired patch View in context: https://bugs.webkit.org/attachment.cgi?id=190054&action=review Thanks for improving my version! Just a couple nits here: > Tools/gtk/install-dependencies:3 > +# This script needs to be run with root rights Your comment is missing a period. > Tools/gtk/install-dependencies:9 > +function notSupported() { In WebKit we prefer functions names that start with verbs. This should probably be called printNotSupportedMessageAndExit. > Tools/gtk/install-dependencies:33 > +function checkInstaller { > +# apt-get - Debian based distributions > +apt-get --version &> /dev/null > +if [ $? -eq 0 ]; then > + aptInstall > + exit 0 > +fi > + > +# yum - Fedora > +yum --version &> /dev/null > +if [ $? -eq 0 ]; then > + yumInstall > + exit 0 > +fi > + > +notSupported > +} I'm curious why you didn't indent the contents of this function. > Tools/gtk/install-dependencies:35 > +function aptInstall { I recommend installDependenciesWithApt > Tools/gtk/install-dependencies:38 > +# These are dependencies necessary for building WebKitGTK+. > +apt-get install \ > + autoconf \ Indentation again. > Tools/gtk/install-dependencies:102 > +function yumInstall { installDependenciesWithYum Created attachment 190065 [details]
v3
Fixed proposed changes from Martin
Comment on attachment 190065 [details] v3 View in context: https://bugs.webkit.org/attachment.cgi?id=190065&action=review Small issues with the packages lists, but otherwise looks good to me, I'll r- since I think you're not a committer > Tools/ChangeLog:1 > +2013-02-25 Martin Robinson <mrobinson@igalia.com>, Tomas Popela <tpopela@redhat.com> Small nit: I think the usual way is to add an 'and' instead of a comma, also the syntax of ChangeLogs asks for two spaces between the name and the email address > Tools/gtk/install-dependencies:77 > + ruby \ The lists of packages have a trailing \ > Tools/gtk/install-dependencies:89 > + xvfb \ Here. > Tools/gtk/install-dependencies:138 > + perl-version \ Here. > Tools/gtk/install-dependencies:149 > + xorg-x11-server-Xvfb \ Here. Created attachment 191225 [details]
v4
Fixed one bug, fixed Kov's recommendations..
Comment on attachment 191225 [details]
v4
Thanks!
Comment on attachment 191225 [details] v4 Clearing flags on attachment: 191225 Committed r144780: <http://trac.webkit.org/changeset/144780> All reviewed patches have been landed. Closing bug. |