Bug 228276 - [GTK][WPE] Organize list of package dependencies into separated files
Summary: [GTK][WPE] Organize list of package dependencies into separated files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Diego Pino
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-26 03:03 PDT by Diego Pino
Modified: 2021-07-27 04:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (46.13 KB, patch)
2021-07-26 03:12 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (45.55 KB, patch)
2021-07-26 05:33 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (46.00 KB, patch)
2021-07-26 17:58 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (46.00 KB, patch)
2021-07-26 17:59 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (45.52 KB, patch)
2021-07-27 03:53 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Pino 2021-07-26 03:03:30 PDT
[GTK][WPE] Organize list of package dependencies into separated files
Comment 1 Diego Pino 2021-07-26 03:12:04 PDT
Created attachment 434198 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2021-07-26 03:35:57 PDT
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?
Comment 3 Diego Pino 2021-07-26 03:56:19 PDT
(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.
Comment 4 Diego Pino 2021-07-26 05:33:15 PDT
Created attachment 434202 [details]
Patch
Comment 5 Adrian Perez 2021-07-26 13:52:48 PDT
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.
Comment 6 Diego Pino 2021-07-26 17:58:02 PDT
Created attachment 434262 [details]
Patch
Comment 7 Diego Pino 2021-07-26 17:59:13 PDT
Created attachment 434263 [details]
Patch
Comment 8 Adrian Perez 2021-07-27 03:26:36 PDT
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 :)
Comment 9 Diego Pino 2021-07-27 03:53:01 PDT
Created attachment 434280 [details]
Patch
Comment 10 EWS 2021-07-27 04:57:30 PDT
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].
Comment 11 Radar WebKit Bug Importer 2021-07-27 04:58:17 PDT
<rdar://problem/81157358>