[Buildroot] [PATCH 02/13] core/pkg-download: change all helpers to use common options
Arnout Vandecappelle
arnout at mind.be
Wed Jul 5 00:38:26 UTC 2017
On 05-07-17 01:14, Yann E. MORIN wrote:
> 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 at 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 [...]
Yes, that's what I meant. I guess I don't formulate very clearly at 01:09 (much
less at 02:38...)
Regards,
Arnout
>
> 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
>
--
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
More information about the buildroot
mailing list