This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 458030 - Review Request: avogadro - Avogadro is an advanced Molecular editor
Review Request: avogadro - Avogadro is an advanced Molecular editor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thorsten Leemhuis
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-06 04:14 EDT by Sebastian Dziallas
Modified: 2008-11-15 13:06 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-15 13:06:45 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
rpandit: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Sebastian Dziallas 2008-08-06 04:14:42 EDT
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 15:13:15 EDT
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 06:28:17 EDT
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 15:23:25 EDT
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 14:50:32 EDT
...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 03:55:09 EDT
= 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 14:09:25 EDT
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 14:57:57 EDT
...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 15:32:45 EDT
Package looks good now; APPROVED; please apply to the packager group in FAS
Comment 9 Rakesh Pandit 2008-09-14 15:57:44 EDT
@Thorsten

I have updated the review flag - you might have forgotten :)
Comment 10 Sebastian Dziallas 2008-09-15 01:24:14 EDT
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 05:28:30 EDT
cvs done
Comment 12 Fedora Update System 2008-09-16 14:59:18 EDT
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-24 20:18:23 EDT
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 17:31:53 EDT
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.