[sword-devel] Use of -Werror (cause found)

[Jonathan Marsden]

Jonathan Marsden wrote:

> I'll try a build of SWORD 1.6.0 with --enable-debug in a Jaunty 32bit
> (x86) VM here... hmmm, it works fine for me!  However, uncommenting the
> --enable-debug in debian/rules gets me FTBFS, on 32bit and 64bit Jaunty.
> 
> Dmitrij: This looks more and more like a packaging bug of some sort?

(1) This is not a packaging bug, it is apparently a SWORD build system bug!

(2) Doing

  tar zxf sword-1.6.0.tar.gz
  cd sword-1.6.0
  ./configure --enable-warnings=yes

will get me the same errors Dmitrij found, and that is on a nice plain
"unpack the tarball" build.  So this is *not* a packaging issue.

I conclude that the -Werror setting in --enable-debug is being overriden
later in the configure.ac by the code handling enable-warnings.  So, I
suspect that no-one has "really truly" been compiling SWORD with -Werror
recently, even when they think they have.

(3) OK, so Dmitrij found some unwanted warnings in the SWORD codebase.
Examining them in detail:

The warnings in src/mgr/filemgr.cpp being treated as errors are because
the code calls write() (the low level system call, write(2) ) but does
not check what it returns.  This is bad practice, because if the write
fails the code will never know it.  Something we are doing is causing
g++ to notice this and turn the warning into an error.

The code currently does:

  while (size > 0) {
    bytes = file->read(nibble, 32767);
    write(fd, nibble, (bytes < size)?bytes:size);
    size -= bytes;
  }

Note the complete lack of any error handling.  I suspect doing something
more like:

  while (size > 0) {
    bytes = file->read(nibble, 32767);
    size -= write(fd, nibble, (bytes < size)?bytes:size);
  }

may help, because this uses the return value of the write call.
Although if write fails now, and returns zero, this may loop
indefinitely.  I'm not going to restructure it to have full-on exception
handling or whatever, at least not right now :) :)

The whole thing looks to me as though it is slightly odd code, mixing a
C++ file object called file for the original file, with a POSIX file
descriptor called fd for the copy... and there are magic numbers like
32767 and later -77 sprinkled around the code.  This makes it harder
(for me) to read than it could be, so I'm only making minimal changes to it!

In the unix world, if you know the filename, I think you can simply do:

  truncate( filename, file->tellp() )

but I don't know if that is portable to all the other platforms SWORD
supports.  I suspect not.

(4) As a quick test, I tried out the first change I suggested in (3)
above, and then kept going until all warnings were eliminated from the
codebase.  The resulting code changes are in just two files,
src/mgr/filemgr.cpp and src/utilfuns/zlib/untgz.c -- at least if I
remove all the other files from src/utilfuns/zlib and use the newer zlib
that comes with Ubuntu (I wasn't going to spend my time patching up
warnings in an ancient convenience copy of a library... someone else can
if they want to -- it might be better to just remove it and use a newer
zlib instead, though!).

The patch (context diff) is 94 lines, and is available at

  http://computeroptions.net/sword/no-more-warnings.diff

NOTE: This is UNTESTED CODE and may not be in any sense a "GOOD" long
term solution!  But, it definitely gets rid of the warnings, so call it
a "proof of concept" implementation :)

Basically I found that there were 3 issues causing warnings:

(a) write() called ignoring its result (twice, in filemgr.cpp)
(b) mkdir used without being declared in untgz.c
(c) TGZname function never used in untgz.c

The third even had a comment saying "This will give a benign warning" --
but with -Wall -Werror, *no* warnings are benign, by definition!

Plus the remaining problem: as far as I can tell, the SWORD build system
does not really do what Troy thinks it does, when used with
--enable-debug .  I'm not changing that yet, because if I did, normal
./usrinst.sh builds would then really use -Werror, and so fail (without
the no-more-warnings.diff applied).

Congrats to Dmitry for uncovering this issue.  And apologies for another
long email!

Jonathan