Bug 228276 – [GTK][WPE] Organize list of package dependencies into separated files

• Component: New Bugs
• Status: RESOLVED FIXED

[Diego Pino]

[GTK][WPE] Organize list of package dependencies into separated files

[Diego Pino]

Created attachment 434198 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 434198 [details]
Patch

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

> Tools/gtk/install-dependencies:55
> +    local packages=$(source $(dirname "$0")/dependencies/brew)

I don't think sourcing the other script is needed, just executing it should be enough, shouldn't it?

[Diego Pino]

(In reply to Carlos Alberto Lopez Perez from comment #2)
> Comment on attachment 434198 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434198&action=review
> 
> > Tools/gtk/install-dependencies:55
> > +    local packages=$(source $(dirname "$0")/dependencies/brew)
> 
> I don't think sourcing the other script is needed, just executing it should
> be enough, shouldn't it?

I didn't make the dependencies/* files executable, that's why sourcing is needed. But I can make them executable though.

One advantage of making the dependency files executable is that it can be an easy way which are the dependencies for a given platform, i.e: `$ Tools/gtk/dependencies/apt` should return all the APT package dependencies.

[Diego Pino]

Created attachment 434202 [details]
Patch

[Adrian Perez]

Comment on attachment 434202 [details]
Patch

I think this could be further simplified by making the “install-dependencies”
script define a PACKAGES array, and then source the file for the package manager
being used. Something like this:

  # ...
  declare -ga PACKAGES=()
  pkgmanager=$(guess-pkgmanager)  # Function defined elsewhere, returns e.g. "apt"
  source "$(dirname "$0")/dependencies/${pkgmanager}"

  install_with_apt () { : "TODO: Implement" }
  install_with_dnf () { : "TODO: Implement" }

  "install_with_${pkgmanager}" "${PACKAGES[@]}"  # Call one of the functions above

and then each script that lists packages adds them to the PACKAGES array:

  PACKAGES+=(
      autoconf
      automake
      $(aptIfElse libfoo-dev libfoo1-dev)
      ...
  )

  if something ; then
      PACKAGES+=(bar)
  endif

WDYT?

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

> Tools/gtk/install-dependencies:55
> +    local packages=$(`dirname "$0"`/dependencies/brew)

Using POSIX command expansion $(cmd) is preferred to `cmd` in backticks
because expansions can be nested. Also, backticks as used here won't work
well if `dirname "$0"` expands to a path that includes spaces. Using
double quotes around the inner expansion will make the command work even
in that case: 

    local packages
    packages=$("$(dirname "$0")/dependencies/brew")

Also, note how the “local” declaration is in separate line: if the command
expasion fails, it will go unnoticed because the “local” command always
succeeds—that's why it should go in two lines :)

(Yeah, I know, shell syntax is tricky!)

> Tools/gtk/install-dependencies:61
> +    local packages=$(`dirname "$0"`/dependencies/apt)

Same issue here.

> Tools/gtk/install-dependencies:75
> +    local packages=$(`dirname "$0"`/dependencies/pacman)

Same issue here.

> Tools/gtk/install-dependencies:102
> +    local packages=$(`dirname "$0"`/dependencies/dnf)

...and here.

> Tools/wpe/install-dependencies:85
> +    local packages=$(`dirname "$0"`/dependencies/dnf)

Here, too.

[Diego Pino]

Created attachment 434262 [details]
Patch

[Diego Pino]

Created attachment 434263 [details]
Patch

[Adrian Perez]

Comment on attachment 434263 [details]
Patch

Much nicer, thanks for updating the patch! I have left a couple of comments more
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:

   "installDependenciesWith${packageManager^}"

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 expansion:

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

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

[Diego Pino]

Created attachment 434280 [details]
Patch

[EWS]

Committed r280339 (239986@main): <https://commits.webkit.org/239986@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434280 [details].

[Radar WebKit Bug Importer]

<rdar://problem/81157358>