Re: [Bug-wget] Libproxy support for wget

[Micah Cowan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dominique Leuenberger wrote:
> Hi everybody,
> 
> Sorry, it took a bit longer than I expected, but some other things just came 
> in between.
> 
> But finally, here it is: my patch which will add libproxy support to wget.
> Of course it's all configurable using configure and the wget parameters 
> --no-proxy and proxyrc have precedence.

Sorry it's taken me so long to respond, as well. I've finally had an
opportunity to review the patch; here are my thoughts.

. It looks like it may be missing the appropriate configure.ac or
Makefile.am directive to actually add libproxy for linking in wget?

. It looks like it will leak memory for "proxies" on every invocation.
Maybe you can copy proxies[0] to rewritten_storage, and then free up
proxies?

. The most important thing to me is maintaining backward-compatibility.
I gather that libproxy supports the typical environment variables; but
it's not clear to me whether they take precedence over proxies specified
elsewhere (in GNOME configuration, for instance), which I need to be the
case, or how compatible its env var support is with wget's, or if those
are both satisfactory right now, whether they can be guaranteed to be in
the future. I'd feel more comfortable letting getproxy's current code
have a go first, and then if it turns up nothing, fall back on libproxy.

. I'm trying to move away from #if and #ifdef within the body of
functions. Not much has actually been done yet to effect this move, but
it would be nice to maybe ensure that if HAVE_LIBPROXY isn't defined, it
gets defined to 0, and then change the #ifdef to a normal if?

. We'll want to advertise libproxy support in our output for --version;
please add an appropriate entry to build_info.c.in:

  ENABLE_LIBPROXY    libproxy

. It'd be great if you could improve wget.texi by updating its
description of how proxies work, when configured for use with libproxy.

. It would be _really_ great if libproxy could report where it got its
proxy from: not knowing makes debugging a PITA. Obviously, this has
nothing directly to do with your patch for Wget, but please consider
adding this to libproxy's API at some point.

. I think we should probably have a --no-libproxy option, and
(especially) a corresponding wgetrc option (see
http://wget.addictivecode.org/OptionsHowto for how to do those), so that
users have the option of using only Wget's historical behavior, if they
want to.

. One additional minor note: "available" is misspelled "availabel" in
configure.ac :)

Also, if I haven't mentioned it to you before: in order to accept your
changes into Wget proper, you'll need to have papers signed with the
Free Software Foundation (and possibly your employer, too). Would you be
willing to assign copyright over your changes to the FSF? If so, do you
want to assign copyright over just these specific changes, or do you
want to assign over "past and future changes" (which would allow you to
continue to contribute in the future, without having to go through the
paper process again)?

- --
Micah J. Cowan
Programmer, musician, typesetting enthusiast, gamer.
Maintainer of GNU Wget and GNU Teseq
http://micah.cowan.name/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrGNesACgkQ7M8hyUobTrF8IACfZpKeynnI/UiYtGUTsWzf4XCt
afsAn3i0Deb0N5o0t++YYuXdWx266TpW
=+2Ht
-----END PGP SIGNATURE-----