[Bug 199154] Review Request: Slony-1 (postgresql-slony-engine)

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: Slony-1 (postgresql-slony-engine)

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=199154

------- Additional Comments From [e-mail]  2007-07-09 02:35 EST -------
Hello,

(In reply to comment #20)
> The upstream project is called Slony-I. Why is the package called

postgresql-slony1-engine?

See configure file for this.
> Source0 is not available.

http://main.slony.info/downloads/1.2/source/slony1-1.2.0.tar.bz2 seems to be the
right one (there's a doc tarball
> as well)

Fixed. (1.2.10) . We don't need doc tarball there; since the main tarball
includes doc tarball, too.
> Errors during build:
>
> chmod -R 644 doc/Makefile doc/adminguide doc/concept doc/howto

doc/implementation doc/support
> chmod: cannot access `doc/adminguide/schemadoc.xml': Permission denied
> chmod: cannot access `doc/adminguide/usingslonik.sgml': Permission denied
> chmod: cannot access `doc/adminguide/slonyupgrade.sgml': Permission denied
> etc...
> the chmod -R 644 sets drw-r--r-- permissions on the directory, so you can't

reach the files in it.
> Maybe you can use something like find doc/ -type f -exec chmod 600 {} ;

Done.
> It also seems to be missing yacc:
>
> Missing yacc parser.y parser.c

? No idea what this means.
> Since postgresql_autodoc is now available, maybe you can add it to the

BuildRequires

I don't think so. Is there anything that depends on autodoc?
> I'd skip the %if %docs and %if %perltools. It's only one perl module, and

that cleans up the specfile a lot.

Removed %perltools, but left docs as it is now -- It is needed, because of the
low NAMELEN issue on some old RH/FC releases. Some people may skip doc builds.
> This isn't necessary:
>  # Strip out -ffast-math from CFLAGS....
>
> CFLAGS=`echo $CFLAGS|xargs -n 1|grep -v ffast-math|xargs -n 100`  
>  There is no -ffast-math in %{optflags}

Ok.
> %configure --includedir %{_includedir}/pgsql --with-pgconfigdir=%{_bindir}
>  is probably not necessary either, pg_config is in the path, and will tell

configure where the libs and headers are

Slony looks for /usr/local/pgsql/bin/pg_config first. If someone has a source
installation of PostgreSQL, then the build will break (see changelog entry Thu
May 17 2007)
> Remove this line: #%define pg_version %(rpm -qv postgresql-devel|head -n

1|awk -F '-' '{print $3}')
> and this one: %define prefix /usr, they're not used

Ok done.
> This won't work:
> if [ -d /etc/rc.d/init.d ]
> then
>     install -d %{buildroot}/etc/rc.d/init.d
> fi
>
> You check if the directory exists, and if it exists, you create it.
> and replace that path with %{_initrddir}. If you add initscripts as a

requirement, that directory should be there

Done.

Thanks for the review. The new spec will follow shortly.

Regards, Devrim
> Good luck!