[Bug 722790] Review Request: spatialindex - Spatial index library

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=722790

Martin Gieseking <martin.gieseking at uos.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |martin.gieseking at uos.de
         AssignedTo|nobody at fedoraproject.org    |martin.gieseking at uos.de
               Flag|                            |fedora-review?

--- Comment #3 from Martin Gieseking <martin.gieseking at uos.de> 2011-08-03 16:43:06 EDT ---
I'm going to sponsor Damian, and will do the formal review of this package
later.

> [+] MUST: The package must meet the Packaging Guidelines.
>     -TODO comments should be removed for the sake of clarity?

The comments can stay in the file as they help keeping track of things to be
considered in future updates.

>     -Is the handling of static libraries during %install ok?

Yes, Fedora usually ships shared libs only, and static libs should be removed
explicitly.

>     -The installed bin files are library files - ending with .so.*, would the
>      addition of lib to the package name be recommended?

Not necessarily. It's up to the packager to add the "lib" prefix. Usually, the
package should match the upstream name of the project/tarball.


> [+] MUST: The package must be licensed with a Fedora approved license.
>     - LGPLv2+ according to spec and source file headers

OK. The copyright information given in the upstream sources are the crucial
resources to identify the license here. The (downstream) spec file must match
this license.

> [X] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.

The %optflags are honored properly as you can see in file build.log created by
mock. The CFLAGS/CXXFLAGS variables are set by the %configure macro (see output
of rpm --eval %configure, for example).


> [] MUST: Files in %doc must not affect the runtime of the application.
>     Not sure which application will use the installed library files to test
> this.

The %doc files are not used (i.e. read) by the library, so no problem here
either.

> [+] MUST: devel packages must require the base package using a fully versioned
> dependency.

The guidelines have been updated in February. The "fully versioned dependency"
looks like this now:
Requires: %{name}%{?_isa} = %{version}-%{release}

http://fedoraproject.org/wiki/PackagingGuidelines#Requiring_Base_Package


> (looks as if no plans to use EPEL - I'll detail anyway)
> 
> EPEL <= 5 only:
> [+] MUST: The spec file must contain a valid BuildRoot field.

There's no BuildRoot field in the spec. ;)

=================

Some additional notes on the spec:

- I recommend to remove the rpath with the sed statements given in the 
  guidelines instead of using chrpath:
  http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath
  It works without mentioning the complete library filename and makes life
  easier with future updates.

- I'd also avoid adding the soversion in %files. A single
  %{_libdir}/lib%{name}*.so.* should be sufficient.

- The copyright information in the source files contain an old FSF address.
  If upstream is still alive, please ask them to update it according to the
  current LGPL license text: http://www.gnu.org/licenses/lgpl-2.1.html

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.