[bug#39530] [PATCH] guix: Support partial download

["Ludovic Courtès" <ludo@gnu.org>]

Hi,

Julien Lepiller <julien@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@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’.