[Buildroot] [PATCH 02/13] core/pkg-download: change all helpers to use common options

[Yann E. MORIN <yann.morin.1998@free.fr>]

Arnout, All,

On 2017-07-05 01:09 +0200, Arnout Vandecappelle spake thusly:
> On 04-07-17 18:22, Maxime Hadjinlian wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > diff --git a/support/download/check-hash b/support/download/check-hash
> > index c1ff53c02b..bf2dbb1a8b 100755
> > --- a/support/download/check-hash
> > +++ b/support/download/check-hash
> > @@ -19,7 +19,7 @@ set -e
> >  #   3:  the hash file exists and there was no hash to check the file against
> >  #   4:  the hash file exists and at least one hash type is unknown
> >  
> > -while getopts :q OPT; do
> > +while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
> 
>  This doesn't make sense to me: the global DL_GETOPTS are never passed to
> check-hash, and you're not using any of them. So I'd keep the explicit :q here.

It took me a bit of time to understand what you meant, because
BR_BACKEND_DL_GETOPTS is actually exported by dl-wrapper, and it is
dl-wrapper that calls check-hash. So let me rephrase it, and you'll tell
me if I understood correctly:

    This doesn't make sense to me: none of the variabels from
    BR_BACKEND_DL_GETOPTS are ever passed to check-hash, so [...]

Right? If so I agree.

Regards,
Yann E. MORIN.

> >      case "${OPT}" in
> >      q)  exec >/dev/null;;
> >      \?) exit 1;;
> 
> [snip]
> > diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
> > index f944b71db5..a29411e0ae 100755
> > --- a/support/download/dl-wrapper
> > +++ b/support/download/dl-wrapper
> > @@ -19,6 +19,8 @@
> >  # We want to catch any unexpected failure, and exit immediately.
> >  set -e
> >  
> > +export BR_BACKEND_DL_GETOPTS=":hb:o:H:rRq"
> 
>  Since this variable is defined here, it makes sense to also document the
> options here. In fact, to make it makes more sense to document them all here
> rather than in each individual backend. Of course, no need to go and remove it
> in each backend, but I would like to have then documented here. Especially since
> a later patch will also parse them here.
> 
> > +
> >  main() {
> >      local OPT OPTARG
> >      local backend output hfile recurse quiet
> > @@ -83,7 +85,10 @@ main() {
> >      # If the backend fails, we can just remove the temporary directory to
> >      # remove all the cruft it may have left behind. Then we just exit in
> >      # error too.
> > -    if ! "${OLDPWD}/support/download/${backend}" ${quiet} ${recurse} "${tmpf}" "${@}"; then
> > +    if ! "${OLDPWD}/support/download/${backend}" \
> > +            ${quiet} ${recurse} \
> 
>  I don't like it very much when options are mixed like this: either fill lines
> up completely, or do one option (or option combination, of course, like -o
> "...") per line. But since this will be changed again later it's not worth
> changing this patch.
> 
>  Regards,
>  Arnout
> 
> > +            -o "${tmpf}" "${@}"
> > +    then
> >          rm -rf "${tmpd}"
> >          exit 1
> >      fi
> [snip]
> 
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'