Bug 466789 - Review Request: jmol - an open-source Java viewer for chemical structures in 3D
Summary: Review Request: jmol - an open-source Java viewer for chemical structures in 3D
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul F. Johnson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-10-13 16:21 UTC by Susi Lehtola
Modified: 2008-10-24 23:53 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-10-24 23:53:10 UTC
Type: ---
Embargoed:
paul: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Susi Lehtola 2008-10-13 16:21:08 UTC
Spec URL: http://theory.physics.helsinki.fi/~jzlehtol/rpms/jmol.spec
SRPM URL: http://theory.physics.helsinki.fi/~jzlehtol/rpms/jmol-11.6-1.10081svn.fc9.src.rpm

Description: 
Jmol is a free, open source molecule viewer for students, educators, 
and researchers in chemistry and biochemistry.

rpmlint output clean:
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 1 Peter Lemenkov 2008-10-13 20:53:32 UTC
Not a formal review:

* Summary should be simply "An open-source Java viewer for chemical structures in 3D" instead of "Jmol: an open-source Java viewer for chemical structures in 3D"

* You should note which svnver you're checking out. E.g. not only "svn co https://jmol.svn.sourceforge.net/svnroot/jmol/branches/v11_6/Jmol" but "svn co -r %{svnrel} https://jmol.svn.sourceforge.net/svnroot/jmol/branches/v11_6/Jmol"

* Use "svn export" instead of "svn co"

* Correct path for icon should be "http://wiki.jmol.org:81/images/Jmol_icon_128.png"

* About commented out "Requires:" - jmol doesn't requires java?

* Utility "install" ignores switch -c.

* No need to explicity create directories with mkdir in your case. You should use "-D" switch of "install" utility, e.g.

install -D -p -m 755 jmol %{buildroot}%{_bindir}/jmol

* You should use %{name} instead of jmol in your %install section (it simplifies copypasting, for example :). E.g.

install -D -p -m 755 %{name} %{buildroot}%{_bindir}/%{name}

* Conversion of documents must be done in more reliable way. I suggest the following:

for txtfile in README.txt COPYRIGHT.txt LICENSE.txt; do
	iconv -f ASCII -t UTF-8 $txtfile > $txtfile.new && mv $txtfile{.new,}
done


Probably there are others issues.

Comment 2 Susi Lehtola 2008-10-14 09:03:17 UTC
(In reply to comment #1)
> * Correct path for icon should be
> "http://wiki.jmol.org:81/images/Jmol_icon_128.png"

With this I get during build

/var/tmp/rpm-tmp.65853: line 52: unexpected EOF while looking for matching `"'
error: Bad exit status from /var/tmp/rpm-tmp.65853 (%install)

It seems that %{SOURCE2} somehow drops the first quotation mark.

Everything else should be fixed. Also I branched the documentation to separate packages. rpmlint output is clean for spec file and the three packages.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/jmol.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/jmol-11.6-2.10081svn.fc9.src.rpm

Comment 3 Paul F. Johnson 2008-10-14 09:17:37 UTC
Okay, I'll take this and review it :-) More to follow....

Comment 4 Susi Lehtola 2008-10-21 06:59:54 UTC
Ping, how's the review coming along?

Comment 5 Paul F. Johnson 2008-10-22 19:13:22 UTC
rpmlint is clean and nothing seems amiss with the build on my buildsys.
Spec file is good and clean and I can't see anything unowned by the package

I'm happy for this to go in.

APPROVED

Comment 6 Susi Lehtola 2008-10-22 20:22:00 UTC
New Package CVS Request
=======================
Package Name: jmol
Short Description: An open-source Java viewer for chemical structures in 3D
Owners: jussilehtola
Branches: F-9 F-10 EL-5
InitialCC:

Comment 7 Susi Lehtola 2008-10-22 20:22:19 UTC
(In reply to comment #5)
> rpmlint is clean and nothing seems amiss with the build on my buildsys.
> Spec file is good and clean and I can't see anything unowned by the package
> 
> I'm happy for this to go in.
> 
> APPROVED

Thanks for the review.

Comment 8 Susi Lehtola 2008-10-22 20:23:44 UTC
Please assign the package to yourself.

You did go through the review guidelines, right?

Comment 9 Kevin Fenzi 2008-10-23 20:16:03 UTC
cvs done.

Comment 10 Fedora Update System 2008-10-24 14:58:46 UTC
jmol-11.6-5.10137svn.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/jmol-11.6-5.10137svn.fc9

Comment 11 Fedora Update System 2008-10-24 23:53:07 UTC
jmol-11.6-5.10137svn.fc9 has been pushed to the Fedora 9 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.