Bug 110692 – [GTK] Move basic dependency installation to a script

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

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.

[Martin Robinson]

It might even make sense to simply include a PackageKit catalog in the repository.

[Martin Robinson]

Created attachment 189934 [details]
Patch

[Gustavo Noronha (kov)]

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 =)

[Tomas Popela]

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

[Gustavo Noronha (kov)]

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
> [phone]  Martin Robinson  <[e-mail]>

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 =)

[Tomas Popela]

Created attachment 190054 [details]
Repaired patch

Fixed proposed changes from Gustavo, added new dependencies for Fedora.

[Martin Robinson]

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

[Tomas Popela]

Created attachment 190065 [details]
v3

Fixed proposed changes from Martin

[Gustavo Noronha (kov)]

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
> [phone]  Martin Robinson  <[e-mail]>, Tomas Popela <[e-mail]>

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.

[Tomas Popela]

Created attachment 191225 [details]
v4

Fixed one bug, fixed Kov's recommendations..

[Gustavo Noronha (kov)]

Comment on attachment 191225 [details]
v4

Thanks!

[WebKit Review Bot]

Comment on attachment 191225 [details]
v4

Clearing flags on attachment: 191225

Committed r144780: <http://trac.webkit.org/changeset/144780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.