Bug 458030

Summary: Review Request: avogadro - Avogadro is an advanced Molecular editor
Product: [Fedora] Fedora Reporter: Sebastian Dziallas <sebastian>
Component: Package ReviewAssignee: Thorsten Leemhuis <fedora>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora, fedora-package-review, huzaifas, notting, rdieter
Target Milestone: ---Flags: rpandit: fedora-review+
huzaifas: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-11-15 18:06:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

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.