On Tue, Jul 30, 2013 at 4:08 PM, Dave Reisner <[email protected]> wrote:
> On Tue, Jul 30, 2013 at 03:55:26PM +0200, Dominik Heidler wrote: > > Signed-off-by: Dominik Heidler <[email protected]> > > --- > > 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 > > +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. > > > /chrootbuild > > then > > move_products > > -- > > 1.8.3.4 > > >
