Bug 246747 - Review Request: postgresql-ip4r - IPv4 and IPv4 range index types for PostgreSQL
Review Request: postgresql-ip4r - IPv4 and IPv4 range index types for PostgreSQL
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ruben Kerkhof
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-04 11:38 EDT by Devrim GUNDUZ
Modified: 2008-01-24 15:33 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-24 15:33:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Devrim GUNDUZ 2007-07-04 11:38:39 EDT
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r.spec
SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r-1.01-1.fc7.src.rpm

Description: ip4 and ip4r are types that contain a single IPv4 address and a range of IPv4 addresses respectively. They can be used as a more flexible, indexable version of the cidr type.
Comment 1 Paul Lindner 2007-07-09 09:23:23 EDT
Here's my review, I'm not an official reviewer.  Anything marked with a ****
indicates potential problems that may need to be addressed.

MUST Items:
    * rpmlint - passes, no errors reported.
    * Package naming is good.  Uses same naming of upstream package with
postgresql prefix %{parent}-%{name}.  Version is all numeric.
    * Package name and spec file name match.
    * The package meets the Packaging Guidelines.

****  - License is BSD, which corresponds to pgfoundry page.  Actual source
distribution does not contain any Licensing attribution

      - No license in source, so no need to include in %doc
      - The spec file is written in American English, and is legible.
      - Upstream source matches source in the RPM
      - RPM builds on i386
      - BuildRequires appears sane.  The postgresql-devel will contain the
appropriate compiler requirements.
      - Locales are not a problem as they are not supported.

***** - ldconfig is called, however the .so files are not installed in the
system library search paths, so it appears that this step is redundant.

      - Relocation not supported, so no special checks for that.

***** - The package does not own the directory %{_datadir}/%{name}
      - MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory. The exception to this are directories listed explicitly
in the Filesystem Hierarchy Standard
(http://www.pathname.com/fhs/pub/fhs-2.3.html), as it is safe to assume that
those directories exist.

      - No duplicate files noted, permissions appear correct, defattr is
present. Correct %clean section is present.
      - Macro usage appears consistent.
      - RPM contains code, not content, no need for a -doc subpackage.
      - There are no dependencies on the %doc readme file.
      - No header files or static libraries, so no need for a -devel package.
      - rm -rf %buildroot is at the beginning of %install
      - All filenames are be valid UTF-8.

SHOULD Items:

***** - SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.

***** - SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
      - SHOULD: The reviewer should test that the package builds in mock.
        ---- I do not have mock installed..  Not tested.

      - SHOULD: The package should compile and build into binary rpms on all
supported architectures.
        ----- Only tested on i386

      - SHOULD: The reviewer should test that the package functions as
described. A package should not segfault instead of running, for example.
        ----- Works well on i386


***** - SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.

  It seems to me that this extension might now work with postgres 8.3, 8.4,
9.0??  The following Requires appears overly broad...

Requires:       postgresql-server >= 8.1

***** Consider adding the -p flag to the install commands for the sql and the
README file to preserve the original timestamps.
Comment 2 Devrim GUNDUZ 2007-07-09 10:10:49 EDT
Hello,

(In reply to comment #1)

> ****  - License is BSD, which corresponds to pgfoundry page.  Actual source
> distribution does not contain any Licensing attribution

Yeah, I bugged the developers about this.
 
> ***** - ldconfig is called, however the .so files are not installed in the
> system library search paths, so it appears that this step is redundant.

'k, removed. 

> ***** - The package does not own the directory %{_datadir}/%{name}

Ok, fixed.

> ***** - SHOULD: The description and summary sections in the package spec file
> should contain translations for supported Non-English languages, if available.

There are no translations available.
 
> ***** - SHOULD: Usually, subpackages other than devel should require the base
> package using a fully versioned dependency.

No subpackage, so no need for this item.
 
>   It seems to me that this extension might now work with postgres 8.3, 8.4,
> 9.0??  The following Requires appears overly broad...

It may work, if one day those releases are out. I don't want to limit it to 8.2.

> ***** Consider adding the -p flag to the install commands for the sql and the
> README file to preserve the original timestamps.

Ok, done.

Thanks for the review.

Regards, Devrim
Comment 3 Devrim GUNDUZ 2007-08-11 11:18:20 EDT
Sorry, I forgot to update this :-(

New spec URL:
http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r.spec
New SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r-1.01-2.fc7.src.rpm

Regards, Devrim
Comment 4 Ruben Kerkhof 2008-01-20 16:07:06 EST
Hi Devrim,

----------------------------------------
Review for release 2.fc8:
* RPM name is OK
* Source ip4r-1.01.tar.gz is the same as upstream
* Builds fine in mock
* Runs fine
* rpmlint is clean

Can you ask your colleagues again about the license file? That's a blocker IMHO.

Comment 5 Devrim GUNDUZ 2008-01-20 18:44:56 EST
Just asked about that (again).

Upstream will release a new version soon, which will be able to be compiled
against PostgreSQL 8.3. Since PostgreSQL 8.3 hit rawhide a few days before, I'll
wait for the next version, so that we can push this package to -devel, too.

Regards, Devrim
Comment 6 Devrim GUNDUZ 2008-01-20 21:19:25 EST
Upstream thinks that the "Distributed under the same terms as PostgreSQL
itself." is enough for describing the license and does not want a simple text
file to the package.

Is it ok if I add the BSD license text to the spec file(ip4r is BSD licensed,
per website)?

Regards, Devrim
Comment 7 Jason Tibbitts 2008-01-20 21:57:09 EST
We have half a billion Perl modules that say "under the same terms as Perl
itself" so I can't see any problem with any non-Perl package doing the same
thing.  There's no need to include the text of the license in this case; just
state what it is and you're done.  The only time I'd suggest adding a copy of
the license when upstream doesn't is when it's something strange.
Comment 8 Devrim GUNDUZ 2008-01-20 22:20:11 EST
Ok then. Upstream released a new version that can be compiled against 8.3, which
means the package can be submitted for -devel, too.

New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r-1.02-1.f8.src.rpm

New Spec
http://developer.postgresql.org/~devrim/rpms/other/ip4r/postgresql-ip4r.spec

Any issues left?

Regards, Devrim
Comment 9 Ruben Kerkhof 2008-01-22 16:10:41 EST
Just one:

Source ip4r-1.02.tar.gz is different from upstream
819fb4b66122b3eccf97a1210f6114566a888e28  SOURCES/ip4r-1.02.tar.gz
77044add07e1b98bca354b9205f3cdd4ea6758a9  ip4r-1.02.tar.gz

Comment 10 Devrim GUNDUZ 2008-01-22 16:24:28 EST
Interesting. I'm getting

819fb4b66122b3eccf97a1210f6114566a888e28  ip4r-1.02.tar.gz

from my local copy, too.

Anyway, I re-uploaded the srpm to be safe.

Regards, Devrim
Comment 11 Ruben Kerkhof 2008-01-22 17:29:28 EST
Of course, the lower one is the one from upstream, that you need to download and put in your SOURCES 
directory.
Comment 12 Devrim GUNDUZ 2008-01-22 17:33:43 EST
Oh, so please re-download from upstream -- It seems ok from here.
Comment 13 Ruben Kerkhof 2008-01-22 18:00:46 EST
Hehehe, that's not how it works.

MUST: The sources used to build the package must match the upstream source, as provided in the spec 
URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, 
please see the Source URL Guidelines for how to deal with this.

Otherwise you would be able to modify the source before you check it in.
So please download the upstream tarball, rebuild your srpm and post it.
Comment 14 Devrim GUNDUZ 2008-01-22 18:08:47 EST
Ok, I was trying to save bytes in my comments, to contribute to world peace ;)

After your first comment about that, I re-downloaded the tarball, rebuilt the
srpm and re-uploaded it.

md5sum ip4r-1.02.tar.gz
50cd932352d5e3662d6e75e1a07d6294  ip4r-1.02.tar.gz

HTH.

Regards, Devrim
Comment 15 Ruben Kerkhof 2008-01-23 08:24:31 EST
I agree world peace is a good thing to aim for ;-)

But still no luck, the md5sum of the upstream package is 
5892cf0e496d326ee532a624124209be.

Here's what I did:

[exu58@kl10fe1f ~]$ rpm -i postgresql-ip4r-1.02-1.f8.src.rpm
warning: postgresql-ip4r-1.02-1.f8.src.rpm: V3 DSA signature: NOKEY, key ID 
442df0f8
[exu58@kl10fe1f ~]$ md5sum rpmbuild/SOURCES/ip4r-1.02.tar.gz
50cd932352d5e3662d6e75e1a07d6294  rpmbuild/SOURCES/ip4r-1.02.tar.gz
[exu58@kl10fe1f ~]$ spectool -g rpmbuild/SPECS/postgresql-ip4r.spec
--14:22:25--  http://pgfoundry.org/frs/download.php/1226/ip4r-1.02.tar.gz
           => `./ip4r-1.02.tar.gz'
Resolving pgfoundry.org... 200.46.204.130
Connecting to pgfoundry.org|200.46.204.130|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 18,140 (18K) [application/binary]

100%
[==============================================================================
===>] 18,140        22.25K/s

Last-modified header missing -- time-stamps turned off.
14:22:27 (22.17 KB/s) - `./ip4r-1.02.tar.gz' saved [18140/18140]

[exu58@kl10fe1f ~]$ md5sum ip4r-1.02.tar.gz
5892cf0e496d326ee532a624124209be  ip4r-1.02.tar.gz
Comment 16 Devrim GUNDUZ 2008-01-23 16:33:52 EST
Heh, I see the problem now. I hadn't updated the download URL in the Fedora spec
file. Sorry about that.

http://pgfoundry.org/frs/download.php/1573/ip4r-1.02.tar.gz

Ok, rebuild and uploaded the spec file and the srpm.

Regards, Devrim
Comment 17 Ruben Kerkhof 2008-01-23 17:29:26 EST
Ah, finally ;-)

[ruben@odin ~]$ md5sum rpmbuild/SOURCES/ip4r-1.02.tar.gz 
50cd932352d5e3662d6e75e1a07d6294  rpmbuild/SOURCES/ip4r-1.02.tar.gz
[ruben@odin ~]$ spectool -g postgresql-ip4r-1.02-1.f8.src.rpm 
[ruben@odin ~]$ md5sum ip4r-1.02.tar.gz 
50cd932352d5e3662d6e75e1a07d6294  ip4r-1.02.tar.gz

Thanks, this package is approved.
Comment 18 Devrim GUNDUZ 2008-01-23 17:36:52 EST
New Package CVS Request
=======================
Package Name: postgresql-ip4r
Short Description: IPv4 and IPv4 range index types for PostgreSQL
Owners: devrim
Branches: F-7 F-8 EL-4 EL-5
Cvsextras Commits: yes
Comment 19 Kevin Fenzi 2008-01-24 12:39:52 EST
cvs done.
Comment 20 Devrim GUNDUZ 2008-01-24 15:33:22 EST
Thanks Paul, Ruben and Kevin.

Package submitted for build.

Closing.

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