Bug 461897 - Review Request: opticalraytracer - OpticalRayTracer is a Linux utility that analyzes systems of lenses
Review Request: opticalraytracer - OpticalRayTracer is a Linux utility that a...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-11 05:14 EDT by Milos Jakubicek
Modified: 2008-10-30 08:50 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-26 17:59:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Milos Jakubicek 2008-09-11 05:14:37 EDT
Spec URL: http://mjakubicek.fedorapeople.org/opticalraytracer/opticalraytracer.spec
SRPM URL: http://mjakubicek.fedorapeople.org/opticalraytracer/opticalraytracer-1.1-1.fc9.src.rpm
Description: OpticalRayTracer is a free (GPL) Linux (X windows GUI-based) utility that analyzes systems of lenses. It uses optical principles and a virtual optical bench to predict the behavior of many kinds of ordinary and exotic lens types.

OpticalRayTracer includes an advanced, easy-to-use interface that allows the
user to rearrange the optical configuration by simply dragging lenses around
using the mouse.

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=819744
Comment 1 Lubomir Rintel 2008-09-11 11:27:25 EDT
Looks very well.

Please remove the leading blank line in %description, I am not sure it would look well in GUI tools. Description probably needs some more love -- the fact that the package is free doesn't need to be mentioned there (redundant, see License tag) and "X windows" is correctly spelled without the trailing "s" and with "w" capitablized: "X Window", "X Window System", or just "X11".

It's not clear what does this line do, and why does it belong to %prep. Please explain and add a comment:
make -f Makefile.cvs dist

What's "%{_datadir}/apps"? Would not "%{_datadir}/raytracer" be more appropriate? In case not, please ensure you depend on the package that creates it or own it.
%{_datadir}/apps/raytracer/
Comment 2 Milos Jakubicek 2008-09-24 13:16:40 EDT
(In reply to comment #1)
> Please remove the leading blank line in %description, I am not sure it would
> look well in GUI tools. 

Done.

> Description probably needs some more love -- the fact
> that the package is free doesn't need to be mentioned there (redundant, see
> License tag) and "X windows" is correctly spelled without the trailing "s" and
> with "w" capitablized: "X Window", "X Window System", or just "X11".

Yep, sorry for that, I've just copied and pasted it from the author's webpages.

> It's not clear what does this line do, and why does it belong to %prep. Please
> explain and add a comment:
> make -f Makefile.cvs dist

This is now not necessary anymore thanks to fast upstream reaction. Also, my patch for 64bit systems has been merged and new version released (how nice!:).
  
> What's "%{_datadir}/apps"? Would not "%{_datadir}/raytracer" be more
> appropriate? In case not, please ensure you depend on the package that creates
> it or own it.
> %{_datadir}/apps/raytracer/

Look like a standard path for KDE apps. It should be ok as opticalraytracer depends on kdelibs3 which in turn depends on kde-filesystem which owns %{datadir}/apps. The same applies for %{docdir}/HTML/en where newly some files are stored too.

Note: rpmlint is silent on spec file and SRPM, but not on RPMs:

>rpmlint -i opticalraytracer-1.2-1.fc9.x86_64.rpm
opticalraytracer.x86_64: W: dangling-relative-symlink /usr/share/doc/HTML/en/raytracer/common ../common
The relative symbolic link points nowhere.

I don't know whether I can avoid this somehow, the pointed directory is owned by kdelibs-common which is also a dependency of kdelibs3, hence this shouldn't be IMHO a problem.

New SPEC file: http://mjakubicek.fedorapeople.org/opticalraytracer/opticalraytracer.spec
New SRPM: http://mjakubicek.fedorapeople.org/opticalraytracer/opticalraytracer-1.2-1.fc9.src.rpm
(there are also x68_64 RPMs in the same dir)
Comment 3 Lubomir Rintel 2008-10-03 09:19:46 EDT
> (In reply to comment #1)
> > Description probably needs some more love -- the fact
> > that the package is free doesn't need to be mentioned there (redundant, see
> > License tag) and "X windows" is correctly spelled without the trailing "s" and
> > with "w" capitablized: "X Window", "X Window System", or just "X11".
> 
> Yep, sorry for that, I've just copied and pasted it from the author's webpages.

Similarly to %description, I suggest you change the Comment= in .desktop file to somthing more sensible (such as "Utility that analyzes the system of lenses" (may apply to Summary as well):

Comment=OpticalRayTracer is a free (GPL) Linux (Xwindows GUI-based) utility that analyzes systems of lenses.(In reply to comment #2)

This is definitely not a blocker though.

> This is now not necessary anymore thanks to fast upstream reaction. Also, my
> patch for 64bit systems has been merged and new version released (how nice!:).

Thanks for that!

> > What's "%{_datadir}/apps"? Would not "%{_datadir}/raytracer" be more
> > appropriate? In case not, please ensure you depend on the package that creates
> > it or own it.
> > %{_datadir}/apps/raytracer/
> 
> Look like a standard path for KDE apps. It should be ok as opticalraytracer
> depends on kdelibs3 which in turn depends on kde-filesystem which owns
> %{datadir}/apps. The same applies for %{docdir}/HTML/en where newly some files
> are stored too.
> 
> Note: rpmlint is silent on spec file and SRPM, but not on RPMs:
> 
> >rpmlint -i opticalraytracer-1.2-1.fc9.x86_64.rpm
> opticalraytracer.x86_64: W: dangling-relative-symlink
> /usr/share/doc/HTML/en/raytracer/common ../common
> The relative symbolic link points nowhere.
> 
> I don't know whether I can avoid this somehow, the pointed directory is owned
> by kdelibs-common which is also a dependency of kdelibs3, hence this shouldn't
> be IMHO a problem.

That's fine then.

The package looks perfect now; thanks and sorry for the delay;

APPROVED
Comment 4 Milos Jakubicek 2008-10-03 12:48:52 EDT
Thanks for the review (the .desktop and summary will be updated of course)!

New Package CVS Request
=======================
Package Name: opticalraytracer
Short Description: Utility that analyzes systems of lenses
Owners: mjakubicek
Branches: F-8 F-9
InitialCC: mmahut
Comment 5 Huzaifa S. Sidhpurwala 2008-10-06 05:51:54 EDT
cvs done
Comment 6 Fedora Update System 2008-10-07 09:04:59 EDT
opticalraytracer-1.2-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/opticalraytracer-1.2-1.fc9
Comment 7 Fedora Update System 2008-10-07 09:08:12 EDT
opticalraytracer-1.2-1.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/opticalraytracer-1.2-1.fc8
Comment 8 Fedora Update System 2008-10-09 17:28:17 EDT
opticalraytracer-1.2-1.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 opticalraytracer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8701
Comment 9 Fedora Update System 2008-10-09 17:34:10 EDT
opticalraytracer-1.2-1.fc8 has been pushed to the Fedora 8 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 opticalraytracer'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-8743
Comment 10 Fedora Update System 2008-10-30 08:49:07 EDT
opticalraytracer-1.2-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 11 Fedora Update System 2008-10-30 08:50:33 EDT
opticalraytracer-1.2-1.fc8 has been pushed to the Fedora 8 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.