[pacman-dev] [PATCH 2/2] Use sysconfdir, localstatedir, BASH instead of hard-coded values

[Dan McGee]

On Mon, Oct 11, 2010 at 5:16 PM, Nezmer <git at nezmer.info> wrote:
> On Mon, Oct 11, 2010 at 04:35:46PM -0500, Dan McGee wrote:
>> On Mon, Oct 11, 2010 at 4:20 PM, Nezmer <git at nezmer.info> wrote:
>> > Signed-off-by: Nezmer <git at nezmer.info>
>> > ---
>> >  contrib/Makefile.am           |   18 ++++++++++++++++--
>> >  contrib/bacman.in             |   16 ++++++++--------
>> >  contrib/pacdiff.in            |    2 +-
>> >  contrib/pacscripts.in         |    8 ++++----
>> >  contrib/pactree.in            |   10 +++++-----
>> >  contrib/wget-xdelta.sh.in     |   10 +++++-----
>> >  contrib/zsh_completion.in     |   18 +++++++++---------
>> >  doc/Makefile.am               |   12 ++++++++++++
>> >  doc/makepkg.8.txt             |    2 +-
>> >  doc/pacman.8.txt              |    4 ++--
>> >  doc/pacman.conf.5.txt         |   12 ++++++------
>> >  scripts/Makefile.am           |    3 +++
>> >  scripts/makepkg.sh.in         |    2 +-
>> >  scripts/pacman-optimize.sh.in |    2 +-
>> >  scripts/pkgdelta.sh.in        |    2 +-
>> >  scripts/rankmirrors.sh.in     |    4 ++--
>> >  scripts/repo-add.sh.in        |    2 +-
>> >  17 files changed, 78 insertions(+), 49 deletions(-)
>> >
>> > diff --git a/contrib/Makefile.am b/contrib/Makefile.am
>> > index c6243b1..15dc227 100644
>> > --- a/contrib/Makefile.am
>> > +++ b/contrib/Makefile.am
>> > @@ -9,7 +9,21 @@ EXTRA_DIST = \
>> >        pactree \
>> >        vimprojects \
>> >        wget-xdelta.sh \
>> > -       zsh_completion \
>> > -       README
>> > +       zsh_completion
>> So we aren't going to distribute the README anymore? I don't see where
>> this is included anymore.
>>
>
> Should I rename README to README.in ? Or split the *.in files out in
> another variable and append it to EXTRA_DIST ?

I'd probably keep the name since we don't need to do anything to it.
The *.in files need to be in EXTRA_DIST so `make dist` actually
packages them up; this is a special variable for autoconf/automake. As
hokey as it is, it might be best to use the same OURSCRIPTS route as
scripts/ but not bin_SCRIPTS (or they will be installed by default).

>> > +
>> > +BASH := $(shell sh -c 'which bash|| echo /bin/bash')
>> I think this task belongs in configure.ac rather than in the
>> makefiles. It would then allow overrriding rather than auto-detection.
>> See the AC_CHECK_PROGS([PYTHON]) line already in there, you can pretty
>> much do the exact same thing.
>>
>
> I'll look into it.
>
>> > +
>> > +edit = sed \
>> > +       -e 's|@sysconfdir[@]|$(sysconfdir)|g' \
>> > +       -e 's|@BASH[@]|$(BASH)|g' \
>> > +       -e 's|@localstatedir[@]|$(localstatedir)|g'
>> I know this is nitpicky, but it would be good to keep these in a
>> similar order to the one in scripts/Makefile.am, and keep the
>> directory substitutions together.
>>
>
> Will do.
>
>> > +
>> > +$(EXTRA_DIST): Makefile
>> > +       @rm -f $@ $@.tmp
>> > +       @cp -a $@.in $@.tmp # To reserve permissions in tmp files
>> Comments are better if they don't have typos...
>
> What typos ?

"reserve" -> "preserve". We can probably just kill the comment anyway though.

>> > +       $(edit) $@.in >$@.tmp
>> > +       @mv $@.tmp $@
>> I'm not sure which way of doing this is better (vs. the existing
>> logic), other than I know I cribbed the logic in scripts/Makefile.am
>> from existing scripts. What is the idea of preserving permissions
>> rather than explicit permission setting?
>>
>
> Files in contrib don't have similar permissions (some 755, some 644).
> '>' If I understand correctly gets permissions from umask. So I think
> what I did is the most efficient way to reserve permissions
> automatically.
>
> contrib files are packaged manually, right? So 'make install' wouldn't
> take care of it!

You are correct; I guess they aren't all truely scripts, are they.

>> > +
>> > +all-am: $(EXTRA_DIST)
>> >
>> >  # vim:set ts=2 sw=2 noet:

>> > diff --git a/doc/Makefile.am b/doc/Makefile.am
>> > index 2e656f6..461528f 100644
>> > --- a/doc/Makefile.am
>> > +++ b/doc/Makefile.am
>> > @@ -66,6 +66,10 @@ else
>> >  REAL_PACKAGE_VERSION = $(PACKAGE_VERSION)
>> >  endif
>> >
>> > +edit = sed \
>> > +       -e 's|@sysconfdir[@]|$(sysconfdir)|g' \
>> > +       -e 's|@localstatedir[@]|$(localstatedir)|g'
>> > +
>> >  man_MANS =
>> >  dist_man_MANS = $(ASCIIDOC_MANS) repo-remove.8
>> >
>> > @@ -107,10 +111,18 @@ A2X_OPTS = \
>> >  # These rules are due to the includes and files of the asciidoc text
>> >  $(ASCIIDOC_MANS): asciidoc.conf footer.txt
>> >        a2x $(A2X_OPTS) --asciidoc-opts="$(ASCIIDOC_OPTS)" $@.txt
>> > +       @rm -f $@.tmp
>> > +       @cp -a $@ $@.tmp # To reserve permissions in tmp files
>> > +       $(edit) $@ >$@.tmp
>> > +       @mv $@.tmp $@
>> >
>> >  %.html: %.txt
>> >        asciidoc $(ASCIIDOC_OPTS) $*.txt
>> >        dos2unix $@
>> > +       @rm -f $@.tmp
>> > +       @cp -a $@ $@.tmp # To reserve permissions in tmp files
>> > +       $(edit) $@ >$@.tmp
>> > +       @mv $@.tmp $@
>> Hmm, I don't know at all what you are doing here. We already do
>> sysconfdir substitution using ASCIIDOC_OPTS; surely we can just fix
>> the docs to use that and add localstatedir as well?
>>
>
> I'm not sure what you mean. /etc,/var are hardcoded in *.txt files.
> Are those autoreplaced by asciidoc?

No, your catches are right where you found them. We definitely need to
fix those, we just already do replacement in some places. This is how
we are doing it already:

Synopsis
--------
{sysconfdir}/pacman.conf

So we should just be able to substitute this var replacement where necessary.

>> >  HACKING.html: ../HACKING
>> >        asciidoc $(ASCIIDOC_OPTS) -o $@ ../HACKING