Bug 244593 - Review Request: pgbouncer - Lightweight connection pooler for PostgreSQL
Summary: Review Request: pgbouncer - Lightweight connection pooler for PostgreSQL
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-06-17 21:09 UTC by Devrim GUNDUZ
Modified: 2008-08-31 18:25 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-31 18:25:38 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
New .spec file (1.43 KB, text/x-rpm-spec)
2007-06-18 17:12 UTC, David Fetter
no flags Details
Signed source RPM file. (94.75 KB, application/x-rpm)
2007-06-18 17:13 UTC, David Fetter
no flags Details

Description Devrim GUNDUZ 2007-06-17 21:09:58 UTC
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer.spec
SRPM URL: 
http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer-1.0.7-2.fc7.src.rpm
Description: 
pgbouncer is a lightweight connection pooler for PostgreSQL.
pgbouncer uses libevent for low-level socket handling.

Comment 1 Jason Tibbitts 2007-06-17 22:07:48 UTC
Why did you set fedora-review to '?' here?  I don't see that anyone is reviewing
this package, so the only effect is that this ticket doesn't appear on the list
of new review tickets and thus will never see a review.

Comment 2 Devrim GUNDUZ 2007-06-18 05:14:46 UTC
Hello,

.(In reply to comment #1)
> Why did you set fedora-review to '?' here?

Sorry, reverted.

Regards, Devrim

Comment 3 David Fetter 2007-06-18 17:12:34 UTC
Created attachment 157296 [details]
New .spec file

Comment 4 David Fetter 2007-06-18 17:13:25 UTC
Created attachment 157297 [details]
Signed source RPM file.

Comment 6 Devrim GUNDUZ 2007-11-07 04:39:38 UTC
Already update to 1.1 in my repo, but forgot to paste here:

Spec URL:

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

SRPM URL:

http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer-1.1.1-1.fc8.src.rpm

Regards, Devrim

Comment 7 Stephen Warren 2007-11-19 05:58:49 UTC
A few quick comments:

URLs given for URL and Source0 don't work. The hostname on its own doesn't even
produce anything useful either.

Description seems a little short, but maybe it's OK. If/when the website comes
back, I would check it against what it says there.

I have no idea what the following does. It might be useful to explain what it
does and why it's needed.
%define debug 0
%{?debug:%define __os_install_post /usr/lib/rpm/brp-compress}

Being anal, there should be a blank line after the Summary line.

I'd prefer to reformat this:

%build
%configure \
%if %debug
	--enable-debug \
	--enable-cassert \
%endif
--datadir=%{_datadir} 

... to this:

%build
%configure \
%if %debug
    --enable-debug \
    --enable-cassert \
%endif
    --datadir=%{_datadir}

i.e. line up the options so it's more obvious they're all one command. Also I
hate TABs, but that may be a personal thing.

I've seen some reviews request that file permissions in %files be more explicit.
Instead of relying on %install to set them correctly, you may want to do
something explicit, modelled after the following:

%files
%defattr(0644,root,root,0755)
%doc COPYING
%doc README.txt
%attr(0755, root, root) /sbin/fxload
%{_mandir}/*/*


Comment 8 Ruben Kerkhof 2008-01-20 13:49:42 UTC
The source tarball doesn't match the upstream one. Devrim, care to do an update?

Comment 9 Jason Tibbitts 2008-03-01 19:21:07 UTC
Any update?  This has been set to needinfo for nearly six weeks, but I know
Devrim is around frequently so I'll keep this open.  I will close it soon if
there's no response.

Comment 10 Devrim GUNDUZ 2008-03-02 04:10:15 UTC
Sorry Ruben and Jason. I have made changes in the spec, and we got rid of
rpmlint errors. Also, updated to 1.1.2 :

Spec file:
http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer.spec
SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer-1.1.2-1.f8.src.rpm



Comment 11 Devrim GUNDUZ 2008-03-08 00:45:19 UTC
I fixed many bugs in init script and spec. Also, added a new patch for .ini file
to match Red Hat defaults.

This new version is now used on a production server:

Spec file:
http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer.spec
SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/postgresql-pgbouncer-1.1.2-2.f8.src.rpm

But BTW: I'm inclined to change the name to pgbouncer only. Adding postgresql-
prefix to all of the packages does not seem good to me, as I felt before.

Regards, Devrim


Comment 12 Devrim GUNDUZ 2008-05-21 04:30:05 UTC
Any comments on the latest version of this package?

Regards, Devrim

Comment 13 Mamoru TASAKA 2008-05-21 17:50:58 UTC
For 1.1.2-2:

* License
  - For this package use "MIT and BSD" as license tag.
    Actually almost all source codes are under MIT,
    only src/md5.{h,c} are under BSD.

* compile log
  http://koji.fedoraproject.org/koji/getfile?taskID=622998&name=build.log
  - Please make compile log more verbose. From outputs like
-------------------------------------------------
make[1]: Leaving directory `/builddir/build/BUILD/pgbouncer-1.1.2/doc'
	CC src/loader.c
	CC src/client.c
	CC src/objects.c
	CC src/proto.c
	CC src/pooler.c
-------------------------------------------------
    we cannot check if compiler flags are passed correctly,
    binaries are stripped by accident, etc..
-------------------------------------------------    
    make %{?_smp_mflags} V=1
-------------------------------------------------
    seems okay

* Timestamp
  - When using "install" or "cp" commands, add "-p" option to
    keep timestamps on installed files.

* Permission
  - Why does %_sysconfdir/sysconfig/%name have not 0644 but 0755
    permission?

* __os_install_post
  - Again from build.log currently only /usr/lib/rpm/brp-compress
    is executed as __os_install_post, which is wrong (you can compare
    with:
    http://koji.fedoraproject.org/koji/getfile?taskID=622988&name=build.log
    for example)
    Note:
-------------------------------------------------
%{?debug:%define __os_install_post /usr/lib/rpm/brp-compress}
-------------------------------------------------
    means that if %debug macro _is defined_ (not is non-zero)
    __os_install_post is defined as /usr/lib/rpm/brp-compress.

* debuginfo missing
  - debuginfo rpm is not correctly created
    * One reason is due to wrong __os_installed_post as described above
    * Another reason is that installed pgbouncer binary are actually
      stripped as "install -s".
      Overriding BININSTALL works for this issue.

* initscript scriptlets
  - Please follow
    http://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets
    * Especially check "Requires(post)" or so

* Documents
  - Please add the following files to %doc
-------------------------------------------------
AUTHORS
-------------------------------------------------

Comment 14 Mamoru TASAKA 2008-05-29 18:01:05 UTC
ping?

Comment 15 Mamoru TASAKA 2008-06-06 12:22:58 UTC
ping again?

Comment 16 Mamoru TASAKA 2008-06-15 11:46:49 UTC
ping again??

Comment 17 Devrim GUNDUZ 2008-06-15 11:58:13 UTC
Please wait, I'm working on about 50 packages nowadays (mostly pg related), and
I'll respond as soon as I finish working on them.

FWIW, the many of the changes are done already... 


Comment 18 Mamoru TASAKA 2008-07-16 17:07:37 UTC
ping again?

Comment 19 Mamoru TASAKA 2008-07-27 12:46:39 UTC
ping again?

Comment 20 Mamoru TASAKA 2008-08-06 13:49:47 UTC
Again ping?

I will close this bug if no response from the reporter is received within
ONE WEEK:
http://fedoraproject.org/wiki/PackageMaintainers/Policy/StalledReviews#Submitter_not_responding

Comment 22 Mamoru TASAKA 2008-08-11 08:52:41 UTC
Thank you for replying. Then before checking your latest srpm:

(In reply to comment #21)
> BTW... Would you object if I change the package name to pgbouncer only?

Well, in my recognition when we rename a package "python-foo" "perl-baa" "ruby-XXX"
it usually means that the package provides a "module" or an extension function
of the main language (python, perl, ...), i.e. usually it is less useful to install
the package _only_ and other application uses the package.

For this package it installs some binaries under %_bindir and it seems this package
is not only a module of postgresql. So I don't object to renaming.

Comment 23 Mamoru TASAKA 2008-08-11 13:33:07 UTC
If you want to rename the srpm, then I will wait for it before checking your
srpm again.

Comment 25 Mamoru TASAKA 2008-08-12 16:17:34 UTC
For 1.2.3-1:

* Requires(preun)
  https://fedoraproject.org/wiki/Packaging/SysVInitScripts#Initscripts_in_spec_file_scriptlets
  - %preun uses /sbin/service and "Requires(preun): initscripts" is needed

! %postun
  - Would you consider to add %postun section as described in
    https://fedoraproject.org/wiki/Packaging/SysVInitScripts#Initscripts_in_spec_file_scriptlets
    (If there is a reason adding this %postun must be avoided, it
     is no problem)

* cflags/install flags
  - build.log shows
---------------------------------------------------------
   201  make[1]: Leaving directory `/builddir/build/BUILD/pgbouncer-1.2.3/doc'
   202  gcc -c -o lib/client.o src/client.c -DHAVE_CONFIG_H -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  -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wmissing-prototypes -Wpointer-arith -Wendif-labels -Wdeclaration-after-statement -Wold-style-definition -Wstrict-prototypes -Wundef -Wformat -Wnonnull -Wstrict-overflow -fomit-frame-pointer -I./include -I./include


   269  mkdir -p /builddir/build/BUILDROOT/pgbouncer-1.2.3-1.fc10.i386/usr/share/doc/pgbouncer
   270  install -s -m 755 ./pgbouncer /builddir/build/BUILDROOT/pgbouncer-1.2.3-1.fc10.i386/usr/bin
---------------------------------------------------------
    -- -fomit-frame-pointer makes debugging very difficult
    -- "install -s" removes debugging information and makes failure of
       debuginfo rpm
    So both are forbidden on Fedora.
    For this package the following can fix these issues:
---------------------------------------------------------
sed -i.fedora \
	-e 's|-fomit-frame-pointer||' \
	-e '/BININSTALL/s|-s||' \
	configure
---------------------------------------------------------

* Timestamps
  - When installing files by "cp" or "install", add "-p" option to
    keep timestamps on them.

* Permission
---------------------------------------------------------
pgbouncer.src: W: strange-permission pgbouncer.init 0775
pgbouncer.i386: E: executable-marked-as-config-file /etc/sysconfig/pgbouncer
pgbouncer.i386: E: script-without-shebang /etc/sysconfig/pgbouncer
---------------------------------------------------------
  - All files in the srpm must have 0644 permission.
  - %_sysconfdir/sysconfig/pgbouncer must have 0644 permission.

* %config
---------------------------------------------------------
pgbouncer.i386: W: conffile-without-noreplace-flag /etc/sysconfig/pgbouncer
---------------------------------------------------------
  - Would you explain why you don't want to use 
    %config(noreplace) %{_sysconfdir}/sysconfig/%{name}?

Comment 26 Mamoru TASAKA 2008-08-29 06:16:50 UTC
ping again?

Comment 27 Devrim GUNDUZ 2008-08-29 06:45:16 UTC
Here is the new spec and srpm:

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

http://developer.postgresql.org/~devrim/rpms/other/pgbouncer/pgbouncer-1.2.3-2.f9.src.rpm

rpmlint is clean, and I believe I applied all changes you suggested.

Regards, Devrim

Comment 28 Mamoru TASAKA 2008-08-29 17:56:23 UTC
For 1.2.3-2:

* %postun scriptlet
  - Now Requires(postun) is needed:
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

* rpmlint issue
---------------------------------------------------
pgbouncer.i386: E: init-script-non-executable /etc/rc.d/init.d/pgbouncer
---------------------------------------------------
  - /etc/rc.d/init.d/pgbouncer must have 0755 permission.

Fix these 2 issues and I guess I can approve this package.

Comment 30 Mamoru TASAKA 2008-08-30 17:23:04 UTC
Okay.

----------------------------------------------------------------
        This package (pgbouncer) is APPROVED by mtasaka
----------------------------------------------------------------

Comment 31 Devrim GUNDUZ 2008-08-30 17:41:16 UTC
BTW, I just added >= 1.3b to libevent-devel dependency -- this version explicitly asks for that (which is available in F-8 +

Comment 32 Devrim GUNDUZ 2008-08-30 17:43:57 UTC
New Package CVS Request
=======================
Package Name: pgbouncer
Short Description:  Lightweight connection pooler for PostgreSQL
Owners: devrim
Branches: F-8 F-9 
InitialCC:

Comment 33 Kevin Fenzi 2008-08-30 21:00:25 UTC
cvs done.

Comment 34 Devrim GUNDUZ 2008-08-31 18:25:38 UTC
Thanks. Pushed package to repositories.


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