a bunch of potential improvements to the postgresql spec files

public inbox for [email protected]  
help / color / mirror / Atom feed

a bunch of potential improvements to the postgresql spec files
4+ messages / 2 participants
[nested] [flat]

* a bunch of potential improvements to the postgresql spec files
@ 2016-12-02 20:06  Jonathon Nelson <[email protected]>
  0 siblings, 2 replies; 4+ messages in thread

From: Jonathon Nelson @ 2016-12-02 20:06 UTC (permalink / raw)
  To: pgsql-pkg-yum

Please find below a list of various improvements that we've made to the
specfile for PostgreSQL 9.x.



0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir},
and /usr/share/man with %{_mandir}

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

2. add min version to flex dependency:
BuildRequires: flex >= 2.5.31

3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel
version tests

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

5. Remove redundant strip of -ffast-math from CFLAGS

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

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

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


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

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



--
Jon Nelson
Dyn / Principal Software Engineer


^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: a bunch of potential improvements to the postgresql spec files
@ 2016-12-09 18:41  Devrim Gündüz <[email protected]>
  parent: Jonathon Nelson <[email protected]>
  1 sibling, 1 reply; 4+ messages in thread

From: Devrim Gündüz @ 2016-12-09 18:41 UTC (permalink / raw)
  To: Jonathon Nelson <[email protected]>; pgsql-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

^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: a bunch of potential improvements to the postgresql spec files
@ 2017-01-01 15:05  Devrim Gündüz <[email protected]>
  parent: Jonathon Nelson <[email protected]>
  1 sibling, 0 replies; 4+ messages in thread

From: Devrim Gündüz @ 2017-01-01 15:05 UTC (permalink / raw)
  To: Jonathon Nelson <[email protected]>; pgsql-pkg-yum


Hi Jonathon,

Thanks for the report. Committed parts of this to spec file:

https://git.postgresql.org/gitweb/?p=pgrpms.git;a=commit;h=b7811829fdbf46b0248f8f0f1c3ca471616b7e04

Regards, Devrim

On Fri, 2016-12-02 at 14:06 -0600, Jonathon Nelson wrote:
> Please find below a list of various improvements that we've made to the
> specfile for PostgreSQL 9.x.
> 
> 
> 
> 0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir},
> and /usr/share/man with %{_mandir}
> 
> 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
> 
> 2. add min version to flex dependency:
> BuildRequires: flex >= 2.5.31
> 
> 3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel
> version tests
> 
> 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
> 
> 5. Remove redundant strip of -ffast-math from CFLAGS
> 
> 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
> 
> 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
> 
> 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
> 
> 
> 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
> 
> 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
> 
> 
> 
> --
> Jon Nelson
> Dyn / Principal Software Engineer

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

^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: a bunch of potential improvements to the postgresql spec files
@ 2017-01-03 16:14  Jonathon Nelson <[email protected]>
  parent: Devrim Gündüz <[email protected]>
  0 siblings, 0 replies; 4+ messages in thread

From: Jonathon Nelson @ 2017-01-03 16:14 UTC (permalink / raw)
  To: Devrim Gündüz <[email protected]>; +Cc: pgsql-pkg-yum

On Fri, Dec 9, 2016 at 12:41 PM, Devrim Gündüz <[email protected]> wrote:

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


Ah!  I think I meant to suggest this:

%if 0%{rhel} > 6
(note the addition of the question mark).  That should be logically
equivalent to
%if 0%{?rhel} && 0%{rhel} > 6

>
...

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

Thanks!  I found that not having ready access to the latest versions of
many of those binaries (without having to specify the full path) was a
pain. Plus, isn't that why alternatives exists?

I will try to push these changes in next minor release set.
>

Thanks!


^ permalink  raw  reply  [nested|flat] 4+ messages in thread

end of thread, other threads:[~2017-01-03 16:14 UTC | newest]

Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2016-12-02 20:06 a bunch of potential improvements to the postgresql spec files Jonathon Nelson <[email protected]>
2016-12-09 18:41 ` Devrim Gündüz <[email protected]>
2017-01-03 16:14   ` Jonathon Nelson <[email protected]>
2017-01-01 15:05 ` Devrim Gündüz <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox