Bug 458030 - Review Request: avogadro - Avogadro is an advanced Molecular editor
Summary: Review Request: avogadro - Avogadro is an advanced Molecular editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thorsten Leemhuis
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-08-06 08:14 UTC by Sebastian Dziallas
Modified: 2008-11-15 18:06 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-11-15 18:06:45 UTC
Type: ---
Embargoed:
rpandit: fedora-review+
huzaifas: fedora-cvs+


Attachments (Terms of Use)

Description Sebastian Dziallas 2008-08-06 08:14:42 UTC
Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro-0.8.1-1.fc9.src.rpm
Description: Avogadro is an advanced molecular editor designed for cross-platform use in computational chemistry, molecular modeling, bioinformatics, materials science, and related areas. It offers flexible rendering and a powerful plugin architecture.
By the way: This is my first package.

Comment 1 Thorsten Leemhuis 2008-08-08 19:13:15 UTC
just a quick initial review from just looking at the spec file:

- > BuildRequires: cmake >= 2.4.5, qt4-devel >= 4.3.0, eigen-devel >= 1.0.5, openbabel-devel >= 2.2.0, python-devel >= 2.5.1

 Thats a pretty long line; would look much cleaer if you split it

- > Requires: libavogadro = %{version}

 Are you sure that's needed? RPM will likely add a dep on the libs it needs automatically

- the description is quite small (just 55 chars instead off less then 80); no blocker, but looks odd

- > %package -n libavogadro

 I'd say the libs package should be called avogadro-libs -- that's the usual way to name the libs subpackage  in Fedora 

- > # set permissions

 Please add a comment to the spec file why you do this

Comment 2 Sebastian Dziallas 2008-08-09 10:28:17 UTC
I have uploaded a new version, which should fix your points and renamed the shared library package to avogadro-libs.

Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogardo-0.8.1-2.fc9.src.rpm

Comment 3 Sebastian Dziallas 2008-08-09 19:23:25 UTC
Here is a new version, which contains a reorganized spec file and fixes some things concerning the icon implementation.

Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro-0.8.1-3.fc9.src.rpm

Comment 4 Sebastian Dziallas 2008-08-13 18:50:32 UTC
...and another new version fixing some typos:

Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro-0.8.1-4.fc9.src.rpm

Comment 5 Thorsten Leemhuis 2008-09-14 07:55:09 UTC
= general =

- rpmlint mentions
 >  avogadro.src: W: strange-permission avogadro-0.8.1.tar.bz2 0777
 please fix

 > avogadro-libs.x86_64: W: no-documentation
 > avogadro-devel.x86_64: W: no-documentation
 Can be ignored


- doesn't build in mock (and thus will faill in the Fedora buildsys):

 > /var/tmp/rpm-tmp.5185: line 37: desktop-file-install: command not found


- from mock right after the first cmake call:

 > -- docbook tools not found, doc targets disabled

Please check; docs should be shipped normally. Likely needs just a porper BuildRequire as well


== libs subpackage ==

- Group should be "System Environment/Libraries"

== devel subpackage ==

- Change 
 > Requires:       %{name} = %{version}
to
 > Requires:       %{name}-libs = %{version}-%{release} 

== More Comments ==

Looks good otherwise. Once the above things are fixed I'll approve the package and sponser you.

Comment 6 Sebastian Dziallas 2008-09-14 18:09:25 UTC
Here's a version, which should address your points:

Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro-0.8.1-5.fc9.src.rpm

Thanks!

Comment 7 Sebastian Dziallas 2008-09-14 18:57:57 UTC
...and yet another version adding the handbook to the docs:

Spec URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro.spec
SRPM URL: http://sdz.fedorapeople.org/rpmbuilds/avogadro-0.8.1-6.fc9.src.rpm

Comment 8 Thorsten Leemhuis 2008-09-14 19:32:45 UTC
Package looks good now; APPROVED; please apply to the packager group in FAS

Comment 9 Rakesh Pandit 2008-09-14 19:57:44 UTC
@Thorsten

I have updated the review flag - you might have forgotten :)

Comment 10 Sebastian Dziallas 2008-09-15 05:24:14 UTC
New Package CVS Request
=======================
Package Name: avogadro
Short Description: Avogadro is an advanced Molecular editor
Owners: sdz
Branches: F-8 F-9
InitialCC:

Comment 11 Huzaifa S. Sidhpurwala 2008-09-15 09:28:30 UTC
cvs done

Comment 12 Fedora Update System 2008-09-16 18:59:18 UTC
avogadro-0.8.1-6.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/avogadro-0.8.1-6.fc9

Comment 13 Fedora Update System 2008-09-25 00:18:23 UTC
avogadro-0.8.1-6.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 update avogadro'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8291

Comment 14 Fedora Update System 2008-10-09 21:31:53 UTC
avogadro-0.8.1-6.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.