Bug 199679 - Review Request: postgresql-pgpool - Connection pooling/replication server for PostgreSQL
Review Request: postgresql-pgpool - Connection pooling/replication server for...
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-07-21 07:19 EDT by Devrim GUNDUZ
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

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


Attachments (Terms of Use)

  None (edit)
Description Devrim GUNDUZ 2006-07-21 07:19:51 EDT
Spec URL: http://pgfoundry.org/frs/download.php/978/pgpool.spec
SRPM URL: http://pgfoundry.org/frs/download.php/979/pgpool-3.1.0-1.src.rpm
Description: pgpool is a connection pooling/replication server for PostgreSQL.
pgpool runs between PostgreSQL's clients(front ends) and servers
(backends). A PostgreSQL client can connect to pgpool as if it
were a standard PostgreSQL server.
Comment 1 Michał Bentkowski 2006-07-22 11:41:21 EDT
I have done quick review and I noticed a few things you have to do:

 * add %{?dist} tag to release number. http://fedoraproject.org/wiki/Packaging/
DistTag
 * Source0 have to be URL, actually it is unable to check if archive in src.rpm
and archive on .net checksums is matching. For more information read:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines#head-
2f03bba0a9f05b2ac0128eb1d70b1e3ce9f9dc40
 * you should use %configure macro instead of hardcoding it
 * Packaging Guidelines suggest to use make %{?_smp_flags} instead of 'make'
alone
 * in %install section you forgot to write $RPM_BUILD_ROOT before %{_mandir}
 * man file has wrong permissions (0744), you have to change it to 644 typing:
%attr(0644, root, root) %{_mandir}/man8/* in %section
Comment 2 Michał Bentkowski 2006-07-22 11:42:27 EDT
Obviously, not in %section, but in %files section ;-)
Comment 3 Devrim GUNDUZ 2006-07-23 06:05:41 EDT
I'll comment on this after we release pgpool 3.1.1 RSN. 

Regards. 
Comment 4 Devrim GUNDUZ 2006-07-23 08:23:28 EDT
H(In reply to comment #1)
> I have done quick reviewi,

 and I noticed a few things you have to do:
> 
>  * add %{?dist} tag to release number. 
Done.

>  * Source0 have to be URL, actually it is unable to check if archive in
src.rpm and archive on .net checksums is matching.

Done.

>  * you should use %configure macro instead of hardcoding it

Done.

>  * Packaging Guidelines suggest to use make %{?_smp_flags} instead of 'make'
> alone

Done.

>  * in %install section you forgot to write $RPM_BUILD_ROOT before %{_mandir}
>  * man file has wrong permissions (0744), you have to change it to 644 typing:
> %attr(0644, root, root) %{_mandir}/man8/* in %section

I solved this during %install.

I'll post the URLs as another comment. Thanks a lot.

Regards, Devrim
Comment 5 Devrim GUNDUZ 2006-07-23 08:55:31 EDT
New spec file for 3.1.1:
http://pgfoundry.org/frs/download.php/984/pgpool.spec

New SRPM for 3.1.1:
http://pgfoundry.org/frs/download.php/983/pgpool-3.1.1-1.src.rpm

Comment 6 Michał Bentkowski 2006-07-23 09:27:40 EDT
 * You got an error in changelog entry. It starts with asterisk instead
of hyphen.
 * The %{?smp_flags} should be after make in %build section, not in
make install.
 * You should use %{_sysconfdir} macro in %configure instead of /etc
hard-coding.
Comment 7 Devrim GUNDUZ 2006-07-23 09:54:40 EDT
Hi,

(In reply to comment #6)
>  * You got an error in changelog entry. It starts with asterisk instead
> of hyphen.
>  * The %{?smp_flags} should be after make in %build section, not in
> make install.
>  * You should use %{_sysconfdir} macro in %configure instead of /etc
> hard-coding.

All done. Thanks for the contribution.

Regards, Devrim
Comment 9 Chris Weyl 2006-07-28 18:17:38 EDT
A couple thoughts:

* With the usage of %configure in %build, you can drop "--bindir=%{_bindir} ..."
from that line (%configure includes them)
* I'm not sure of the best practice in this area myself, but it would seem to
make sense to put a .conf.sample file in %doc, rather than %{_sysconfdir}; maybe
either placing a default config in there (minus the .sample) or %ghost'ing
%{_sysconfdir}/pgpool.conf.  (/me invites comments)
Comment 10 Devrim GUNDUZ 2006-07-31 07:26:45 EDT
Sorry for the late response.

I've removed --bindir part. Thanks.

I am also not sure about your second review. Is there anything strict in the
guidelines?

Regards, Devrim
Comment 12 Kevin Fenzi 2006-09-09 20:57:43 EDT
See below - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (BSD)
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:
8adb39f18780a93e4b2ac0e31364314d  pgpool-3.1.1.tar.gz
8adb39f18780a93e4b2ac0e31364314d  pgpool-3.1.1.tar.gz.1
OK - Package compiles and builds on at least one arch.
OK - BuildRequires correct
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package doesn't own any directories other packages own.
See below - No rpmlint output. 

SHOULD Items:

OK - Should include License or ask upstream to include it.
OK - Should build in mock.

Issues:

1. Should this package be named 'postgresql-pgpool', since it
"enhances or adds a new functionality to an existing Fedora Core 
or Fedora Extras package without being useful on its own"

2. rpmlint says:

W: pgpool incoherent-version-in-changelog 3.1.1-4 3.1.1-3.fc6

Might fix the changelog to not have .fc6 in it. (minor).

3. I'd have to agree with the point in comment #9 about the
/etc/pgpool.conf.sample file. You should either construct a
/etc/pgpool.conf file for there, or ship no config file and
put the .sample file under doc or the like. Looking at that
file I think you could just install it as /etc/pgpool.conf
and let the end user modify it if they have a diffrent config.

4. Is the "Requires:       postgresql-server" correct?
Couldn't this run on a machine with no local postgresql-server
installed, talking to remote machines with it installed?

5. In agreement with comment #9 again, you shouldn't need
to pass anything to the %configure macro. You have:
%configure --sysconfdir=%{_sysconfdir} --mandir=%{_mandir} --libdir=%{_libdir}

but %configure expands to a configure call that already passes
all those values. You can simply have:

%configure

See /usr/lib/rpm/macros for more information.
Comment 13 Kevin Fenzi 2006-09-17 19:24:07 EDT
Ping Devrim. 

Are you still interested in submitting this? 
I haven't seen any reply since the review on the 9th. Just want to make sure 
you are still wanting to move forward with this at some point. 
Comment 14 Guillaume Smet 2006-09-18 01:58:59 EDT
Kevin,

Devrim is on vacation at the moment (10 days left IIRC). No doubt he'll work on
this again when he'll be back.
Comment 15 Kevin Fenzi 2006-09-18 11:11:25 EDT
Excellent. Thanks for the info. I will look forward to hearing from him when he 
gets back. :) 
Comment 16 Kevin Fenzi 2006-10-08 00:16:55 EDT
Devrim: Are you back from vacation yet? Any chance to move this review forward?
Comment 17 Kevin Fenzi 2006-11-11 20:40:22 EST
Ping Devrim. Any news on this package?
Comment 18 Devrim GUNDUZ 2006-11-24 08:29:27 EST
Hello,

(In reply to comment #12)
> Issues:
> 
> 1. Should this package be named 'postgresql-pgpool', since it
> "enhances or adds a new functionality to an existing Fedora Core 
> or Fedora Extras package without being useful on its own"

Good point. Done.
 
> 2. rpmlint says:
> 
> W: pgpool incoherent-version-in-changelog 3.1.1-4 3.1.1-3.fc6
> 
> Might fix the changelog to not have .fc6 in it. (minor).

Fixed, thanks.

> 3. I'd have to agree with the point in comment #9 about the
> /etc/pgpool.conf.sample file. You should either construct a
> /etc/pgpool.conf file for there, or ship no config file and
> put the .sample file under doc or the like. Looking at that
> file I think you could just install it as /etc/pgpool.conf
> and let the end user modify it if they have a diffrent config.

Ok, moved that file to docdir.
 
> 4. Is the "Requires:       postgresql-server" correct?
> Couldn't this run on a machine with no local postgresql-server
> installed, talking to remote machines with it installed?

I was aware of that problem, and removed that already.

> 5. In agreement with comment #9 again, you shouldn't need
> to pass anything to the %configure macro. You have:
> %configure --sysconfdir=%{_sysconfdir} --mandir=%{_mandir} --libdir=%{_libdir}
> but %configure expands to a configure call that already passes
> all those values. You can simply have:
> 
> %configure
 

I need to pass them, because configure script has some hardcoded values in it.

New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool/postgresql-pgpool.spec
New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool/postgresql-pgpool-3.1.1-5.src.rpm

Regards, Devrim
Comment 19 Kevin Fenzi 2006-11-27 23:20:37 EST
1 thru 4: ok. 

On item 5:

The %configure macro already passes those variables (along with many others):

%configure \
  CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS ; \
  CXXFLAGS="${CXXFLAGS:-%optflags}" ; export CXXFLAGS ; \
  FFLAGS="${FFLAGS:-%optflags}" ; export FFLAGS ; \
  ./configure --host=%{_host} --build=%{_build} \\\
        --target=%{_target_platform} \\\
        --program-prefix=%{?_program_prefix} \\\
        --prefix=%{_prefix} \\\
        --exec-prefix=%{_exec_prefix} \\\
        --bindir=%{_bindir} \\\
        --sbindir=%{_sbindir} \\\
        --sysconfdir=%{_sysconfdir} \\\
        --datadir=%{_datadir} \\\
        --includedir=%{_includedir} \\\
        --libdir=%{_libdir} \\\
        --libexecdir=%{_libexecdir} \\\
        --localstatedir=%{_localstatedir} \\\
        --sharedstatedir=%{_sharedstatedir} \\\
        --mandir=%{_mandir} \\\
        --infodir=%{_infodir}

If you were calling ./configure you would have to pass those, but using the
%configure macro I don't see how it's needed. Perhaps I'm missing something?

Comment 20 Devrim GUNDUZ 2006-11-28 00:41:40 EST
Hello,

As I wrote before, it is a problem with pgpool configure script. You can try by
cleaning up configure line in the spec file. You will get this error:

 Checking for unpackaged file(s): /usr/lib/rpm/check-files
/var/tmp/postgresql-pgpool-3.1.1-6-root-root
error: Installed (but unpackaged) file(s) found:
   /etc/pgpool.conf.sample


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /etc/pgpool.conf.sample

Anyway, I'll truncate the other parts.
Comment 22 Kevin Fenzi 2006-11-28 01:01:17 EST
ok, I see what you are trying to do. Thats not going to work however. 

Setting '--sysconfdir=%{_docdir}/%{name}-%{version}' will cause the package not
to function correctly. It's going to be looking for the config file in that
location...ie, to configure the package a user will have to create a 
/usr/share/doc/postgresql-pgpool-3.1.1-6/pgpool.conf file. Thats not the desired
result. 

Possible solutions: 

- Change configure back to just %configure, then at the end of your %install
section 'rm -f %{buildroot}/%{_sysconfdir}/pgpool.conf.sample'

- Change configure back to just %configure, then at the end of your %install
section 'mv %{buildroot}/%{_sysconfdir}/pgpool.conf.sample
%{buildroot}/%{_sysconfdir}/pgpool.conf'

What does pgpool do when you don't have a pgpool.conf file? If it gives a
coherent error message, then option 1 should be fine, if it doesn't then perhaps
  option 2 would be better to provide more info for the user. 
Comment 23 Kevin Fenzi 2006-12-03 18:06:43 EST
Hey Devrim. 

Did comment #22 make sense? Or did I miss something?

If you like I can try and test and see what it does with no config file
available... I think we should do whatever option gives the most info to a user
that installs the package and tries to use it out of the box. 
Comment 24 Devrim GUNDUZ 2006-12-05 08:10:51 EST
Hi,

(In reply to comment #23)
> Did comment #22 make sense? Or did I miss something?

Ok, I think we made it:
New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool/postgresql-pgpool.spec

New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool/postgresql-pgpool-3.1.1-7.src.rpm

What is left for approval? 

Regards, Devrim
Comment 25 Kevin Fenzi 2006-12-05 13:16:38 EST
Everything looks great with the version in comment #24. 

This package is APPROVED. 

Don't forget to close this review request NEXTRELEASE once it's been imported
and built. 

Also, consider reviewing another waiting package to help spread out the
reviewing load. 
Comment 26 Kevin Fenzi 2006-12-21 22:03:19 EST
Fixing the Summary for tracking purposes. 
Comment 27 Devrim GUNDUZ 2007-07-18 07:44:56 EDT
Package Change Request
======================
Package Name: postgresql-pgpool
New Branches: EL-4 EL-5
Comment 28 Kevin Fenzi 2007-07-18 12:09:54 EDT
cvs done.

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