On Sat, Jan 11, 2014 at 11:43:07AM -0800, Brian Dolbec wrote:
> -		self.settings["target_path"]=normpath(self.settings["storedir"]+\
> -			"/builds/"+self.settings["target_subpath"]+".tar.bz2")
> +		self.settings["target_path"] = normpath(self.settings["storedir"] +
> +			"/builds/" + self.settings["target_subpath"].rstrip('/') +
> +			".tar.bz2")

I find it hard to imagine target_subpath ending with a slash.  If it
did, I think that would be a bug where it's set, not something we
should try to work around here.  In the current master (e5a9e20):

  $ git grep -A2 'target_subpath.*=' | cat
  modules/generic_stage_target.py:                self.settings["target_subpath"]=self.settings["rel_type"]+"/"+\
  modules/generic_stage_target.py-                                self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
  modules/generic_stage_target.py-                                self.settings["version_stamp"]
  --
  modules/snapshot_target.py:             self.settings["target_subpath"]="portage"
  modules/snapshot_target.py-             st=self.settings["storedir"]
  modules/snapshot_target.py-             self.settings["snapshot_path"] = normpath(st + "/snapshots/"

so we should be safe without this change.

> -			self.settings["source_path"]=normpath(self.settings["storedir"]+\
> -				"/builds/"+self.settings["source_subpath"]+".tar.bz2")
> +			self.settings["source_path"] = normpath(self.settings["storedir"] +
> +				"/builds/" + self.settings["source_subpath"].rstrip("/") +
> +				".tar.bz2")

source_subpath comes from the spec file, but I think we should check
for trailing slashes (and error out if we find them) when we load the
spec file, not here.  This also holds for the later source_subpath and
snapshot stripping.

>  			self.settings["snapshot_cache_path"]=\
>  				normpath(self.settings["snapshot_cache"]+"/"+\
> -				self.settings["snapshot"]+"/")
> +				self.settings["snapshot"])

Looks good to me, but doesn't this need a corresponding update to:

    if "SNAPCACHE" in self.settings:
        snapshot_cache_hash=\
            read_from_clst(self.settings["snapshot_cache_path"]+\
            "catalyst-hash")

and also to:

    if "SNAPCACHE" in self.settings:
        myf=open(self.settings["snapshot_cache_path"]+"catalyst-hash","w")
        myf.write(self.settings["snapshot_path_hash"])
        myf.close()

These will still *work* without a properly joined path, but they won't
use their intended …/catalyst-hash file (and may not be cleaned up
correctly).

>  		self.settings["chroot_path"]=normpath(self.settings["storedir"]+\
> -			"/tmp/"+self.settings["target_subpath"]+"/")
> +			"/tmp/"+self.settings["target_subpath"])

Looks good to me, but grepping for chroot_path gives lots of hits, and
I didn't check them all.

> -	local destdir=".${subdir}/tmp"
> +	local destdir="${subdir}/tmp"

Looks good to me, but…

>  	echo "Running ${file_name} in chroot ${chroot_path}"
> -	${clst_CHROOT} ${chroot_path} ${destdir}/${file_name} || exit 1
> +	${clst_CHROOT} ${chroot_path} .${destdir}/${file_name} || exit 1

…do we really need this dot?  I'll test and find out ;).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy