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-1.src.rpm Description: pgpool-II is a connection pooling/replication server for PostgreSQL. pgpool-II runs between PostgreSQL's clients(front ends) and servers (backends). A PostgreSQL client can connect to pgpool-II as if it were a standard PostgreSQL server.
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?