[webkit-reviews] review granted: [Bug 228276] [GTK][WPE] Organize list of package dependencies into separated files : [Attachment 434263] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 27 03:26:36 PDT 2021

Adrian Perez <aperez at igalia.com> has granted Diego Pino <dpino at igalia.com>'s
request for review:
Bug 228276: [GTK][WPE] Organize list of package dependencies into separated

Attachment 434263: Patch


--- Comment #8 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 434263
  --> https://bugs.webkit.org/attachment.cgi?id=434263

Much nicer, thanks for updating the patch! I have left a couple of comments
in case you feel like applying them before landing =)

View in context: https://bugs.webkit.org/attachment.cgi?id=434263&action=review

> Tools/wpe/install-dependencies:43
> +    source $(dirname "$0")/dependencies/$packageManager

Missing quoting around the path passed to source:

  source "$(dirname "$0")/dependencies/$packageManager"

> Tools/wpe/install-dependencies:45
> +    case "$packageManager" in

...and then this “case” statement won't be needed, you can replace it with:


which will expand to installDependenciesWithApt, installDependenciesWithDnf,
and so on. The caret in the ${foo^} expansion converts the first letter of
the value to uppercase =)

> Tools/wpe/install-dependencies:59
> +    apt-get install -y --no-install-recommends ${PACKAGES[@]}

Typically I would point out that there is quoting around the variable

  apt-get install -y --no-install-recommends "${PACKAGES[@]}"

but in this case it does not matter much because we know that package names
contain spaces or other characters that would be interpreted in some special
by the shell. Feel free to add the quoting before landing, but as said, it's
strictly needed here :)

More information about the webkit-reviews mailing list