Re: [Bug-wget] Building and testing wget 1.16.1 on MinGW

[Tim Ruehsen]

Hi Eli,

On Friday 19 December 2014 17:02:53 Eli Zaretskii wrote:
> > From: Tim Ruehsen <address@hidden>
> > Date: Fri, 19 Dec 2014 12:53:13 +0100
> >
> > >      warc.c: In function 'warc_write_warcinfo_record':
> > >      warc.c:677:3: warning: passing argument 1 of 'strdup' makes pointer
> > >
> > > from integer without a cast [enabled by default] In file included from
> > > ../lib/string.h:27:0,
> > >
> > >                 from
> > >
> > > d:\usr\bin\../lib/gcc/mingw32/4.7.2/../../../../include/winnt.h:37, from
> > > d:\usr\bin\../lib/gcc/mingw32/4.7.2/../../../../include/windef.h:253,
> > > from
> > > d:\usr\bin\../lib/gcc/mingw32/4.7.2/../../../../include/windows.h:48,
> > > from
> > > mswindows.h:44,
> > >
> > >                 from sysdep.h:98,
> > >                 from wget.h:47,
> > >
> > >                 from warc.c:34:
> > >      d:
\usr\bin\../lib/gcc/mingw32/4.7.2/../../../../include/string.h:92:39:
> > > note: expected 'const char *' but argument is of type 'int'
> > >
> > >    This is because warc.c includes libgen.h only on non-Windows
> > >    systems, and the prototype for basename is declared on MinGW's
> > >    libgen.h.
> > >
> > >    Proposed solution:
> > > --- src/warc.c~0  2014-12-02 09:49:37.000000000 +0200
> > > +++ src/warc.c    2014-12-19 12:16:25.827125000 +0200
> > > @@ -54,10 +54,11 @@ as that of the covered work.  */
> > >
> > >  #include <uuid.h>
> > >  #endif
> > >
> > > -#ifndef WINDOWS
> > > -#include <libgen.h>
> > > -#else
> > > -#include <fcntl.h>
> > > +#if !defined WINDOWS || defined __MINGW32__
> > > +# include <libgen.h>
> > > +#endif
> > > +#ifdef WINDOWS
> > > +# include <fcntl.h>
> > >
> > >  #endif
> > >
> > >  #include "warc.h"
> >
> > The man page for basename says that there is a POSIX-2001 and a GNU
> > version. The POSIX version does not allow string literals - the function
> > writes into the argument string :-(
>
> The MinGW implementation indeed writes into its argument string.
> However, warc.c already handles this contingency, and works on a copy
> of the argument passed to warc_write_record.  So I see no problem
> here.
>
> > So I guess we should take basename from gnulib to have a consistent (and
> > graceful) behavior.
>
> See above: I don't see the need.
>
> Moreover, I'm not sure we are talking about the same problem.  Note
> that the compiler warning is about an argument being an 'int' whereas
> strdup expected a 'char *'.  This is due to implicitly assuming the
> return value is an 'int' when there's no prototype.  So the issue
> isn't 'const char *' vs 'char *', the issue is that there was no
> prototype for the compiler to use.

Right, we have two issues. But using basename from gnulib increases overall
compatibility *AND* also fixes your issue regarding libgen.h. We simply do not
need it any more when using gnulib.

The only question I have, is if we can drop #include <fcntl.h> for WINDOWS ?

I prepared a patch... please give it a try (you need ./bootstrap and
./configure after applying) and report back.


> > Commands work either way:
> > remoteencoding = UTF-8
> > remote-encoding = UTF-8
> > remote_encoding = UTF-8
>
> This is not documented anywhere I could see (I suggest to document
> that, perhaps in the same example file, if not in the Info manual).
> In any case, the documented forms of the options use the underscore
> '_', so I think so should the example file, for didactic reasons if
> nothing else.

ACK. Either it becomes documented or the example wgetrc should use the
documented names.


> > > +#httpsonly = off  ??? doesn't seem to exist
> > Only exists when HAVE_SSL is defined.
>
> I have HAVE_SSL defined, see the configure report I posted.  But the
> main issue here is that this option is not documented in the manual,
> AFAICS.

From 'man wget' (man page and info page generated from wget.texi):
       --https-only
           When in recursive mode, only HTTPS links are followed.


> Btw, to debug the Test-N issue, I had to add the time stamps being
> compared to the Perl script that runs the test.  It would be nice to
> have that in the tests by default, because just by looking at the time
> stamps, one can immediately understand in what direction to look for
> the problem (in my case I saw a difference of 3600 sec).

Since I am not 100% sure what you are talking about and you already extended
the (or a) Test-N, please send us diff/patch.

Tim

0001-gnulib-Use-basename-from-gnulib-module-dirname.patch
Description: Text Data

signature.asc
Description: This is a digitally signed message part.