Bug 229321

Okay, ran through the spec file, and found a fair number of things that I
believe need fixing. I often prefer to make my suggestions in the form of direct
spec file changes that can then be diffed versus the original spec. My altered
spec is here:

http://people.redhat.com/jwilson/packages/postgresql-pgpool-II/postgresql-pgpool-II.spec

Basic summary of things to change:

* Tue Feb 20 2007 Jarod Wilson <jwilson> 1.0.2-2
- Create proper devel package, drop -libs package
- Nuke rpath
- Don't install libtool archive and static lib
- Clean up %%configure line
- Use proper %%_smp_mflags
- Install config files properly, without .sample on the end
- Preserve timestamps on header files

Thanks for the review.

Spec URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/pgpool-II.spec
SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-2.src.rpm

Regards, Devrim

I'll see if I can make another run through the latest srpm tomorrow.

FYI, the spec you're linking to is a rather outdated one that doesn't match
what's in the srpm...

Hi,

(In reply to comment #3)
> I'll see if I can make another run through the latest srpm tomorrow.

Ok.
 
> FYI, the spec you're linking to is a rather outdated one that doesn't match
> what's in the srpm...

Ah thanks. It is a copy-paste laziness :)

Here is the real spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II.spec

Regards, Devrim

At first glance, you need to add BuildRequires: libpqxx-devel.  Also, once
built, rpmlint complains about a lack of docs in -devel, but is otherwise clean.
 TBC. . .

Hello,

(In reply to comment #5)
> At first glance, you need to add BuildRequires: libpqxx-devel. 

AFAICS pgpool-II does not depend on libpqxx-devel. Are you sure about that?

> Also, once built, rpmlint complains about a lack of docs in -devel, but is 
> otherwise clean.

Thanks a lot for this review.

Regards, Devrim

It build with it, and not without it, on FC6.  I'm testing a mock build without it.

Mock confirms the need for that BR.  Otherwise mock build happily, and the
resulting RPMS have only the -devel doc warning, otherwise clean.  

Needs naming guidelines.
Good spec name.
Meets Packaging Guidelines, AFAICS.
License is OK.

Spec is American English, and legible.
ISSUE:
md5sum from upstream 
adf88e4b7eb7f3347740a6b54aa09e92  pgpool-II-1.0.2.tar.gz
md5sum from SRPM
4abe109bfc3c78d441a9533365ccccd7  /usr/src/redhat/SOURCES/pgpool-II-1.0.2.tar.gz

Builds locally and in mock on x86.
BRs, see above.
No locales to handle.
ldconfig correct.
Not relocatable.
Explicitly creates no directories.
ISSUE: %{_mandir}/man8/* will own all man8 man pages.  Must be narrowed down.
No dupes, perms ok.
%clean correct.
Macros OK.
Code, not content.
Docs are reasonably sized and non-critical at runtime.
Dev/static handling correct, no .la.
No pkgconfig.pc.
Devel requires base, versioned.
Not a GUI app.
%install begins correctly.
ALL filenames UTF8.

All MUSTS are OK except for the BR, md5sum, and man8 ownership.

Thanks for the review. I still cannot see libpgxx-devel issue. I don't have it
on my box, but I can build pgpool-II. Also I'm pretty sure that there is nothing
in the code that is dependent on libpqxx...

Other than this... I am really not sure about how md5 problem appeared. I fixed
that now. Also, fixed the man ownership.

Spec URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/pgpool-II.spec
SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-2.src.rpm

Regards, Devrim

Several issues:

1. Missing BR's:
...
checking for PQexecPrepared in -lpq... no
configure: error: libpq is not installed or libpq is old
error: Bad exit status from /var/tmp/rpm-tmp.37113 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.37113 (%build)

=> MISSING: BR: postgresql-devel

2. Package supports --disable-static
=> append --disable-static to %configure

3. All of the manual installs in %install are superfluous:

@@ -52,11 +53,6 @@
 make DESTDIR=%{buildroot} install
 mv %{buildroot}%{_sysconfdir}/pgpool.conf.sample
%{buildroot}%{_sysconfdir}/pgpool.conf
 mv %{buildroot}%{_sysconfdir}/pcp.conf.sample %{buildroot}%{_sysconfdir}/pcp.conf
-install -m 644 pgpool.8 %{buildroot}%{_mandir}/man8/
-install -m 644 -p pcp/pcp.h  %{buildroot}%{_includedir}/
-install -m 755 pcp/.libs/libpcp.so.0.* %{buildroot}%{_libdir}/
-cp -a pcp/.libs/libpcp.so.0 %{buildroot}%{_libdir}/
-install -m 644 -p pool_type.h  %{buildroot}%{_includedir}/
 # nuke libtool archive and static lib
 rm -f %{buildroot}%{_libdir}/libpcp.{a,la}

4. Very serious compiler warnings:

if gcc -DHAVE_CONFIG_H -DDEFAULT_CONFIGDIR=\"/etc\" -I. -I. -I.  -Wall
-Wmissing-prototypes -Wmissing-declarations -D_GNU_SOURCE -I /usr/include/pgsql
  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -MT pool_process_query.o -MD -MP -MF
".deps/pool_process_query.Tpo" -c -o pool_process_query.o pool_process_query.c; \
        then mv -f ".deps/pool_process_query.Tpo" ".deps/pool_process_query.Po";
else rm -f ".deps/pool_process_query.Tpo"; exit 1; fi
pool_process_query.c: In function 'process_reporting':
pool_process_query.c:2453: warning: call to __builtin___strncpy_chk will always
overflow destination buffer
pool_process_query.c:2578: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2580: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2583: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2585: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2588: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2590: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2593: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2595: warning: call to __builtin___snprintf_chk will always
overflow destination buffer

Created attachment 152359 [details]
patch to fix 4. from comment #13

Why does this BZ depend on 229322 and 229323?

/me thinks these blockers are reversed. Probably this BZ should block 229322 and
229323 - Please correct them.

Hi,

(In reply to comment #13)
> 1. Missing BR's:
> ...
> => MISSING: BR: postgresql-devel

:-) I thought I fixed this already. Anyway, thanks.
 
> 2. Package supports --disable-static
> => append --disable-static to %configure

Done.
 
> 3. All of the manual installs in %install are superfluous:

Ok, removed. Thanks.

> 4. Very serious compiler warnings:

Submitted your patch to upstream. I'll apply that patch tomorrow, unless someone
objects. Also added that patch to spec file.

Thanks again.

Regards, Devrim

Wow, that'll teach me to sleep. :) Thanks, good catches, Ralf. 

Is there an updated spec/SRPM addressing the above yet?

Forgot to set flag. . .

(In reply to comment #18)
> Is there an updated spec/SRPM addressing the above yet?

Spec file:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II.spec

SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-4.src.rpm

Regards, Devrim

BRs, MD5 and man8 issues fixed.  Ralf's issues have been addressed.
APPROVED.
I'll start looking at 229322.

New Package CVS Request
=======================
Package Name: pgpool-II
Short Description: Connection pooling/replication server for PostgreSQL
Owners: devrim
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC: devrim

Opps, fixing package name:

New Package CVS Request
=======================
Package Name: postgresql-pgpool-II
Short Description: Connection pooling/replication server for PostgreSQL
Owners: devrim
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC: devrim

Regards, Devrim

Package submitted for build.

Has this been fixed yet?

postgresql-pgpool - 3.2-1.fc7.i386
  File conflict with: postgresql-pgpool-II - 1.1-1.fc8.i386
     /usr/bin/pgpool

postgresql-pgpool-II - 1.1-1.fc8.i386
  File conflict with: postgresql-pgpool - 3.2-1.fc7.i386
     /usr/bin/pgpool

I can't believe I didn't catch this.  

My recommendation would be to rename /u/b/pgbool to /u/b/pgpool-II and patch the
rest to accommodate.

Hello,

Good point, but I think we should just obsolete postgresql-pgpool. pgpool-II and
pgpool (-I) should not be run at the same server --pgpool-II can do what pgpool
can do, plus it has some other features.

If adding a Obsoletes: is ok for everyone, I'll go with it.

Regards, Devrim

Well, since you're the pgpool maintainer, I can see no problem with that.

First, a question: Why is it that postgresql-pgpool-II has been pushed into the
fedora repo before this package is approved?

postgresql-pgpool-II needs an init-script to start pgpool as a service.

See flag and comment #21.  But you're right, it could use an init script. 
Should this have been a blocker?

I pushed init script and /etc/sysconfig/pgpool to -devel. Could you please test?

Regards, Devrim

/etc/sysconfig/pgpool is 755, shouldn't it be 644?  Otherwise, looks great.

Although I don't see it in the system services menu in the text mode config
tools.   

Hi,

(In reply to comment #32)
> /etc/sysconfig/pgpool is 755, shouldn't it be 644?  Otherwise, looks great.

Oops, done.

(In reply to comment #33)
> Although I don't see it in the system services menu in the text mode config
> tools.   

Added:

chkconfig --add pgpool

I am working on these packages nowadays -- Found some problems while using with
the web interface. I will submit it for building ASAP.

Regards, Devrim

This can probably be closed, no?

No. I'm waiting to push pgpool-ha.

Regards, Devrim

Ok, just checking.

(In reply to comment #36)
> No. I'm waiting to push pgpool-ha.
> 
> Regards, Devrim

-ha has been pushed, and this package has even been updated a few times. Closable?