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

[Dave Reisner]

On Tue, Jul 30, 2013 at 04:37:50PM +0200, Dominik Heidler wrote:
> 
> 
> 
> On Tue, Jul 30, 2013 at 4:08 PM, Dave Reisner <d at falconindy.com> wrote:
> 
>     On Tue, Jul 30, 2013 at 03:55:26PM +0200, Dominik Heidler wrote:
>     > Signed-off-by: Dominik Heidler <dheidler at gmail.com>
>     > ---
>     >  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
>     >
> 
>