[pacman-dev] [RFC] curl-based internal downloader

[Dan McGee]

On Sun, Jan 2, 2011 at 7:13 PM, Dave Reisner <d at falconindy.com> wrote:
> Greetings,
>
> I've been working on and off on a curl-based internal downloader to replace the
> libfetch based implementation. Why?
>
> - libcurl is more widely used in the arch repos. fetch is used for one and only
>  package: pacman.
> - support for compression over http connections. I see proposals for adding
>  this to fetch, but never any acceptance. Please correct me if I'm mistaken.
> - personal bias. not really a reason, but I think the implementation is cleaner
>  and more readable than the fetch equivalent.
> - dan wanted me to. (the devil made me do it?)
>
> In it's current form, I'd say this patch set is 90% complete. I've been using it
> on top of pacman-git for about a week now and haven't come across any functional
> issues. I've done a fair bit of testing with ftp, http, and file based sync'ing.
>
> So what's remaining?
>
> - setting pm_errno: libfetch provides a global variable fetchLastErrString,
>  whereas curl returns error codes (CURLcode) from operations and you feed them
>  to curl_easy_strerror() to get a char* back. I'm not sure what the best way
>  is to get back an error string from curl when (what will be) PM_ERR_LIBCURL
>  is set.  could the global handle be used to store the CURLcode (it's just a
>  typedef'd long).
Yeah, on the handle probably works although that seems like a slight
crosscutting concern. Of course I don't have a much more spectacular
idea. The other option is to call easy_strerror() immediately after a
cURL error occurs and store a pointer to the string there instead of
the error code- but not sure of the memory conventions surrounding
that and if you need to free the string, who knows what.

And it doesn't matter one iota what the typedef is, because you are
going to use their type of course. :)

> - the progress bar does _not_ play well with curl. The bar itself is fine, but the
>  time remaining jumps around a fair bit. You'll see a fair bit of hackery in
>  curl_progress() (lib/libalpm/dload.c) where I'm fighting with the progress
>  bar. Perhaps this belongs in the progress bar voodoo itself?


> - my autoconf-fu sucks. Big time. I'm not sure what I've done is best practice,
>  but after applying patch 3/4, compiling without extra flags links in curl
>  and leaves out fetch. There is, however, no checking for both --with-curl and
>  --with-fetch being given, which will break the build.
Fixing this would be kinda cool so we can at least test both side by
side for a while; but I haven't looked near close enough to see what
is going on. I'd probably start by confusing yourself less and
renaming them to download_fetch and download_curl or something, rather
than the ifdef madness and only having one of them at a time. And you
could shoehorn another "temporary" patch in here that checked an env
var or something to see which internal function to use.

>
> Dan (and anyone else), I look forward to your tower of constructive criticism.
>
> dave
>
>  configure.ac        |   33 ++++---
>  lib/libalpm/alpm.c  |   13 +--
>  lib/libalpm/dload.c |  263 ++++++++++++++++++++++++++++++---------------------
>  3 files changed, 176 insertions(+), 133 deletions(-)