| 1 |
On Sat, Jan 11, 2014 at 11:43:07AM -0800, Brian Dolbec wrote: |
| 2 |
> - self.settings["target_path"]=normpath(self.settings["storedir"]+\ |
| 3 |
> - "/builds/"+self.settings["target_subpath"]+".tar.bz2") |
| 4 |
> + self.settings["target_path"] = normpath(self.settings["storedir"] + |
| 5 |
> + "/builds/" + self.settings["target_subpath"].rstrip('/') + |
| 6 |
> + ".tar.bz2") |
| 7 |
|
| 8 |
I find it hard to imagine target_subpath ending with a slash. If it |
| 9 |
did, I think that would be a bug where it's set, not something we |
| 10 |
should try to work around here. In the current master (e5a9e20): |
| 11 |
|
| 12 |
$ git grep -A2 'target_subpath.*=' | cat |
| 13 |
modules/generic_stage_target.py: self.settings["target_subpath"]=self.settings["rel_type"]+"/"+\ |
| 14 |
modules/generic_stage_target.py- self.settings["target"]+"-"+self.settings["subarch"]+"-"+\ |
| 15 |
modules/generic_stage_target.py- self.settings["version_stamp"] |
| 16 |
-- |
| 17 |
modules/snapshot_target.py: self.settings["target_subpath"]="portage" |
| 18 |
modules/snapshot_target.py- st=self.settings["storedir"] |
| 19 |
modules/snapshot_target.py- self.settings["snapshot_path"] = normpath(st + "/snapshots/" |
| 20 |
|
| 21 |
so we should be safe without this change. |
| 22 |
|
| 23 |
> - self.settings["source_path"]=normpath(self.settings["storedir"]+\ |
| 24 |
> - "/builds/"+self.settings["source_subpath"]+".tar.bz2") |
| 25 |
> + self.settings["source_path"] = normpath(self.settings["storedir"] + |
| 26 |
> + "/builds/" + self.settings["source_subpath"].rstrip("/") + |
| 27 |
> + ".tar.bz2") |
| 28 |
|
| 29 |
source_subpath comes from the spec file, but I think we should check |
| 30 |
for trailing slashes (and error out if we find them) when we load the |
| 31 |
spec file, not here. This also holds for the later source_subpath and |
| 32 |
snapshot stripping. |
| 33 |
|
| 34 |
> self.settings["snapshot_cache_path"]=\ |
| 35 |
> normpath(self.settings["snapshot_cache"]+"/"+\ |
| 36 |
> - self.settings["snapshot"]+"/") |
| 37 |
> + self.settings["snapshot"]) |
| 38 |
|
| 39 |
Looks good to me, but doesn't this need a corresponding update to: |
| 40 |
|
| 41 |
if "SNAPCACHE" in self.settings: |
| 42 |
snapshot_cache_hash=\ |
| 43 |
read_from_clst(self.settings["snapshot_cache_path"]+\ |
| 44 |
"catalyst-hash") |
| 45 |
|
| 46 |
and also to: |
| 47 |
|
| 48 |
if "SNAPCACHE" in self.settings: |
| 49 |
myf=open(self.settings["snapshot_cache_path"]+"catalyst-hash","w") |
| 50 |
myf.write(self.settings["snapshot_path_hash"]) |
| 51 |
myf.close() |
| 52 |
|
| 53 |
These will still *work* without a properly joined path, but they won't |
| 54 |
use their intended …/catalyst-hash file (and may not be cleaned up |
| 55 |
correctly). |
| 56 |
|
| 57 |
> self.settings["chroot_path"]=normpath(self.settings["storedir"]+\ |
| 58 |
> - "/tmp/"+self.settings["target_subpath"]+"/") |
| 59 |
> + "/tmp/"+self.settings["target_subpath"]) |
| 60 |
|
| 61 |
Looks good to me, but grepping for chroot_path gives lots of hits, and |
| 62 |
I didn't check them all. |
| 63 |
|
| 64 |
> - local destdir=".${subdir}/tmp" |
| 65 |
> + local destdir="${subdir}/tmp" |
| 66 |
|
| 67 |
Looks good to me, but… |
| 68 |
|
| 69 |
> echo "Running ${file_name} in chroot ${chroot_path}" |
| 70 |
> - ${clst_CHROOT} ${chroot_path} ${destdir}/${file_name} || exit 1 |
| 71 |
> + ${clst_CHROOT} ${chroot_path} .${destdir}/${file_name} || exit 1 |
| 72 |
|
| 73 |
…do we really need this dot? I'll test and find out ;). |
| 74 |
|
| 75 |
Cheers, |
| 76 |
Trevor |
| 77 |
|
| 78 |
-- |
| 79 |
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
| 80 |
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy |
Archives