Spec URL: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec SRPM URL: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-2.fc5.src.rpm Description: Helmut Dersch's Panorama Tools library. Provides very high quality manipulation, correction and stitching of panoramic photographs. Note 1: I've been maintaining this and other packages in a 3rd party repository which I'd like to migrate to Extras. This is the first package, so I need a sponsor. Note 2: IPIX Corporation have a history of enforcing a US software patent covering stitching two >180 degree fisheye video streams into a 360 degree movie. The version of libpano12 on sourceforge has been modified to prevent the use of fisheye images greater than 160 degrees - This is the version I'm submitting for review.
Why has this been closed?
I guess I closed it accidentally when I added FE-NEEDSPONSOR to the blocks list. I'll try and reopen it.
Okay, let's look at the spec %define name_version %{name}-%{version} This isn't needed. Just wipe it BR gcc isn't needed Not sure on the obsoletes/provides. %configure --prefix - %configure is enough (unless the tarball decides to put it somewhere odd). If it is available as a configure option, include --disable-static %install the two strip lines aren't required. %package devel Requires : should be %{version}-%{release}. It helps keep the devel files in pace with the main package. %description devel The second paragraph isn't needed %files %{_bindir}/* - how many binaries does the package make? If they all start with PT, then %{_bindir}/PT* and then one for the other one is a much better idea. The same applies with the %{_libdir} Does libpano12 not create it's own directory in %{_includedir} or is it again just a couple of files?
(In reply to comment #3) I've updated the files: Spec URL: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec SRPM URL: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-3.src.rpm > %define name_version %{name}-%{version} > > This isn't needed. Just wipe it > BR gcc isn't needed Ok gone. > Not sure on the obsoletes/provides. This was for users migrating from rh9 to fc1, I guess it isn't needed any more ;-) > %configure --prefix - %configure is enough (unless the tarball decides to put it > somewhere odd). If it is available as a configure option, include --disable-static Ok done. --disable-static is an option, but I still have to rm the .la file > the two strip lines aren't required. Ok done. > %package devel > > Requires : should be %{version}-%{release}. It helps keep the devel files in > pace with the main package. > > %description devel > > The second paragraph isn't needed Fixed. > %files > > %{_bindir}/* - how many binaries does the package make? If they all start with > PT, then %{_bindir}/PT* and then one for the other one is a much better idea. > The same applies with the %{_libdir} Ok I've changed them to be more specific. > Does libpano12 not create it's own directory in %{_includedir} or is it again > just a couple of files? Yes it creates %{_includedir}/pano12. Fixed
You seem to have no make line in there other than make install! The %setup, %build and %prep are incorrect. setup should be setup -q (dearchives the tarball) and application of any patches, %prep is the configure step and build has the make
D'oh! %prep %setup -q %build %configure --disable-static make I need more coffee!
(In reply to comment #6) > > %prep > %setup -q > > %build > %configure --disable-static > make Ok done: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-4.fc5.src.rpm There is a %build example without a make here: http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee
That URL is purely as an example of using find_lang With the changes (which are the same as yours) the package builds fine outside of mock. rpmlint shows nothing for the main binary, a warning (no-documentation) for the -devel package, and nothing for the -debuginfo and -src.rpm. I'm test building it in mock now. Out of interest, how easy would it be for someone to build this and break the patent? It is really my only concern on this package.
(In reply to comment #8) > Out of interest, how easy would it be for someone to build this and break the > patent? It is really my only concern on this package. It's a one-line change to a header file and a rebuild.
Given the ease by which a patent can be broken, I'll carry on reviewing it, but will need to clarify the position higher up the food chain.
builds fine in mock. I've asked about #10 and will let you know when I have an answer. rpmlint on the installed packages is fine as well. All being fine with the legal bods, I can't see a problem with the amended spec and package, though the no-documentation warning in devel does need some attention.
Okay, I've asked higher up the food chain and they can't see any problem as it stands. I can't sponsor you (currently) and have asked one of the existing sponsors to come have a look around. Some other points though... %doc AUTHORS ChangeLog COPYING INSTALL NEWS README You don't need the INSTALL file. However, you do need the text files inside of doc to be added (probably best to add these to the devel package). You also need to include the README.linux file. In the changelogs, the format is %{version}-%{release} - you don't need the ?{%dist} tag on the end - infact, you shouldn't have it there full stop! For example Tue Jul 25 2006 Paul F. Johnson <paul.uk> 0.7.6.2237-6 - fixed foo - removed bar - changed usr-lib to usr-lib-nuffle
(In reply to comment #12) > You don't need the INSTALL file. However, you do need the text files inside of > doc to be added (probably best to add these to the devel package). You also need > to include the README.linux file. Done. The files in doc/ are end-user documentation, so I put them in the main package. I've taken the dist tag out of the changelogs. I seem to remember putting it in there quieten some version of rpmlint. http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-5.fc5.src.rpm
Okay, everything checks out happily on building, with rpmlint (though it does give a warning about no docs in the -devel package). The next stage is for you to get a sponsor. After that happens, I'm happy for this to be allowed in.
This needs at least a few more cleanups: * Separate the tools into a separate subpackage from the library. * This is something of a cosmetic change but all packages I know of in Fedora group the meta info at the top of the file. So I'd very strongly encourage putting the devel subpackage %package through %description info right after the main package's %description.
(In reply to comment #15) Ok, I've put the command-line tools and related docs into a separate -tools subpackage and moved the %package and %description texts to the top of the spec file: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-6.fc5.src.rpm
Can you include something to the effect that this is the modified version - altered in order to not break the patent? Builds fine under mock (x86) rpmlint clean for all (-devel does give a no documentation warning) rpm -qa --requires all met I can't see any other problems with this package. The software runs fine on my test rig APPROVED
(In reply to comment #17) > Can you include something to the effect that this is the modified version - > altered in order to not break the patent? Ok, I've added this text to the %description: "Due to patent restrictions, this library has a maximum fisheye field-of-view restriction of 160 degrees to prevent stitching of hemispherical photographs." http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/libpano12.spec http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/libpano12-2.8.4-7.fc5.src.rpm
Hey Bruno. I see you have applied for sponsorship... You might want to take a look at: http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored It's hard for sponsors to know you are ready based on just one package. Do you have more to submit to give a better idea? Or if you can add comments to other reviews that will show that you understand the guidelines...
(In reply to comment #19) > Do you have more to submit to give a better idea? Yes lots: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/ I've created another review request for vigra: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=204975
Removing FE-NEEDSPONSOR as submitter was sponsored in: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=204975
Excellent! Please feel free to import this into Fedora Extras - remember to close this bug and set the RESOLVE BUG to NEXT_RELEASE