Bug 490576 - Review Request: bibtex2html - Collection of tools for translating from BibTeX to HTML
Review Request: bibtex2html - Collection of tools for translating from BibTeX...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-16 23:02 EDT by Guido Grazioli
Modified: 2009-05-29 22:39 EDT (History)
3 users (show)

See Also:
Fixed In Version: 1.93-3.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-29 22:27:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Guido Grazioli 2009-03-16 23:02:50 EDT
Spec URL: http://guidograzioli.fedorapeople.org/packages/bibtex2html/bibtex2html.spec
SRPM URL: http://guidograzioli.fedorapeople.org/packages/bibtex2html/bibtex2html-1.93-1.src.rpm
Description: They allow to produce, from a set of bibliography files in BibTeX format, a bibliography in HTML format.

This package was requested in the wishlist wiki page; rpmlint is silent (though with ocaml 3.10.2 shipped with fedora 10, the binary rpm gives "W: executable-stack", see bug 450551; rpms built on koji are fine).
koji build is here: http://koji.fedoraproject.org/koji/taskinfo?taskID=1245193
Comment 1 Susi Lehtola 2009-03-17 19:32:28 EDT
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
Comment 2 Susi Lehtola 2009-03-17 19:55:01 EDT
Note, I'm not a sponsor so I can't sponsor you. I still can help you get the package into acceptable shape.
Comment 3 Guido Grazioli 2009-03-17 21:30:47 EDT
(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
Comment 4 Susi Lehtola 2009-03-18 10:49:42 EDT
- 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}'.
Comment 5 Guido Grazioli 2009-03-18 11:40:32 EDT
(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
Comment 6 Susi Lehtola 2009-03-18 12:05:01 EDT
(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.
Comment 7 Guido Grazioli 2009-03-19 12:53:35 EDT
(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
Comment 8 Susi Lehtola 2009-03-19 16:19:50 EDT
(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..
Comment 9 Guido Grazioli 2009-03-19 21:54:06 EDT
(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
Comment 10 Susi Lehtola 2009-03-20 11:14:42 EDT
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.
Comment 11 Guido Grazioli 2009-03-20 12:17:32 EDT
Thank you very much for your help Jussi; i'll look for some small & easy package to review to start with.
Comment 12 Susi Lehtola 2009-04-25 03:42:13 EDT
Ping, have you done any reviews and do you still need a sponsor?
Comment 13 Guido Grazioli 2009-04-29 10:02:42 EDT
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.
Comment 14 Susi Lehtola 2009-04-30 02:11:29 EDT
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.
Comment 15 Guido Grazioli 2009-05-09 05:15:45 EDT
(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.
Comment 16 Susi Lehtola 2009-05-09 05:52:10 EDT
Okay, having a look. Removing NEEDSPONSOR tag.
Comment 17 Guido Grazioli 2009-05-09 08:55:36 EDT
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:
Comment 18 Kevin Fenzi 2009-05-09 16:47:16 EDT
cvs done.
Comment 19 Fedora Update System 2009-05-09 18:34:02 EDT
bibtex2html-1.93-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc11
Comment 20 Fedora Update System 2009-05-09 18:34:08 EDT
bibtex2html-1.93-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc10
Comment 21 Fedora Update System 2009-05-09 18:34:13 EDT
bibtex2html-1.93-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/bibtex2html-1.93-3.fc9
Comment 22 Fedora Update System 2009-05-11 23:54:17 EDT
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
Comment 23 Fedora Update System 2009-05-12 00:00:04 EDT
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
Comment 24 Fedora Update System 2009-05-12 00:05:48 EDT
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
Comment 25 Fedora Update System 2009-05-29 22:27:37 EDT
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.
Comment 26 Fedora Update System 2009-05-29 22:32:05 EDT
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.
Comment 27 Fedora Update System 2009-05-29 22:39:43 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.