Bug 507052
Summary: | Review Request: Panini - A tool for creating perspective views from panoramic and wide angle images | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ankur Sinha (FranciscoD) <sanjay.ankur> |
Component: | Package Review | Assignee: | Nicolas Chauvet (kwizart) <kwizart> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | bruno, fedora-package-review, julian.fedora, kwizart, notting, sanjay.ankur, tcallawa |
Target Milestone: | --- | Flags: | kwizart:
fedora-review+
j: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 0.71.103-3.fc11 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-08-17 21:52:50 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Ankur Sinha (FranciscoD)
2009-06-20 08:01:13 UTC
- starting review - A new version is here : 0.71.100 %prep try to keep timestamp when using %{__sed} -i 's/\r//' It shouldn't be needed to use CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS to use our opt flags(at least on recent Fedora). License Field is GPLv2+ (which match source code header) but GPLv3 text file is redistributed. (need to check lastest version) Please submit .desktop and icon upstream or pick the one from the package if available. Try to have the compiled binary / pixmaps installed with make install at %install step. (In reply to comment #2) > A new version is here : 0.71.100 > > %prep > try to keep timestamp when using %{__sed} -i 's/\r//' Done > > It shouldn't be needed to use CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS to > use our opt flags(at least on recent Fedora). > removed > License Field is GPLv2+ (which match source code header) but GPLv3 text file is > redistributed. (need to check lastest version) > GPLv3. Its been clarified in the latest version. > Please submit .desktop and icon upstream or pick the one from the package if > available. Submitted them. If the upstream does not provide them, I need to make them ? I got them from http://hanzlici.cz/packages/fedora/panini/ this time. > > Try to have the compiled binary / pixmaps installed with make install at > %install step. I'm not sure on how to do this. The package generates a makefile using qmake-qt4. I need to patch this generated makefile before make uses it?? the srpm: http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm the spec: http://ankursinha.fedorapeople.org/panini/Panini.spec all other files, (logs and the desktop and icon etc.) are at http://ankursinha.fedorapeople.org/panini/ regards, Ankur (In reply to comment #3) > (In reply to comment #2) >> License Field is GPLv2+ (which match source code header) but GPLv3 text file is >> redistributed. (need to check lastest version) >> >GPLv3. Its been clarified in the latest version. How ? from the source code header, none are tag as GPLv3 but GPLv2+, with the exeption of pvQt_QTVR.{cpp,h} which are licensed as LGPLv2+ The binary package will ends to be GPLv2+ then! But since the GPLv3 is available from the svn repository, this will be more accurate to use GPLv3+ in the license fied anyway. So OKay. ... > > Try to have the compiled binary / pixmaps installed with make install at > > %install step. > > I'm not sure on how to do this. The package generates a makefile using > qmake-qt4. I need to patch this generated makefile before make uses it?? You will need to patch the panini.pro file. > the srpm: > > http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm I don't understand why having %global fname panini, then fname is only used once?! But this remains trivial. Also, you could consider to clean the old comments while importing. FE-Legal: From https://bugzilla.redhat.com/show_bug.cgi?id=503252#c3 my understanding is that we aren't really creating panorama with this tool, but we are extracting perspective from already made panorama pictures. Hence, this doesn't fall under any patent anyone may beleive to have. But I would like a confirmation from FE-Legal about this... (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > >> License Field is GPLv2+ (which match source code header) but GPLv3 text file is > >> redistributed. (need to check lastest version) > >> > > >GPLv3. Its been clarified in the latest version. > How ? from the source code header, none are tag as GPLv3 but GPLv2+, with the > exeption of pvQt_QTVR.{cpp,h} which are licensed as LGPLv2+ > The binary package will ends to be GPLv2+ then! But since the GPLv3 is > available from the svn repository, this will be more accurate to use GPLv3+ in > the license fied anyway. So OKay. > i was referring to this in the release file (panini-0.71-release.txt): "LICENSE Panini 0.7 is free software, copyright (C) 2008 - 2009 Thomas K Sharpless. You can redistribute and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 3 of the License, or (at your option) any later version." Hadn't checked the source files. I'll try contacting upstream to clarify. > ... > > > Try to have the compiled binary / pixmaps installed with make install at > > > %install step. > > > > I'm not sure on how to do this. The package generates a makefile using > > qmake-qt4. I need to patch this generated makefile before make uses it?? > You will need to patch the panini.pro file. okay. I'll try that. might take me 2-3 days to learn up qt. > > the srpm: > > > > http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm > I don't understand why having %global fname panini, then fname is only used > once?! But this remains trivial. I had used it more earlier. will remove it. > Also, you could consider to clean the old comments while importing. will do that. > > FE-Legal: > From https://bugzilla.redhat.com/show_bug.cgi?id=503252#c3 my understanding is > that we aren't really creating panorama with this tool, but we are extracting > perspective from already made panorama pictures. Hence, this doesn't fall under > any patent anyone may beleive to have. > But I would like a confirmation from FE-Legal about this... okay I'll do the other work until then regards, Ankur [not a review, just trying to help] I have a spec for an earlier version of Panini here: http://bpostle.fedorapeople.org/reviews/Panini/Panini.spec You might find some useful stuff, in particular you are not running update-desktop-database in %post and %postun (ignore the update-mime-database bit, I have it wrong). I've spoken to upstream and there is no 'install' target, he doesn't have a use for one. You are mixing macro styles, this is frowned upon by the guidelines: $RPM_BUILD_ROOT%{_bindir} should be something like %{buildroot}/%{_bindir} My version of the %description has more keywords, in particular for searching you really need 'hugin', 'vedutismo', 'stereographic', 'QuickTimeVR' and 'QTVR'. You should also try and get the singular 'panorama' and the alternative 'Pannini' spelling into the description: > Panini can load most common photo and panoramic formats from image files such > as those created with hugin or QuickTimeVR (QTVR .mov) files. Like all panorama > viewers, it then shows a linear perspective view that can be panned and zoomed. > But Panini can also display a range of wide angle perspectives via the > stereographic and "Pannini" vedutismo families of projections, and shift, > rotate, and stretch the image like a software view camera. (In reply to comment #6) > [not a review, just trying to help] > > I have a spec for an earlier version of Panini here: > http://bpostle.fedorapeople.org/reviews/Panini/Panini.spec > > You might find some useful stuff, in particular you are not running > update-desktop-database in %post and %postun (ignore the update-mime-database > bit, I have it wrong). > Thanks for this.I've added this portion.However, I don't know what the command is responsible for yet. Do I need to add the update-mime-database thing too? I don't completely understand it from here. for eg, what is a "MimeType key."? http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database > I've spoken to upstream and there is no 'install' target, he doesn't have a use > for one. Do I still need to patch the .pro? > > You are mixing macro styles, this is frowned upon by the guidelines: > $RPM_BUILD_ROOT%{_bindir} should be something like %{buildroot}/%{_bindir} > Corrected :) > My version of the %description has more keywords, in particular for searching > you really need 'hugin', 'vedutismo', 'stereographic', 'QuickTimeVR' and > 'QTVR'. You should also try and get the singular 'panorama' and the alternative > 'Pannini' spelling into the description: > > > Panini can load most common photo and panoramic formats from image files such > > as those created with hugin or QuickTimeVR (QTVR .mov) files. Like all panorama > > viewers, it then shows a linear perspective view that can be panned and zoomed. > > But Panini can also display a range of wide angle perspectives via the > > stereographic and "Pannini" vedutismo families of projections, and shift, > > rotate, and stretch the image like a software view camera. I changed the spec to this one.. new spec and srpm: http://ankursinha.fedorapeople.org/panini/Panini.spec http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm all the other mock build results are here (rpms etc.): http://ankursinha.fedorapeople.org/panini/ Okay, so there are two issues here: 1. Since the code in the tarball all says GPLv2+ or LGPLv2+, and the readme says GPLv3+ (one of them does, another one says GPLv2+), and the latest SVN code (not just the readme) says GPLv2+ or LGPLv2+, we should email upstream and ask them to correct the license attributions in the code in SVN, and once that is done, we should be fine to use License: GPLv3+. Otherwise, if you don't want to wait, use License: GPLv2+ now and update it when and if they release something with proper code attribution. 2. Patent issues. The only patents I'm aware of are around control point generators for stitching. This code does not seem to do that, it seems to use already created panoramics. Can you confirm that? > However, I don't know what the command is responsible for yet. Do I need to > add the update-mime-database thing too? You need to update the desktop database when you install a .desktop file. You are not introducing a new mime-type so apparently it isn't necessary to update the mime database. > I don't completely understand it from here. for eg, what is a "MimeType key."? This is for file associations, users should be able to select these kinds of files in (e.g) nautilus and open them in Panini: MimeType=video/quicktime;image/jpeg;image/tiff;image/png; (In reply to comment #8) > > 2. Patent issues. The only patents I'm aware of are around control point > generators for stitching. This code does not seem to do that, it seems to use > already created panoramics. Can you confirm that? Yes, Panini uses an already complete single panorama as input. Someone has sent an email upstream already ? We haven't leaved Fe-Legal, does patents around panoramic image are still searched, or it just remains the License question ? The only remaining issue is the License question. I have not reached out to upstream, I'd prefer if the package maintainer did that. (In reply to comment #11) > Someone has sent an email upstream already ? > hi, I have. Waiting for a reply. regards, Ankur hi, Quick response from upstream :) new srpm : http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-1.fc10.src.rpm spec : http://ankursinha.fedorapeople.org/panini/Panini.spec other results from the mock build: http://ankursinha.fedorapeople.org/panini/ regards, Ankur Upstream source is now consistent, but you're still using the wrong license tag. Since all of the code says: * This file is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 3 of the License, or * (at your option) any later version. You need to use: License: GPLv3+ Hello? This should be an easy fix, then I can lift FE-Legal. (In reply to comment #16) > Hello? This should be an easy fix, then I can lift FE-Legal. hi spot, Sorry to have kept you waiting. Just got my internet connection today(in college) . Will have it up for you max by tomorrow. Upstream's requested my to use the icon he's packed. I'll correct both the issues and upload. regards, Ankur hi, spec: http://ankursinha.fedorapeople.org/panini/Panini.spec SRPM: http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-2.fc10.src.rpm I've tried to use upstream's jpg file as the icon: I changed the .desktop file[1] and tested it. It *din't* work. How is one supposed to do it? I've already read https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage . It doesn't have much info on the .desktop. [1]http://ankursinha.fedorapeople.org/panini/Panini.desktop Other results from the mock build (logs etc.) : http://ankursinha.fedorapeople.org/panini/ regards, Ankur There is a company called Panini, if that matters: http://www.paninigroup.com/ "The image files must be one of the types: PNG, XPM, or SVG, and the extension must be ".png", ".xpm", or ".svg" (lower case)." http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html Since your image file is JPG, that's why it isn't working. Convert it to a PNG and it should be fine. Lifting FE-Legal. (In reply to comment #18) > hi, > > spec: > > http://ankursinha.fedorapeople.org/panini/Panini.spec > > SRPM: > > http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-2.fc10.src.rpm > > I've tried to use upstream's jpg file as the icon: I changed the .desktop > file[1] and tested it. It *din't* work. How is one supposed to do it? I've > already read > https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage > . It doesn't have much info on the .desktop. > > [1]http://ankursinha.fedorapeople.org/panini/Panini.desktop > > Other results from the mock build (logs etc.) : > http://ankursinha.fedorapeople.org/panini/ > > regards, > > Ankur hi, Spec : http://ankursinha.fedorapeople.org/panini/Panini.spec SRPM: http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-3.fc10.src.rpm other files at : http://ankursinha.fedorapeople.org/panini/ - icon works. - seems to have packaged up correctly. - no errors/warnings from rpmlint regards, Ankur OK! Panini is APPROVED ---------------------------------- New Package CVS Request ======================= Package Name: Panini Short Description: A tool for creating perspective views from panoramic and wide angle images Owners: ankursinha Branches: F-10 F-11 InitialCC: CVS done. Panini-0.71.103-3.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/Panini-0.71.103-3.fc11 Panini-0.71.103-3.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/Panini-0.71.103-3.fc10 Panini-0.71.103-3.fc11 has been pushed to the Fedora 11 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 Panini'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8203 Panini-0.71.103-3.fc10 has been pushed to the Fedora 10 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 Panini'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-8211 Panini-0.71.103-3.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. Panini-0.71.103-3.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report. |