Request For Review: lout (was Re: rpm package of lout)

[Ralf Corsepius]

On Thu, 2005-06-30 at 09:25 -0500, Tom 'spot' Callaway wrote:
> On Thu, 2005-06-30 at 14:58 +0200, Oliver Falk wrote:
> > On 06/30/2005 02:15 PM, Scott Jan wrote:
> > > I am new to Linux. I am interested in using the lout ( see
> > > http://lout.sourceforge.net/ ) document formatting package. Is there
> > > anyone who can make an rpm package for fedora core 4?
> 
> Easy enough. :)
> 
> SRPM: http://www.auroralinux.org/people/spot/review/lout-3.30-1.src.rpm
> SPEC: http://www.auroralinux.org/people/spot/review/lout.spec

> Please review,

Some non-critical issues:

- compilation warnings which should be taken serious and be looked into:
z07.c:317: warning: comparison is always true due to limited range of
data type
z07.c:328: warning: comparison is always true due to limited range of
data type
z12.c:395: warning: comparison is always true due to limited range of
data type
z12.c:436: warning: comparison is always true due to limited range of
data type

- The patch hard-codes directories into the makefile. 
A more flexible approach would be to propagate directories into the
makefile using sed during %setup or pass them from the environment
during "make" and "make install".


One more critical issue with the patch. It contains:

> +ZLIB           = /usr/lib/libz.a
> +ZLIBPATH       = -I/usr/include

This way, you link statically against libz.a and pass -I/usr/include to
the compiler (A no-no - Never use -I/usr/include)

I'd recommend to use
ZLIB           = -lz
ZLIBPATH       =
instead

Ralf