On Sat, 28 Apr 2012, Tatsuhiro Tsujikawa wrote:

> I finally updated my 3year-old Metalink support patch to the curl master
> branch. The patch is attached.

Thanks Tatsuhiro! I think the time has come for us to get proper metalink
support added. Maybe not just yet though, see below...

> The Metalink support is enabled when configure option --with-libmetalink is
> used. It defaults to be disabled.

...

I fell into quite a number of problems immediately and I'd like to share them
with you (all):

  1 - building without libmetalink fails to compile since you've hidden a
      define (GETOUT_METALINK) the code used outside the #ifdefs. (see attached
      patch)

  2 - when (1) is fixed, the build still warns and since I build with -Werror
      that means failure. The header logic needs to be fixed.

  3 - I then proceeded and downloaded and intalled libmetalink in a custom
      install path to try it out. configure --with-libmetalink didn't work
      and I had to patch configure.ac first to properly use the custom path
      I gave it. (see second attached patch)

  4 - now I could run configure and it found the lib and it started to build
      only to fail as soon as it tried to build the libmetalink using code:

      metalink_types.h:50:1: error: function declaration isn¢t a prototype
        [-Werror=strict-prototypes]
      metalink_error.h:53:8: error: C++ style comments are not allowed in ISO
        C90 [-Werror]

      So I had to edit my Makefile to make it build, but it when spewed out
      warnings many times. I figure a fix would make curl use -isystem or
      something instead of -I for the includes when gcc is used, to make it
      inhibit the complaints.

  5 - when I had edited my Makefile I found more funny warnings:

      tool_metalink.c:733:5: warning: implicit declaration of function
        'easysrc_perform' [-Wimplicit-function-declaration]
      tool_metalink.c:733:5: warning: nested extern declaration of
        'easysrc_perform' [-Wnested-externs]

  6 - I don't think the --metalink option should be conditional, I would prefer
      it to output a friendly message in case curl was built without metalink
      support instead of just saying that the option is unknown.

> This adds --metalink option to curl.
> You can give Metalink file to curl like this:
>    $ curl -O --metalink foo.metalink

Is this really how you want to use it? I mean, how you a typical user get that
foo.metalink file in the first place? Also, what exactly does -O mean here?
What would the code do if we don't provide a -O for metalink?

> The functionality is very limited. It only tries next resource if the
> download is not successful (e.g., 404 Not found).

How did you come up with that logic? Since a metalink file is supposed to
specify existing and working URLs, wouldn't a tool then more or less be
expected to try the next link pretty much no matter which error you get? Or
put another way: why would getting a 5xx or a 3xx code not be subject for
another attempt?

> I admit that the patch is in very early stage. There are many duplicated
> code.

Yes. To such an extent that I really cannot see myself merging the code with
all this duplication. I think we need to come up with a way that avoid that
first.

> Maybe it would be better to integrate Metalink function to operate() in
> tool_operate.c but there is the difference in control flow (for example, in
> Metalink, we don't use URI globbing. Instead we need another loop through
> all resources in Metalink file) so we will need many if-else cause.

It is basically metalink instead of globbing so I don't think it would have to
become _that_ messy.

Ok, let me end with what happened when I eventually tried to run my new build:

$ ./src/curl -O --metalink metalink
ERROR: code=902
$ echo $?
1

The metalink file was downloaded from here:
http://curl.haxx.se/metalink.cgi?curl=tar.lzma

-- 
  / daniel.haxx.se

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-users
FAQ: http://curl.haxx.se/docs/faq.html
Etiquette: http://curl.haxx.se/mail/etiquette.html