Bug 478674

1.) Please do not strip the packages while %install-ing
The debuginfo is generated empty.

export CXXFLAGS="%{optflags} -s"
make %{?_smp_mflags}

Why do you add -s there? You should not strip the debugging symbols off the binaries. Tip: Besides environment variables, make takes initial variable values also from command line:

make %{?_smp_mflags} CXXFLAGS="%{optflags}

2.) The manual contains also images, but you include only *.html in the package. You may want to wget -p it, generate a tarball and Source it along with the comment on how did you generate it. Tip: Work with upstream -- they may be willing to include documentation in the source tarball in next release. They may also be willing to include information on how to install it in Fedora, since the manual has "RPM installation section"

3.) You patch for gcc 4.3 compatibility, not 3.4 (I guess) ;)

4.) You don't have to patch makefile just to override variables. Just unset them on command line! (see 1.)

By the way -- why do you patch info installation away? Please comment!

1) Fixed.

2) Fixed, pp3-1.3.3-manual.tar.gz created.

3) Oops. Fixed.

4) Make info is trying to generate pdf files out of examples using pp3 itself - I thought adding BUILD_ROOT to PATH isn't the best idea as we don't need these files anyway if we are shipping it with pp3-1.3.3-manual.tar.gz - What do you think about it? 

Spec URL: http://mmahut.fedorapeople.org/reviews/pp3/pp3.spec
SRPM URL: http://mmahut.fedorapeople.org/reviews/pp3/pp3-1.3.3-1.fc8.src.rpm

1.) You do not need to unpack the first (zero-th) tarball two times. And do not need to move the manual directory.

%setup -q
%setup -q -b1
cp -vr ../manual/ . 

This seems useless to me. Skip the first line. (In case you're keeping the manual tarball). And unpack the manual directory in place:

%setup -q -a1

2.) Please do not use the dependency on obsolete package (tetex):

Requires:       tetex-latex

this would be more appropriate:

Requires:       tex(latex)

(In reply to comment #2)
> 4) Make info is trying to generate pdf files out of examples using pp3 itself -
> I thought adding BUILD_ROOT to PATH isn't the best idea as we don't need these
> files anyway if we are shipping it with pp3-1.3.3-manual.tar.gz - What do you
> think about it? 

I think you should be generating the manual, and adding build root to path is perfectly legal way to do that. Alternatively, you could patch the makefile to use ../pp3 instead of pp3 when building documentation.

All should be fixed, have a look:

Spec URL: http://mmahut.fedorapeople.org/reviews/pp3/pp3.spec
SRPM URL: http://mmahut.fedorapeople.org/reviews/pp3/pp3-1.3.3-1.fc8.src.rpm

Much better!

1.) Soooo... you actually strip the binary upon installation:

        install -s pp3 $(ROOT)$(DESTDIR)

Please patch the -s away from that, so that debuginfo is not empty.

2.) You get a lot of these:

pp3.i386: W: file-not-utf8 /usr/share/info/leo1.png.gz
The character encoding of this file is not UTF-8.  Consider converting it in
the specfile for example using iconv(1).

Please %exclude %{_infodir}/*.png* files

You can fix those upon commit.

APPROVED

Ok, I'll fix these before committing. Thank you for review.

New Package CVS Request
=======================
Package Name: pp3
Short Description: Creation of sky charts in Postscript or PDF format
Owners: mmahut
Branches: F-9 F-10 EL-5

cvs done.

Marek: ping. Please import && build this.