public inbox for [email protected]
help / color / mirror / Atom feedFrom: Devrim Gündüz <[email protected]>
To: Jonathon Nelson <[email protected]>
To: [email protected]
Subject: Re: a bunch of potential improvements to the postgresql spec files
Date: Fri, 09 Dec 2016 21:41:13 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJqAM3ZeLM8qqHJxF0wTOtDGEJDSb2DOmOF+RLNH6BNQ6j=Jw@mail.gmail.com>
References: <CACJqAM3ZeLM8qqHJxF0wTOtDGEJDSb2DOmOF+RLNH6BNQ6j=Jw@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgsql-pkg-yum>
Hi Jonathon,
First of all, thank your for working on this. Comments inline:
On Fri, 2016-12-02 at 14:06 -0600, Jonathon Nelson wrote:
>
> 0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir},
> and /usr/share/man with %{_mandir}
Looks good.
> 1. Remove the redundant use of 0%{?rhel} in tests for RHEL-specific values.
> Turns:
> %if 0%{?rhel} && 0%{rhel} > 6
> into:
> %if 0%{rhel} > 6
If we remove that one, then Fedora builds should fail.
> 2. add min version to flex dependency:
> BuildRequires: flex >= 2.5.31
Ok.
> 3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel
> version tests
AFAIR, we don't need this on RHEL 7 and Fedora. Are you sure that this is safe
to move?
> 4. Remove comma (it's not an error, it's just not as aesthetic) from
> Requires line(s):
>
> -Requires(pre): %{_sbindir}/useradd, %{_sbindir}/groupadd
> +Requires(pre): %{_sbindir}/useradd %{_sbindir}/groupadd
Ok.
> 5. Remove redundant strip of -ffast-math from CFLAGS
Wow, ok.
> 6. replace hard-coded path for PGENGINE variable with %{pgbaseinstdir}:
> # prep the setup script, including insertion of some values it needs
> sed -e 's|^PGVERSION=.*$|PGVERSION=%{version}|' \
> - -e 's|^PGENGINE=.*$|PGENGINE=/usr/pgsql-%{majorversion}/bin|' \
> + -e 's|^PGENGINE=.*$|PGENGINE=%{pgbaseinstdir}/bin|' \
> <%{SOURCE17} >postgresql%{packageversion}-setup
Ok.
> 7. Remove redundant file list init:
>
> # initialize file lists
> %{__cp} /dev/null main.lst
> %{__cp} /dev/null libs.lst
> %{__cp} /dev/null server.lst
> %{__cp} /dev/null devel.lst
> %{__cp} /dev/null plperl.lst
> %{__cp} /dev/null pltcl.lst
> %{__cp} /dev/null plpython.lst
Ok (Funny that noone saw this before
> 8. Replace all repetitive blocks for update-alternatives with for loop
> *AND* use %{mandir} and %{bindir} (per item 0):
>
> %{_sbindir}/update-alternatives --install %{bindir}/psql pgsql-psql
> %{pgbaseinstdir}/bin/psql %{packageversion}0
> %{_sbindir}/update-alternatives --install %{bindir}/clusterdb
> pgsql-clusterdb %{pgbaseinstdir}/bin/clusterdb %{packageversion}0
> ...
> %{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup
> pgsql-pg_basebackup %{pgbaseinstdir}/bin/pg_basebackup %{packageversion}0
> ...
>
> with:
>
> for i in \
> pgbench pg_test_timing pg_upgrade pg_xlogdump pg_archivecleanup \
> pg_config pg_isready pg_test_fsync pg_receivexlog \
> psql clusterdb \
> createdb createlang createuser \
> dropdb droplang dropuser \
> pg_basebackup pg_dump pg_dumpall pg_restore \
> reindexdb vacuumdb; do
> %{_sbindir}/update-alternatives --install %{bindir}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1
> pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
> done
This line creates alternatives for *all* binaries, but we don't do this. We
only create alternatives entries for the binaries that can work across major
releases -- but I liked the approach. Will apply with a few changes.
> 9. in %post for -server, don't overwrite the symlink for 'postmaster' which
> may be supplied by the OS vendor:
>
> # N.B. -- manually excluded 'postmaster' from update-alternatives
> # so that it doesn't conflict with the native vendor-supplied package
> # which does sometimes symlink postmaster to postgres.
> #
> for i in \
> initdb pg_controldata pg_ctl pg_resetxlog postgres; do
> %{_sbindir}/update-alternatives --install %{bindir}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1
> pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
> done
>
> # fixup install of postmaster update-alternatives without wrecking
> # native OS-install of symlink
> if [ "$( readlink /usr/bin/postmaster )" =
> "/etc/alternatives/pgsql-postmaster" ]; then
> %{_sbindir}/update-alternatives --remove pgsql-postmaster
> %{pgbaseinstdir}/bin/postmaster
> %{_sbindir}/update-alternatives --remove pgsql-postmasterman
> %{pgbaseinstdir}/share/man/man1/postmaster.1
> fi
>
> and a similar block in -server's %postun:
>
> # N.B. -- manually excluded 'postmaster' from update-alternatives
> # so that it doesn't conflict with the native OS-supplied package
> # which does sometimes symlink postmaster to postgres.
> #
> if [ "$1" -eq 0 ]
> then
> for i in \
> initdb pg_controldata pg_ctl pg_resetxlog postgres; do
> %{_sbindir}/update-alternatives --remove pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> %{_sbindir}/update-alternatives --remove pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> done
I personally would ignore this part, because I don't think we are interested in
the installations that OS-supplied PG and ours would work together. It would
break many more things.
> 10. Additional update-alternatives for -contrib and -devel:
>
> %post devel
> for i in ecpg; do
> %{_sbindir}/update-alternatives --install %{bindir}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1
> pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
> done
>
> %postun devel
> if [ "$1" -eq 0 ]
> then
> for i in ecpg; do
> %{_sbindir}/update-alternatives --remove pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> %{_sbindir}/update-alternatives --remove pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> done
> fi
>
> %post contrib
> for i in oid2name vacuumlo pg_recvlogical pg_standby; do
> %{_sbindir}/update-alternatives --install %{bindir}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1
> pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
> done
>
> %postun contrib
> if [ "$1" -eq 0 ]
> then
> for i in oid2name vacuumlo pg_recvlogical pg_standby; do
> %{_sbindir}/update-alternatives --remove pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> %{_sbindir}/update-alternatives --remove pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> done
> fi
>
Same as 8.
I will try to push these changes in next minor release set.
Regards,
--
Devrim GÜNDÜZ
EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR
Attachments:
[application/pgp-signature] signature.asc (819B, 2-signature.asc)
download
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: a bunch of potential improvements to the postgresql spec files
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox