GNU bug report logs - #39530
[PATCH] guix: Support partial download

Previous Next

To reply to this bug, email your comments to 39530 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: [PATCH] guix: Support partial download
Date: Sun, 9 Feb 2020 20:23:58 +0100

[Message part 1 (text/plain, inline)]

Hi Guix!

This patch adds support for partial download of fixed-output
derivations. I'm not very confident it can be pushed as-is, and it has
some shortcomings.

First, I make sure that the guix daemon will not remove previously
failed attempts when trying to build something again, when that is a
fixed-output derivation. Then, I add a Range HTTP header when
performing an HTTP fetch; this ensures that we only query for the part
we don't already have, and append it to the target file.

If a partial download fails, the same mirror/url is tried again, but
the partial file is removed first, ensuring we do a complete fetch this
time around. If that failed too, we try with the following url. If we
only perform a complete fetch, we proceed as usual. The next url will
be a partial fetch if there is already something locally.

The use-case is: I have a very unreliable wifi currently and when
downloading a big source (or substitute, but this patch doesn't address
that use-case), the connection is sometimes dropped in the middle and i
have to fetch everything from scratch. With this patch, the download
resumes.

Some issues that might need to be fixed: progress only shows for the
rest of the file, it would be nicer if it could start again where it
was before (say the connection dropped at 34%, then the progress bar
should start from 34%). When there are at least two urls it goes like
that: fetch a partial file, connection drops. Remove the file and try
again, connection drops. Go to the next mirror, fetch a partial file.
The first mirror restarted the download from the beginning, but we'd
like it not to, and skip to the following mirror instead. When there is
a hash mismatch, the file is fetched twice on a further attempt.

When testing locally with guix build -S ghostscript (and running the
daemon from ./pre-inst-env), the download went fine. Cancelling it in
the middle and restarting it did continue the download instead of
starting again, which is nice :).

However, with that daemon there was a lot of new builds required to run
guix environment guix as my user (and nothing was substituted, which
is weird), whereas with the system's daemon, there was nothing to
build. Maybe there's something fishy in that patch...

[0001-guix-download-Add-partial-download-support.patch (text/x-patch, attachment)]

Message #8 received at 39530 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 39530 <at> debbugs.gnu.org
Subject: Re: [bug#39530] [PATCH] guix: Support partial download
Date: Wed, 19 Feb 2020 17:04:32 +0100

Hi,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> First, I make sure that the guix daemon will not remove previously
> failed attempts when trying to build something again, when that is a
> fixed-output derivation. Then, I add a Range HTTP header when
> performing an HTTP fetch; this ensures that we only query for the part
> we don't already have, and append it to the target file.
>
> If a partial download fails, the same mirror/url is tried again, but
> the partial file is removed first, ensuring we do a complete fetch this
> time around. If that failed too, we try with the following url. If we
> only perform a complete fetch, we proceed as usual. The next url will
> be a partial fetch if there is already something locally.

Nice!

> However, with that daemon there was a lot of new builds required to run
> guix environment guix as my user (and nothing was substituted, which
> is weird), whereas with the system's daemon, there was nothing to
> build. Maybe there's something fishy in that patch...

Hmm, that sounds really weird.  Could you clarify what you did?

>>From 332793b7f29ea68ac9a1af22e3d1c4745200da7e Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> lepiller.eu>
> Date: Sun, 9 Feb 2020 19:47:27 +0100
> Subject: [PATCH] guix: download: Add partial download support.

Nitpick: you can remove “guix:” from the subject.

> * nix/libstore/build.cc (tryToBuild): Do not remove invalid fixed-output
> derivations.
> * guix/build/download.scm (http-fetch): Add a range argument.
> (url-fetch): Performa partial download if a file already exists.

[...]

> -(define* (http-fetch uri #:key timeout (verify-certificate? #t))
> +(define* (http-fetch uri #:key timeout (verify-certificate? #t) range)
>    "Return an input port containing the data at URI, and the expected number of
>  bytes available or #f.  When TIMEOUT is true, bail out if the connection could
>  not be established in less than TIMEOUT seconds.  When VERIFY-CERTIFICATE? is
> -true, verify HTTPS certificates; otherwise simply ignore them."
> +true, verify HTTPS certificates; otherwise simply ignore them.  When RANGE is
> +a number, it is the number of bytes we want to skip from the data at URI;
> +otherwise the full document is requested."

I’d suggest to rename #:range to #:offset because it denotes the start
offset.

What response do we get if the server doesn’t support “Range”?

Can servers silently ignore “Range”?

> +                      (if (file-exists? file)
> +                        (http-fetch uri
> +                                    #:verify-certificate? verify-certificate?
> +                                    #:timeout timeout
> +                                    #:range (stat:size (stat file)))
> +                        (http-fetch uri
> +                                    #:verify-certificate? verify-certificate?
> +                                    #:timeout timeout))))

I’d remove the ‘if’:

  (http-fetch …
              #:offset (and=> (stat file #f) stat:size))

> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -1320,6 +1320,7 @@ void DerivationGoal::tryToBuild()
>          Path path = i->second.path;
>          if (worker.store.isValidPath(path)) continue;
>          if (!pathExists(path)) continue;
> +	if (fixedOutput) continue;

Please add a comment above explaining why fixed outputs are not deleted.

Also please: not tabs.  :-)

Thanks!

Ludo’.

Message #11 received at 39530 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 39530 <at> debbugs.gnu.org
Subject: Re: [bug#39530] [PATCH] guix: Support partial download
Date: Wed, 19 Feb 2020 11:25:05 -0500

Le 19 février 2020 11:04:32 GMT-05:00, "Ludovic Courtès" <ludo <at> gnu.org> a écrit :
>Hi,
>
>Julien Lepiller <julien <at> lepiller.eu> skribis:
>
>> First, I make sure that the guix daemon will not remove previously
>> failed attempts when trying to build something again, when that is a
>> fixed-output derivation. Then, I add a Range HTTP header when
>> performing an HTTP fetch; this ensures that we only query for the
>part
>> we don't already have, and append it to the target file.
>>
>> If a partial download fails, the same mirror/url is tried again, but
>> the partial file is removed first, ensuring we do a complete fetch
>this
>> time around. If that failed too, we try with the following url. If we
>> only perform a complete fetch, we proceed as usual. The next url will
>> be a partial fetch if there is already something locally.
>
>Nice!
>
>> However, with that daemon there was a lot of new builds required to
>run
>> guix environment guix as my user (and nothing was substituted, which
>> is weird), whereas with the system's daemon, there was nothing to
>> build. Maybe there's something fishy in that patch...
>
>Hmm, that sounds really weird.  Could you clarify what you did?

Actually I'm not sure how to test the new daemon. I ran the guix daemon from a checkout and probably forgot to set sysconfdir to /etc, so it was missing authorized keys, hence no substitutes.

>
>>>From 332793b7f29ea68ac9a1af22e3d1c4745200da7e Mon Sep 17 00:00:00
>2001
>> From: Julien Lepiller <julien <at> lepiller.eu>
>> Date: Sun, 9 Feb 2020 19:47:27 +0100
>> Subject: [PATCH] guix: download: Add partial download support.
>
>Nitpick: you can remove “guix:” from the subject.
>
>> * nix/libstore/build.cc (tryToBuild): Do not remove invalid
>fixed-output
>> derivations.
>> * guix/build/download.scm (http-fetch): Add a range argument.
>> (url-fetch): Performa partial download if a file already exists.
>
>[...]
>
>> -(define* (http-fetch uri #:key timeout (verify-certificate? #t))
>> +(define* (http-fetch uri #:key timeout (verify-certificate? #t)
>range)
>>    "Return an input port containing the data at URI, and the expected
>number of
>>  bytes available or #f.  When TIMEOUT is true, bail out if the
>connection could
>>  not be established in less than TIMEOUT seconds.  When
>VERIFY-CERTIFICATE? is
>> -true, verify HTTPS certificates; otherwise simply ignore them."
>> +true, verify HTTPS certificates; otherwise simply ignore them.  When
>RANGE is
>> +a number, it is the number of bytes we want to skip from the data at
>URI;
>> +otherwise the full document is requested."
>
>I’d suggest to rename #:range to #:offset because it denotes the start
>offset.
>
>What response do we get if the server doesn’t support “Range”?
>
>Can servers silently ignore “Range”?
>
>> +                      (if (file-exists? file)
>> +                        (http-fetch uri
>> +                                    #:verify-certificate?
>verify-certificate?
>> +                                    #:timeout timeout
>> +                                    #:range (stat:size (stat file)))
>> +                        (http-fetch uri
>> +                                    #:verify-certificate?
>verify-certificate?
>> +                                    #:timeout timeout))))
>
>I’d remove the ‘if’:
>
>  (http-fetch …
>              #:offset (and=> (stat file #f) stat:size))
>
>> --- a/nix/libstore/build.cc
>> +++ b/nix/libstore/build.cc
>> @@ -1320,6 +1320,7 @@ void DerivationGoal::tryToBuild()
>>          Path path = i->second.path;
>>          if (worker.store.isValidPath(path)) continue;
>>          if (!pathExists(path)) continue;
>> +	if (fixedOutput) continue;
>
>Please add a comment above explaining why fixed outputs are not
>deleted.
>
>Also please: not tabs.  :-)
>
>Thanks!
>
>Ludo’.

Thanks!

Message #14 received at 39530 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 39530 <at> debbugs.gnu.org
Subject: Re: [bug#39530] [PATCH] guix: Support partial download
Date: Wed, 19 Feb 2020 23:07:31 +0100

Julien Lepiller <julien <at> lepiller.eu> skribis:

> Actually I'm not sure how to test the new daemon. I ran the guix daemon from a checkout and probably forgot to set sysconfdir to /etc, so it was missing authorized keys, hence no substitutes.

Ah yes.  This should work:

  sudo -E ./pre-inst-env guix-daemon --build-users-group=… …

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.