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
signature.asc
Description: OpenPGP digital signature
