Bug 219973 - (powerdns) Review Request: pdns - A modern, advanced and high performance authoritative-only nameserver
Review Request: pdns - A modern, advanced and high performance authoritative-...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-17 19:09 EST by Ruben Kerkhof
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-30 14:39:17 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ruben Kerkhof 2006-12-17 19:09:12 EST
Spec URL: http://rubenkerkhof.com/packages/powerdns/powerdns.spec
SRPM URL: http://rubenkerkhof.com/packages/powerdns/powerdns-2.9.20-1.src.rpm
Description:
The PowerDNS Nameserver is a modern, advanced and high performance
authoritative-only nameserver. It is written from scratch and conforms
to all relevant DNS standards documents.
Furthermore, PowerDNS interfaces with almost any database.
Comment 1 Kevin Fenzi 2006-12-26 15:58:51 EST
I would be happy to review this package... 


See below - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
33b20ef1b767f93297101f2aa09e99ed  pdns-2.9.20.tar.gz
33b20ef1b767f93297101f2aa09e99ed  pdns-2.9.20.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK/See below - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

See below - Should build in mock.
See below - Should build on all supported archs
OK - Should have sane scriptlets.
See below - Should have subpackages require base package with fully versioned
depend.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. The upstream tar is 'pdns' and their shipped spec file makes a 'pdns-static'
package. Should this package be called 'pdns' instead of 'powerdns' ?

2. Doesn't build on x86_64 under mock. Looks like they have a hard coded
check for mysql libs using /usr/lib:

checking for MySQL library directory... configure: error: Didn't find the mysql
library dir in '/usr/local/mysql/lib/mysql /usr/local/lib/mysql
/opt/mysql/lib/mysql /usr/lib/mysql /usr/local/mysql/lib /usr/local/lib
/opt/mysql/lib /usr/lib'
error: Bad exit status from /var/tmp/rpm-tmp.75004 (%build)

I would be happy to provide access to a x86_64 box for testing if you
need one.

3. You should probably have your Requires for the subpackages 
also require the release, ie:

Requires: %{name} = %{version}-%{release}

4. Why the ldconfig calls in post/postun? The main package has no library files
at all, and the subpackages just have .so's in %{_libdir}/%{name}/ directory
that I assume are dlopened by the package when configured to do so. There should
be no need for any ldconfig that I can see here, unless I am missing something...
Comment 2 Ruben Kerkhof 2006-12-27 15:54:26 EST
Thanks for the review Kevin,

> 1. The upstream tar is 'pdns' and their shipped spec file makes a 'pdns-static'
> package. Should this package be called 'pdns' instead of 'powerdns' ?

Changed it to pdns, althought I'm not entirely happy about it. Most users will try to "yum install 
powerdns".

> 2. Doesn't build on x86_64 under mock. Looks like they have a hard coded
> check for mysql libs using /usr/lib:

Changed  to %{_libdir}/mysql, but I can't test if that works. Can you give it a try?

3. You should probably have your Requires for the subpackages 
also require the release, ie:

Fixed.

4.  Why the ldconfig calls in post/postun?

Left them in by accident, now fixed.

New files:

http://rubenkerkhof.com/packages/powerdns/pdns.spec
http://rubenkerkhof.com/packages/powerdns/pdns-2.9.20-2.src.rpm
Comment 3 Kevin Fenzi 2006-12-27 22:22:55 EST
On the name, I don't think it's a blocker either way, but since the upstream
source calls it pdns, I think thats the better choice. If a lot of people know
it by 'powerdns' perhaps you could add a: 
Provides: powerdns = %{name}-%{version}

That way if someone does a 'yum install powerdns' it would install the pdns
package, and if someone brings in a package called powerdns, you could just make
sure it is a newer version than pdns... 

3 and 4 look to be fixed. 

2 seems to still be broken. You will need to patch the configure or the like... :( 

checking whether we will be doing verbose logging... no
checking whether we should build static binaries...
checking whether we will be building the server... yes
checking whether we will be building the recursor... no
checking for MySQL library directory... configure: error: Didn't find the mysql
library dir in '/usr/local/mysql/lib/mysql /usr/local/lib/mysql
/opt/mysql/lib/mysql /usr/lib/mysql /usr/local/mysql/lib /usr/local/lib
/opt/mysql/lib /usr/lib'
error: Bad exit status from /var/tmp/rpm-tmp.2512 (%build)
Comment 4 Ruben Kerkhof 2006-12-28 15:10:28 EST
Hi Kevin,

- I added Provide: powerdns = %{name}-%{version}-%{release}
- It should now find the mysql libs on x64, turns out it is --with-mysql-lib= instead of --with-mysql-
libs.

New versions:
http://rubenkerkhof.com/packages/powerdns/pdns.spec
http://rubenkerkhof.com/packages/powerdns/pdns-2.9.20-3.src.rpm
Comment 5 Kevin Fenzi 2006-12-28 17:37:50 EST
I don't think you want the %{name} in the provides, since that will be 'pdns' in
this case... providing powerdns version 'pdns-2.9.20-3' isn't what you want.
Just remove the '%{name}-' from there and it should be correct. 

FYI, I am changing the Summary to match the new name (This helps for tracking
purposes). 

I now get a compiled package on x86_64, but it contains rpaths:

E: pdns binary-or-shlib-defines-rpath /usr/sbin/pdns_server ['/usr/lib64']
E: pdns binary-or-shlib-defines-rpath /usr/bin/zone2ldap ['/usr/lib64']
E: pdns binary-or-shlib-defines-rpath /usr/bin/pdns_control ['/usr/lib64']
E: pdns binary-or-shlib-defines-rpath /usr/bin/zone2sql ['/usr/lib64']
E: pdns-backend-geo binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgeobackend.so ['/usr/lib64']
E: pdns-backend-ldap binary-or-shlib-defines-rpath
/usr/lib64/pdns/libldapbackend.so ['/usr/lib64']
E: pdns-backend-mysql binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgmysqlbackend.so ['/usr/lib64']
E: pdns-backend-pipe binary-or-shlib-defines-rpath
/usr/lib64/pdns/libpipebackend.so ['/usr/lib64']
E: pdns-backend-postgresql binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgpgsqlbackend.so ['/usr/lib64']

See: 
http://fedoraproject.org/wiki/Packaging/Guidelines?#head-a1dfb5f46bf4098841e31a75d833e6e1b3e72544

for some possible ways to fix this. 
Comment 6 Ruben Kerkhof 2006-12-29 15:18:24 EST
Hi Kevin,

- Fixed the provides
- I tried patching the Makefile.in for the backends to remove the -rpath options, and tried patching the 
included libtool as well, but the build failed in the install phase because it couldn't find a number of .lai 
files. I've now used chrpath to remove the rpath information.

New versions:
http://rubenkerkhof.com/packages/powerdns/pdns.spec
http://rubenkerkhof.com/packages/powerdns/pdns-2.9.20-4.src.rpm

Thanks,

Ruben
Comment 7 Kevin Fenzi 2006-12-29 18:17:01 EST
The package in comment #6 still has some rpath issues. You fixed up the
binaries, but some of the shared libraries still have rpath. rpmlint says: 

E: pdns-backend-geo binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgeobackend.so ['/usr/lib64']
E: pdns-backend-ldap binary-or-shlib-defines-rpath
/usr/lib64/pdns/libldapbackend.so ['/usr/lib64']
E: pdns-backend-mysql binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgmysqlbackend.so ['/usr/lib64']
E: pdns-backend-pipe binary-or-shlib-defines-rpath
/usr/lib64/pdns/libpipebackend.so ['/usr/lib64']
E: pdns-backend-postgresql binary-or-shlib-defines-rpath
/usr/lib64/pdns/libgpgsqlbackend.so ['/usr/lib64']

Fix those up and I think we are reaching approval... ;) 
Comment 8 Ruben Kerkhof 2006-12-30 08:16:29 EST
Ah, of course.

New version:

http://rubenkerkhof.com/packages/powerdns/pdns.spec
http://rubenkerkhof.com/packages/powerdns/pdns-2.9.20-5.src.rpm

Thanks again,

Ruben
Comment 9 Kevin Fenzi 2006-12-30 13:59:03 EST
The package in comment #8 handles all the rpath issues... 
I see no further blockers, so this package is APPROVED. 

Don't forget to close this review request with NEXTRELEASE once it's been
imported and built. 
Comment 10 Kevin Fenzi 2006-12-30 14:50:02 EST
You should leave the FE-ACCEPT blocker for recordkeeping. 
Re-added it. 

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