Re: [arch-projects] [devtools] [PATCH] add ccache support

30 Jul 2013

      On Tue, Jul 30, 2013 at 04:37:50PM +0200, Dominik Heidler wrote:
...
On Tue, Jul 30, 2013 at 4:08 PM, Dave Reisner  wrote:
On Tue, Jul 30, 2013 at 03:55:26PM +0200, Dominik Heidler wrote:
    > Signed-off-by: Dominik Heidler 
    > ---
    >  makechrootpkg.in | 13 ++++++++++++-
    >  1 file changed, 12 insertions(+), 1 deletion(-)
    >
    > diff --git a/makechrootpkg.in b/makechrootpkg.in
    > index d7d3ecf..d4c6abe 100644
    > --- a/makechrootpkg.in
    > +++ b/makechrootpkg.in
    > @@ -21,6 +21,8 @@ run_namcap=false
    >  temp_chroot=false
    >  chrootdir=
    >  passeddir=
    > +ccachedir=
    > +passedccdir=
    >  declare -a install_pkgs
    >  declare -i ret=0
    >
    > @@ -58,10 +60,11 @@ usage() {
    >       echo "           Default: $copy"
    >       echo '-n         Run namcap on the package'
    >       echo '-T         Build in a temporary directory'
    > +     echo '-C <dir>   The ccache directory to use'
    >       exit 1
    >  }
    >
    > -while getopts 'hcur:I:l:nT' arg; do
    > +while getopts 'hcur:I:l:C:nT' arg; do
    >       case "$arg" in
    >               h) usage ;;
    >               c) clean_first=true ;;
    > @@ -71,6 +74,7 @@ while getopts 'hcur:I:l:nT' arg; do
    >               l) copy="$OPTARG" ;;
    >               n) run_namcap=true; makepkg_args="$makepkg_args -i" ;;
    >               T) temp_chroot=true; copy+="-$$" ;;
    > +             C) passedccdir="$OPTARG" ;;
    >               *) makepkg_args="$makepkg_args -$arg $OPTARG" ;;
    >       esac
    >  done
    > @@ -84,6 +88,12 @@ chrootdir=$(readlink -e "$passeddir")
    >  [[ ! -d $chrootdir ]] && die "No chroot dir defined, or invalid path
    '$passeddir'"
    >  [[ ! -d $chrootdir/root ]] && die "Missing chroot dir root directory.
    Try using: mkarchroot $chrootdir/root base-devel"
    >
    > +# Canonicalize ccachedir, getting rid of trailing /
Ok, but why is this necessary? Based on the comment, I'd say you just
    want to make the assignment dir=${dir%/} and readlink is overkill. But,
    I still don't understand why this is needed.
Ranty/semi-offtopic: Comments should explain why, not what. If the
    "what" needs explaining, then your code is probably too complicated. I
    realize bash is an odd case and sometimes the syntax itself needs
    clarification, but that's clearly not the case here.
> +if [[ "$passedccdir" != "" ]]; then
if [[ -z $passedccdir ]]; then ...
> +     ccachedir=$(readlink -e "$passedccdir")
    > +     [[ ! -d $ccachedir ]] && die "Invalid path '$passedccdir' given for
    ccache directory"
die takes a format string and arguments. please don't inject code into
    the format string.

doesn't seem to work here - and is not done that in the script at all
Hrmm... makechrootpkg.in has "m4_include(lib/common.sh)". lib/common.sh
has:

  error() {
    local mesg=$1; shift
    printf "${RED}==> ERROR:${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
  }

  ...

  die() {
    error "$*"
    cleanup 1
  }

Sigh. die's definition is wrong and must use "$@" instead of "$*". This
*should* work.
...
> +fi
    > +
    >  # Detect chrootdir filesystem type
    >  chroottype=$(stat -f -c %T "$chrootdir")
    >
    > @@ -349,6 +359,7 @@ download_sources
    >  if arch-nspawn "$copydir" \
    >       --bind-ro="$PWD:/startdir_host" \
    >       --bind-ro="$SRCDEST:/srcdest_host" \
    > +     ${ccachedir:+"--bind=$ccachedir:/build/.ccache"} \
Why do you modify the ccachedir path? What guarantee is there that this
    extra path exists on top of the user provided directory? I'm not a
    ccache user, but this seems rather unexpected at a glance.
The build directory is created by makearchrootpkg and the
.ccache subdir is automaticly created by systemd-nspawn.
The paths are different, because nobody can't use root's
.ccache dir and also this allows using different .ccache
directories for different architectures.
Sorry, I misread this and mentally passed over the colon in the --bind
argument.
...

>       /chrootbuild
    >  then
    >       move_products
    > --
    > 1.8.3.4
    >