Bug 490576

I can do the review on this one.

A few quick notes:

- change all of the %define:s to %global:s since the behaviour is going to change shortly (defines are only meant to be local definitions according to RPM specifications).

- Why do you turn off building of the debug package? Add a comment on this to the spec file.

- You need to preserve time stamps while copying: change all 'cp' to 'cp -p'.

- Man pages should not be in %doc; remove the %{doc} prefix from the files section for the man pages. Also, you might want to shorten the whole thing to just
%{_mandir}/man1/*.1.gz

Note, I'm not a sponsor so I can't sponsor you. I still can help you get the package into acceptable shape.

(In reply to comment #1)
> I can do the review on this one.
> 

Thanks for your suggestions, version below should look better now.

> A few quick notes:
> 
> - change all of the %define:s to %global:s since the behaviour is going to
> change shortly (defines are only meant to be local definitions according to RPM
> specifications).
 
ok

> - Why do you turn off building of the debug package? Add a comment on this to
> the spec file.

I added a note telling that being written in ocaml, there's no useful debuginfo data for gdb.

> - You need to preserve time stamps while copying: change all 'cp' to 'cp -p'.
 
Thanks for that, will take care of it in my next packaging attempts; this time i could remove the file copying, as i noticed the makefile already takes care of it.

> - Man pages should not be in %doc; remove the %{doc} prefix from the files
> section for the man pages. Also, you might want to shorten the whole thing to
> just
> %{_mandir}/man1/*.1.gz  

ok

Spec URL:
http://guidograzioli.fedorapeople.org/packages/bibtex2html/bibtex2html.spec
SRPM URL:
http://guidograzioli.fedorapeople.org/packages/bibtex2html/bibtex2html-1.93-1.src.rpm

- You should increment the release number every time you make changes to the package. Now it's hard to tell what changes you have made inbetween versions. Also remember to add a comment to the changelog about what you have done in each release.

- Change the source line to use %{version} instead of the version number. This way when a new version comes out you only need to change the Version: tag.

- Character set conversion is not done safely, use:
for file in CHANGES ; do
   mv $file timestamp && \
   iconv -f ISO-8859-1 -t UTF-8 -o $file timestamp &&  \
   touch -r timestamp $file && \
   rm timestamp
done

- No need to set prefix in %configure, since the %configure macro already does it (among a bunch of other things). Just use %configure.

- Is there a reason why you're not enabling SMP make? Use 'make %{?_smp_mflags}' instead of plain 'make'.

- Install phase: use 'make install DESTDIR=$RPM_BUILD_ROOT' instead of 'make install prefix=%{buildroot} BINDIR=%{buildroot}%{_bindir} MANDIR=%{buildroot}%{_mandir}'.

(In reply to comment #4)
> - You should increment the release number every time you make changes to the
> package. Now it's hard to tell what changes you have made inbetween versions.
> Also remember to add a comment to the changelog about what you have done in
> each release.
> - Change the source line to use %{version} instead of the version number. This
> way when a new version comes out you only need to change the Version: tag.
> 
> - Character set conversion is not done safely, use:
> for file in CHANGES ; do
>    mv $file timestamp && \
>    iconv -f ISO-8859-1 -t UTF-8 -o $file timestamp &&  \
>    touch -r timestamp $file && \
>    rm timestamp
> done
> 
> - No need to set prefix in %configure, since the %configure macro already does
> it (among a bunch of other things). Just use %configure.

ok (for the first one, i just didnt consider this one a release until approval!)
 
> - Is there a reason why you're not enabling SMP make? Use 'make
> %{?_smp_mflags}' instead of plain 'make'.

Not actually, added it and package was built cleanly; i used 'rpmdev-newspec -t ocaml' at start and flag was not proposed (specifying -t ocaml seems to be best suited to package libraries, am i wrong?)
 
> - Install phase: use 'make install DESTDIR=$RPM_BUILD_ROOT' instead of 'make
> install prefix=%{buildroot} BINDIR=%{buildroot}%{_bindir}
> MANDIR=%{buildroot}%{_mandir}'.  

Original Makefile does a mess with file copying and doesnt use install, so just passing DESTDIR makes the build fail. 
I may do something like 
%{__perl} -pi -e 's|^BINDIR=.*|BINDIR=%{buildroot}%{_bindir}|g;' Makefile
but that would require perl.
Sending a patch upstream would be best choice i think, but im not willing to editing a handwritten Makefile (i'm a java developer after all)

Files here: http://guidograzioli.fedorapeople.org/packages/bibtex2html

Thanks

(In reply to comment #5)
> > - Install phase: use 'make install DESTDIR=$RPM_BUILD_ROOT' instead of 'make
> > install prefix=%{buildroot} BINDIR=%{buildroot}%{_bindir}
> > MANDIR=%{buildroot}%{_mandir}'.  
> 
> Original Makefile does a mess with file copying and doesnt use install, so just
> passing DESTDIR makes the build fail. 

Very well, then the first version was OK. The patching with perl/sed is OK too, maybe even a bit tidier.

- The package includes documentation that can be built - please add
 BuildRequires: hevea
 BuildRequires: tex(latex)

and

 make doc

to the spec file. Include manual.ps and manual.html (and maybe also manual.dvi) to documentation.

(In reply to comment #6)
> The patching with perl/sed is OK too, maybe even a bit tidier.

ok then

> - The package includes documentation that can be built - please add
>  BuildRequires: hevea
>  BuildRequires: tex(latex)
> 
> and
> 
>  make doc
> 
> to the spec file. Include manual.ps and manual.html (and maybe also manual.dvi)
> to documentation.  

The hevea package is not available for ppc64 because of problems with ocaml; i could copy the ExclusiveArch directive from hevea.spec (i already checked building of that package fails on ppc64), but i preferred to include conditionals
 %ifnarch to build html manual for archs other than ppc64 only (and provide just manual.ps for ppc64). The wiki doesnt seem to specify which way would fit best.

Files here: http://guidograzioli.fedorapeople.org/packages/bibtex2html

(In reply to comment #7)
> The hevea package is not available for ppc64 because of problems with ocaml; i
> could copy the ExclusiveArch directive from hevea.spec (i already checked
> building of that package fails on ppc64), but i preferred to include
> conditionals
>  %ifnarch to build html manual for archs other than ppc64 only (and provide
> just manual.ps for ppc64). The wiki doesnt seem to specify which way would fit
> best.

OK, sounds reasonable. Did you test the build process to work on ppc64 now? I believe it will fail since hevea is not available and make doc tries to build the html file too..

(In reply to comment #8)
> (In reply to comment #7)
> > The hevea package is not available for ppc64 because of problems with ocaml; i
> > could copy the ExclusiveArch directive from hevea.spec (i already checked
> > building of that package fails on ppc64), but i preferred to include
> > conditionals
> >  %ifnarch to build html manual for archs other than ppc64 only (and provide
> > just manual.ps for ppc64). The wiki doesnt seem to specify which way would fit
> > best.
> 
> OK, sounds reasonable. Did you test the build process to work on ppc64 now? I
> believe it will fail since hevea is not available and make doc tries to build
> the html file too..  

No, because:

%ifnarch ppc64
make doc
%else
make manual.ps
%endif
...
%doc manual.ps manual.dvi
%ifnarch ppc64
%doc manual.html
%endif

last build it here: http://koji.fedoraproject.org/koji/taskinfo?taskID=1249703

True, I seem to have missed that.

I have reviewed the package and found it to conform with the Fedora Packaging and Package Review Guidelines. The package is hereby ACCEPTED.

Now, you still need to get a sponsor before you can get the package to CVS. You should probably make a couple of unofficial reviews on other Review Requests on Bugzilla, so that sponsors see that you know the guidelines. You might also want to submit other packages for review, but at the moment Bugzilla is quite cramped and we're hoping for more package reviews.

Thank you very much for your help Jussi; i'll look for some small & easy package to review to start with.

Ping, have you done any reviews and do you still need a sponsor?

Hi, no and yes actually; i'm trying to help packaging an eclipse feature right now (wtp), which is software i use daily. If someone would take over the maintenance of this package because i'm not sponsored, its no problem for me.

Okay. I can sponsor you, but first I want to see that you know the packaging guidelines. This means you must do a) informal reviews of other packages [you can do real ones after you have been sponsored] and b) submit a few other packages for review.

(In reply to comment #14)
> Okay. I can sponsor you, but first I want to see that you know the packaging
> guidelines. This means you must do a) informal reviews of other packages [you
> can do real ones after you have been sponsored] and b) submit a few other
> packages for review.  

hi Jussi, there are new package review requests here bug 490061 and here bug 499409 and my first attempt to do a complete review here bug 499319.

Okay, having a look. Removing NEEDSPONSOR tag.

Thanks Jussi

New Package CVS Request
=======================
Package Name: bibtex2html
Short Description: Collection of tools for translating from BibTeX to HTML
Owners: guidograzioli
Branches: F-9 F-10 F-11
InitialCC:

cvs done.

bibtex2html-1.93-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc11

bibtex2html-1.93-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc10

bibtex2html-1.93-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc9

bibtex2html-1.93-3.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update bibtex2html'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-4683

bibtex2html-1.93-3.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update bibtex2html'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-4749

bibtex2html-1.93-3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update bibtex2html'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-4807

bibtex2html-1.93-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

bibtex2html-1.93-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

bibtex2html-1.93-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.