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.
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.
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
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
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.
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
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
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.
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
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
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
Of course, the lower one is the one from upstream, that you need to download and put in your SOURCES directory.
Oh, so please re-download from upstream -- It seems ok from here.
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.
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
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
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
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.
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
cvs done.
Thanks Paul, Ruben and Kevin. Package submitted for build. Closing.