Bug 503297 - Review Request: panoglview - Immersive viewer for spherical panoramas
Review Request: panoglview - Immersive viewer for spherical panoramas
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nicolas Chauvet (kwizart)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-30 18:47 EDT by Bruno Postle
Modified: 2009-07-27 07:32 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-27 07:32:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kwizart: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Bruno Postle 2009-05-30 18:47:40 EDT
Spec URL: http://bpostle.fedorapeople.org/reviews/panoglview/panoglview.spec
SRPM URL: http://bpostle.fedorapeople.org/reviews/panoglview/panoglview-0.2.2-3.fc10.src.rpm
Description:

Use panoglview to explore equirectangular panoramic images.  Equirectangular
panoramas are typically JPEG/TIFF/PNG images with a 2:1 aspect ratio.
Comment 1 Fabian Affolter 2009-06-16 05:51:20 EDT
Just some quick comments on your spec file

- You can remove '--vendor=""'
- '%defattr(-, root, root)' should be '%defattr(-,root,root,-)'
- Please add '%{?_smp_mflags}' to make
Comment 2 Bruno Postle 2009-06-17 15:23:59 EDT
Thanks I've fixed %defattr and added %{?_smp_mflags}, I've left --vendor="" as this is required for it to build on el4 and el5:

http://bpostle.fedorapeople.org/reviews/panoglview/panoglview.spec
http://bpostle.fedorapeople.org/reviews/panoglview/panoglview-0.2.2-4.fc10.src.rpm

I forgot to add that the way to test this package is to download some typical equirectangular panoramas and try to view them:

http://www.flickr.com/search/?q=equirectangular&l=commderiv&ss=2&ct=6&w=all&adv=1
Comment 3 Nicolas Chauvet (kwizart) 2009-07-01 11:18:21 EDT
- starting review -

OK - rpmlint panoglview is quiet on installed package
OK - build in mock (fedora 11 x86_64 )

NEEDWORK - You aren't expected to run:
update-mime-database %{_datadir}/mime
Because this package doesn't bring any new mime type. But indeed update-desktop-database is mandatory since the .desktop file use a MimeType= field. As said:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

By the way, using || : at the end of each scriptlet filters the error code if anything went wrong. So this only makes sense for the last line:
example :
first command &> /dev/null 
second command  &> /dev/null || :

That way, the rpm transaction will continue, even if update-desktop-database failed.

NEEDWORK - License match source code : GPLv2+ but COPYING text is about GPLv3. This would need clarification

NEEDWORK - url field cannot permit to download the source tarball (is it the right url ?)

USABILITY test: I'm experiencing some refresh delay when moving the sphere.
I will try to reproduce on another workstation.
(Is it expected to see only from the inside despite of the outside of the sphere ?)
Comment 4 Nicolas Chauvet (kwizart) 2009-07-01 12:13:31 EDT
There isn't any icon from the package and .desktop file !
Is it possible to pick one ?
Comment 5 Nicolas Chauvet (kwizart) 2009-07-01 12:18:19 EDT
OKay, package works with radeon (x600se hardware) i686 F-11 whereas gives problem with nvidia lastest 3D driver F-11 x86_64.

I will consider usability test to have succeeded
Comment 6 Bruno Postle 2009-07-01 17:48:15 EDT
Thanks, updated spec and src.rpm here:
http://bpostle.fedorapeople.org/reviews/panoglview/panoglview.spec
http://bpostle.fedorapeople.org/reviews/panoglview/panoglview-0.2.2-5.fc11.src.rpm

* removed the update-mime-database line.
* changed License to GPLv3+.  The COPYING file isn't in SVN and was added by automake at tarball build time.  So the project is still GPLv2+, but the released tarball can only be interpreted as GPLv3+. I guess we are allowed to use 'any later version'.
* fixed URL paste error in Source line.
* I've added an icon based on the hugin icon.
Comment 7 Jason Tibbitts 2009-07-01 18:00:42 EDT
Please note that the version of the GPL in the COPYING file has absolutely no bearing at all on the version of the GPL the software is under.
Comment 8 Bruno Postle 2009-07-01 18:28:02 EDT
Regarding the license, what do you suggest? I see three options:

* Delete COPYING and regenerate it with `automake --missing --copy` (result is a GPLv2 file).
* Since we are entitled to upgrade to GPLv3 any time we like, just do that in the spec file.
* Since the COPYING file has absolutely no bearing, ship it anyway and put GPLv2+ in the spec file.
Comment 9 Nicolas Chauvet (kwizart) 2009-07-06 19:13:15 EDT
Given the license of the build dependency,(wxWidgets, zlib, libtiff) which all are GPLv3 compatible we will not have a problem to move to GPLv3+ (like if any dependency were GPLv2 only). Now nothing requires us to be GPLv3+ either (exept if we consider COPYING text matter much than source code).

Quoting the guidelines, we have to:
- Have a license field
- If a COPYING is provided, have it bundled.
- Consider the binary rpm as the license field.

What we don't have to do:
- Have license field to match COPYING text.

So this ends to be the #3rd choice which is the one of the src.rpm


All other fields from the review have been corrected.

-----------------------

This package (panoglview) is APPROVED by me

-----------------------
Comment 10 Bruno Postle 2009-07-07 11:43:17 EDT
New Package CVS Request
=======================
Package Name: panoglview
Short Description: Immersive viewer for spherical panoramas
Owners: bpostle
Branches: F-11
Comment 11 Jason Tibbitts 2009-07-08 12:30:27 EDT
CVS done.
Comment 12 Fabian Affolter 2009-07-27 07:32:34 EDT
Package was built: http://koji.fedoraproject.org/koji/buildinfo?buildID=120355

Note You need to log in before you can comment on or make changes to this bug.