Re: Patch to tidy up wget use, and add return=4 when 404's are seen
On Mon, Mar 10, 2008 at 12:33:44AM +0100, Frans Pop wrote:
> On Friday 07 March 2008, Philip Hands wrote:
> > Frans Pop wrote:
> > > Or maybe use a switch to toggle between "continuation" and "full"
> > > retries. Continuation retries probably make sense for larger files
> > > (Packages comes to mind) for which the integrity can be checked
> > > (md5sum).
> > > Given the caveats in the wget manpage for -c, it may be safer to not do
> > > continuation retries at all, but just do a full retry in other cases
> > > (like fetching preseed files).
> >
> > OK, I've done -c as an option to fetch-url that adds a -c option to wget
> > for the 3 retries that it tries if the initial wget fails. I've also
> > made preseed_fetch accept options and pass them onto fetch-url, so you
> > can use -c in preseed scripts (for downloading udebs or debs, say)
>
> Hmm. That is not what I had in mind. Also, you now *always* run with -c if
> it is passed (even on the first try) and I don't think that is desirable
> (and even very dangerous).
Doh! That's not what I intended either -- well spotted :-)
The wget404 version does what you're suggesting (without the -r option),
so I think this was caused by me trying to tidy the intermediate patch
a little more than was wise.
> Let me show what I'm thinking in plain text^Wcode.
> This supports two separate options -c and -r that probably should not be
> used at the same time.
> I've thrown in preservation of the return code for free :-)
>
> protocol_fetch() {
> local i j RET
> [other initial stuff and proxy]
>
> RET=0
> for i in 1 2; do
> wget -q "$url" -O "$file" || RET=$?
> [ $RET != 0 ] || return 0
>
> if [ "$ALLOW_CONTINUES" = yes ]; then
> for j in 1 2 3; do
> wget -c -q "$url" -O "$file" || RET=$?
> [ $RET != 0 ] || return 0
> done
> fi
> [ "$DO_REPEAT" = yes ] || break
> done
> return $RET
> }
>
> > Here's the patch, split in two as you suggested so that the first half
> > is just the moving of fetch-url into di-utils:
> > http://hands.com/~phil/d-i/fetch-url.diff
>
> Looks good. Some comments.
>
> +++ b/packages/debian-installer-utils/fetch-url
> +while true ; do
>
> We normally don't do spaces before ;
Ah, old habit -- my personal programming tutor at Imperial used to be
somewhat strict about students using _his_ coding style (i.e. use his
style and get beween 50-100% otherwise try your luck with the full 0-100%
range ;-)
His opinion was that inserting a spare space before ; made it really
obvious that the ; was there -- I've always thought he had a point --
but if it is against d-i style I'll try to comply (but it might not work,
as that's built into my fingers)
> + case "$1" in
> + -c)
> + ALLOW_CONTINUES=yes
>
> Current coding style prefers 4 spaces before options and then a single tab
> for the code (saves an indentation level and in most cases also bytes):
OK
> + case "$1" in
> + -c)
> + ALLOW_CONTINUES=yes
>
> +++ b/packages/debian-installer-utils/fetch-url-methods/floppy
> + mountfloppy || true
> + touch /var/run/preseed-usedfloppy
> +
>
> Trailing spaces on the last line (also in original, but still :-).
fixed.
> +++ b/packages/debian-installer-utils/fetch-url-methods/http
> + [ "$ALLOW_CONTINUES" = "yes" ] && local WGET_C="-c"
>
> This breaks when run with 'set -e'.
>
> +++ b/packages/preseed/preseed_fetch
> +# eat options starting with a -, so we can pass them on
> +while expr "$1" : "-" >/dev/null ; do
>
> Doesn't this test for -'s anywhere in $1, not only at the start?
> And space before ; again.
No expr : patterns are tied to the front of the string (i.e. they
effectively have a ^ at the start)
>
> > and the second bit is the wget404 stuff:
> > http://hands.com/~phil/d-i/wget404.diff
>
> Similar minor coding style issues and main patch will need rewrite depending
> on final version of protocol_fetch.
>
> Cheers,
> FJP
OK, the next attempt:
http://hands.com/~phil/d-i/fetch-url-2.diff
http://hands.com/~phil/d-i/wget404-2.diff
and for completeness, the combination of those two diffs:
http://hands.com/~phil/d-i/fetch+wget-2.diff
Cheers, Phil.
P.S. I did something a little odd with git earlier -- while I'm 99%
certain I've got back to where I was, I've not had chance to do all the
tests to make sure -- please point out anything that looks out of place.
--
|)| Philip Hands [+44 (0)20 8530 9560] http://www.hands.com/
|-| HANDS.COM Ltd. http://www.uk.debian.org/
|(| 10 Onslow Gardens, South Woodford, London E18 1NE ENGLAND
Reply to: