Bug 110692 - [GTK] Move basic dependency installation to a script
Summary: [GTK] Move basic dependency installation to a script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks: 110693
  Show dependency treegraph
 
Reported: 2013-02-23 10:43 PST by Martin Robinson
Modified: 2013-03-05 10:33 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.56 KB, patch)
2013-02-23 11:19 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2013-02-25 01:47 PST, Tomas Popela
gns: review-
Details | Formatted Diff | Diff
Repaired patch (4.76 KB, patch)
2013-02-25 07:26 PST, Tomas Popela
no flags Details | Formatted Diff | Diff
v3 (5.33 KB, patch)
2013-02-25 08:12 PST, Tomas Popela
gns: review-
Details | Formatted Diff | Diff
v4 (5.37 KB, patch)
2013-03-04 06:38 PST, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2013-02-23 10:43:14 PST
There are a lot of basic dependencies required to build and test WebKitGTK+. We should move those to a script so that it's easy to tell people how to install them.
Comment 1 Martin Robinson 2013-02-23 10:49:59 PST
It might even make sense to simply include a PackageKit catalog in the repository.
Comment 2 Martin Robinson 2013-02-23 11:19:42 PST
Created attachment 189934 [details]
Patch
Comment 3 Gustavo Noronha (kov) 2013-02-24 09:18:50 PST
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 =)
Comment 4 Tomas Popela 2013-02-25 01:47:44 PST
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 5 Gustavo Noronha (kov) 2013-02-25 05:52:03 PST
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 =)
Comment 6 Tomas Popela 2013-02-25 07:26:28 PST
Created attachment 190054 [details]
Repaired patch

Fixed proposed changes from Gustavo, added new dependencies for Fedora.
Comment 7 Martin Robinson 2013-02-25 07:54:48 PST
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
Comment 8 Tomas Popela 2013-02-25 08:12:07 PST
Created attachment 190065 [details]
v3

Fixed proposed changes from Martin
Comment 9 Gustavo Noronha (kov) 2013-03-04 06:06:12 PST
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.
Comment 10 Tomas Popela 2013-03-04 06:38:52 PST
Created attachment 191225 [details]
v4

Fixed one bug, fixed Kov's recommendations..
Comment 11 Gustavo Noronha (kov) 2013-03-05 10:08:39 PST
Comment on attachment 191225 [details]
v4

Thanks!
Comment 12 WebKit Review Bot 2013-03-05 10:33:02 PST
Comment on attachment 191225 [details]
v4

Clearing flags on attachment: 191225

Committed r144780: <http://trac.webkit.org/changeset/144780>
Comment 13 WebKit Review Bot 2013-03-05 10:33:05 PST
All reviewed patches have been landed.  Closing bug.