[sword-devel] configure.ac defaults

[Jonathan Marsden]

Troy A. Griffitts wrote:

> COLORED_ECHO([    BUILD TESTS:      $enable_tests])
> COLORED_ECHO([    BUILD EXAMPLES:   $enable_examples])
> COLORED_ECHO([    BUILD UTILITIES:  $enable_utilities])

If we're going for comprehensiveness, we could add

  COLORED_ECHO([    CONF:             $with_conf])
  COLORED_ECHO([    PROFILEFN:        $enable_profilefn])
  COLORED_ECHO([    SHAREDLIB:        $enable_shared])
  COLORED_ECHO([    STATICLIB:        $enable_static])
  COLORED_ECHO([    WARNINGS:         $enable_warnings])

as well.  And then maybe sort them all alphabetically?

> Of course we could look at these extra warnings and make some code
> changes, but unless we build with these additional warnings always
> enabled on our end, chances are, g++ will continue to add new warning
> checks and we have the potential to release code which unknowingly won't
> compile a debug build because we are adding -Werror with --enable-debug
> 
> Thoughts on this?

The -DFORTIFY_SOURCE=2 also adds various stack and buffer overflow
checking, all manner of security goodness that I barely understand :)
Apparently when first used on the Red Hat package codebase (in 2004) it
found quite a few real security bugs.  But that's past history.

The warnings it generates on the current SWORD code are all related to
not checking return values of 3 functions: read(), write() and fgets().
 These tend to be mostly in the utilities, not the main library code
itself, I think.

My suspicion is that these low level areas of the SWORD codebase do not
change all that fast, so a one time addition of patches to check these
return values is sane and reasonable.  At that point, whether to always
check for such warnings or not, or have a separate --enable-fortify
option to configure in order to see them, or something inbetween, isn't
that big a deal for me.  I see the security value in
-D_FORTIFY_SOURCE=2, and my sense is that once the initial patch is in
place, these checks won't bite anyone in SWORD for a very long time, so
I'd be tempted to leave it enabled by default, but that's just me.

Regarding -Werror, I'd recommend making --enable-debug do *only* what
its help string in configure.ac says it does, which is "build debug
library (default=no)".  There is a separate configure option
--enable-warnings which says it will "build with compiler warnings as
errors (default=no)".  In other words, it is really only
--enable-warnings that should add -Werror to CXXFLAGS, not --enable-debug.

If these two options worked independently, as their help strings seem to
suggest they should, then there would be no real need (as I see it) for
a default of -Werror anywhere.  Those developers who need it for making
*sure* there are no warnings left (just before a release, for example)
can use it by design, at that time.  Most people would by default build
with plenty of warnings enabled, so those who care to look can see and
fix them, but any extra warnings that arrive unexpectedly courtesy of
g++ changes would not in and of themselves lead to a build failure (just
to some new warnings appearing at build time).  Best of both worlds?

SUMMARY OF ABOVE SUGGESTIONS:
 (1) Patch the code for all the currently known warnings.
 (2) Make --enable-debug and --enable-warnings independent.
 (3) Default to -Wall -D_FORTIFY_SOURCE=2 for all gcc/g++ builds.

(If (3) is too radical, then default to -Wall and add a new
--enable-fortify option which adds -D_FORTIFY_SOURCE=2 .  I just think
that's more work for less benefit.)

Jonathan