Bug 199679

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

Obviously, not in %section, but in %files section ;-)

I'll comment on this after we release pgpool 3.1.1 RSN. 

Regards. 

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

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

 * 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.

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

New spec:
http://pgfoundry.org/frs/download.php/986/pgpool.spec
New srpm:
http://pgfoundry.org/frs/download.php/987/pgpool-3.1.1-2.src.rpm

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)

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

New spec:

http://developer.postgresql.org/~devrim/rpms/other/pgpool/pgpool.spec

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

Regards, Devrim

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.

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. 

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.

Excellent. Thanks for the info. I will look forward to hearing from him when he 
gets back. :) 

Devrim: Are you back from vacation yet? Any chance to move this review forward?

Ping Devrim. Any news on this package?

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

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?

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.

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-6.src.rpm

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. 

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. 

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

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. 

Fixing the Summary for tracking purposes. 

Package Change Request
======================
Package Name: postgresql-pgpool
New Branches: EL-4 EL-5

cvs done.