Bug 200064 - Review Request: libpano12 : Library and tools for manipulating panoramic images
Summary: Review Request: libpano12 : Library and tools for manipulating panoramic images
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul F. Johnson
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-07-25 09:10 UTC by Bruno Postle
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-11 21:10:24 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Bruno Postle 2006-07-25 09:10:44 UTC
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.

Comment 1 Paul F. Johnson 2006-07-25 09:16:57 UTC
Why has this been closed?

Comment 2 Bruno Postle 2006-07-25 09:23:45 UTC
I guess I closed it accidentally when I added FE-NEEDSPONSOR to the
blocks list.  I'll try and reopen it.

Comment 3 Paul F. Johnson 2006-07-25 09:38:39 UTC
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?



Comment 4 Bruno Postle 2006-07-25 10:19:04 UTC
(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

Comment 5 Paul F. Johnson 2006-07-25 10:52:42 UTC
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 

Comment 6 Paul F. Johnson 2006-07-25 10:54:50 UTC
D'oh!

%prep 
%setup -q

%build
%configure --disable-static
make

I need more coffee!

Comment 8 Paul F. Johnson 2006-07-25 11:24:13 UTC
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.

Comment 9 Bruno Postle 2006-07-25 11:35:00 UTC
(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.

Comment 10 Paul F. Johnson 2006-07-25 11:39:18 UTC
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.

Comment 11 Paul F. Johnson 2006-07-25 12:00:33 UTC
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.

Comment 12 Paul F. Johnson 2006-07-25 13:52:38 UTC
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

Comment 13 Bruno Postle 2006-07-25 14:54:04 UTC
(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

Comment 14 Paul F. Johnson 2006-07-26 20:52:08 UTC
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.

Comment 15 Toshio Kuratomi 2006-07-26 20:59:13 UTC
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.

Comment 16 Bruno Postle 2006-07-26 21:41:08 UTC
(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

Comment 17 Paul F. Johnson 2006-07-30 22:09:28 UTC
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



Comment 18 Bruno Postle 2006-08-15 21:43:06 UTC
(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

Comment 19 Kevin Fenzi 2006-09-01 18:50:51 UTC
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...

Comment 20 Bruno Postle 2006-09-01 20:07:06 UTC
(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

Comment 21 Kevin Fenzi 2006-09-08 02:25:39 UTC
Removing FE-NEEDSPONSOR as submitter was sponsored in: 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=204975

Comment 22 Paul F. Johnson 2006-09-08 20:52:41 UTC
Excellent!

Please feel free to import this into Fedora Extras - remember to close this bug
and set the RESOLVE BUG to NEXT_RELEASE


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