[gentoo-catalyst] Start bringing consistent format to paths

[gentoo-catalyst] Start bringing consistent format to paths

[Brian Dolbec]

This patch removes trailing slashes from paths since most paths
added to them already have a leading slash.  It also includes a 
relative path fix needed as a result.  The result is less multiple 
slahes in a row in the paths.

[gentoo-catalyst] [PATCH] Remove some troublesome trailing slashes from paths

[Brian Dolbec]

Change the docstring to warn to use a proper path join function.
Fix a relative path bug after removing the above slash
by moving the "."
---
 modules/generic_stage_target.py | 28 +++++++++++++++-------------
 modules/livecd_stage2_target.py |  4 +++-
 modules/stage2_target.py        |  8 ++++++--
 targets/support/functions.sh    |  4 ++--
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3d66231..a5b52b0 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -331,8 +331,9 @@ class generic_stage_target(generic_target):
 				"/kerncache/"+self.settings["target_subpath"]+"/")
 
 	def set_target_path(self):
-		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")
 		if "AUTORESUME" in self.settings\
 			and os.path.exists(self.settings["autoresume_path"]+\
 				"setup_target_path"):
@@ -417,8 +418,9 @@ class generic_stage_target(generic_target):
 			self.settings["source_path"]=normpath(self.settings["storedir"]+\
 				"/tmp/"+self.settings["source_subpath"]+"/")
 		else:
-			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")
 			if os.path.isfile(self.settings["source_path"]):
 				# XXX: Is this even necessary if the previous check passes?
 				if os.path.exists(self.settings["source_path"]):
@@ -431,8 +433,8 @@ class generic_stage_target(generic_target):
 			print "\tIf this is not desired, remove this directory or turn off"
 			print "\tseedcache in the options of catalyst.conf the source path"
 			print "\twill then be "+\
-				normpath(self.settings["storedir"]+"/builds/"+\
-				self.settings["source_subpath"]+".tar.bz2\n")
+				normpath(self.settings["storedir"] + "/builds/" +
+					self.settings["source_subpath"].rstrip("/") + ".tar.bz2\n")
 
 	def set_dest_path(self):
 		if "root_path" in self.settings:
@@ -446,9 +448,9 @@ class generic_stage_target(generic_target):
 			"/root/*", self.settings["portdir"]]
 
 	def set_snapshot_path(self):
-		self.settings["snapshot_path"]=normpath(self.settings["storedir"]+\
+		self.settings["snapshot_path"] = normpath(self.settings["storedir"] +
 			"/snapshots/" + self.settings["snapshot_name"] +
-			self.settings["snapshot"] + ".tar.xz")
+			self.settings["snapshot"].rstrip("/") + ".tar.xz")
 
 		if os.path.exists(self.settings["snapshot_path"]):
 			self.settings["snapshot_path_hash"]=\
@@ -457,7 +459,7 @@ class generic_stage_target(generic_target):
 		else:
 			self.settings["snapshot_path"]=normpath(self.settings["storedir"]+\
 				"/snapshots/" + self.settings["snapshot_name"] +
-				self.settings["snapshot"] + ".tar.bz2")
+				self.settings["snapshot"].rstrip("/") + ".tar.bz2")
 
 			if os.path.exists(self.settings["snapshot_path"]):
 				self.settings["snapshot_path_hash"]=\
@@ -468,18 +470,18 @@ class generic_stage_target(generic_target):
 		if "SNAPCACHE" in self.settings:
 			self.settings["snapshot_cache_path"]=\
 				normpath(self.settings["snapshot_cache"]+"/"+\
-				self.settings["snapshot"]+"/")
+				self.settings["snapshot"])
 			self.snapcache_lock=\
 				catalyst_lock.LockDir(self.settings["snapshot_cache_path"])
 			print "Caching snapshot to "+self.settings["snapshot_cache_path"]
 
 	def set_chroot_path(self):
 		"""
-		NOTE: the trailing slash is very important!
-		Things *will* break without it!
+		NOTE: the trailing slash has been removed
+		Things *could* break if you don't use a proper join()
 		"""
 		self.settings["chroot_path"]=normpath(self.settings["storedir"]+\
-			"/tmp/"+self.settings["target_subpath"]+"/")
+			"/tmp/"+self.settings["target_subpath"])
 		self.chroot_lock=catalyst_lock.LockDir(self.settings["chroot_path"])
 
 	def set_autoresume_path(self):
diff --git a/modules/livecd_stage2_target.py b/modules/livecd_stage2_target.py
index 5be8fd2..c74c16d 100644
--- a/modules/livecd_stage2_target.py
+++ b/modules/livecd_stage2_target.py
@@ -33,7 +33,9 @@ class livecd_stage2_target(generic_stage_target):
 		file_locate(self.settings, ["cdtar","controller_file"])
 
 	def set_source_path(self):
-		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")
 		if os.path.isfile(self.settings["source_path"]):
 			self.settings["source_path_hash"]=generate_hash(self.settings["source_path"])
 		else:
diff --git a/modules/stage2_target.py b/modules/stage2_target.py
index 6083e2b..803ec59 100644
--- a/modules/stage2_target.py
+++ b/modules/stage2_target.py
@@ -19,7 +19,9 @@ class stage2_target(generic_stage_target):
 		if "SEEDCACHE" in self.settings and os.path.isdir(normpath(self.settings["storedir"]+"/tmp/"+self.settings["source_subpath"]+"/tmp/stage1root/")):
 			self.settings["source_path"]=normpath(self.settings["storedir"]+"/tmp/"+self.settings["source_subpath"]+"/tmp/stage1root/")
 		else:
-			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")
 			if os.path.isfile(self.settings["source_path"]):
 				if os.path.exists(self.settings["source_path"]):
 				# XXX: Is this even necessary if the previous check passes?
@@ -28,7 +30,9 @@ class stage2_target(generic_stage_target):
 		print "Source path set to "+self.settings["source_path"]
 		if os.path.isdir(self.settings["source_path"]):
 			print "\tIf this is not desired, remove this directory or turn of seedcache in the options of catalyst.conf"
-			print "\tthe source path will then be "+normpath(self.settings["storedir"]+"/builds/"+self.settings["source_subpath"]+".tar.bz2\n")
+			print "\tthe source path will then be " + \
+				normpath(self.settings["storedir"] + "/builds/" + \
+				self.settings["source_subpath"].restrip("/") + ".tar.bz2\n")
 
 	# XXX: How do these override_foo() functions differ from the ones in
 	# generic_stage_target and why aren't they in stage3_target?
diff --git a/targets/support/functions.sh b/targets/support/functions.sh
index 80e371c..3245862 100755
--- a/targets/support/functions.sh
+++ b/targets/support/functions.sh
@@ -20,7 +20,7 @@ exec_in_chroot(){
 # and executes it.
 	local file_name=$(basename ${1})
 	local subdir=${2}
-	local destdir=".${subdir}/tmp"
+	local destdir="${subdir}/tmp"
 
 	echo "Copying ${file_name} to ${destdir}"
 	copy_to_chroot ${1} ${destdir}
@@ -33,7 +33,7 @@ exec_in_chroot(){
 	chmod +x ${chroot_path}/${destdir}/${file_name}
 
 	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
 
 	delete_from_chroot ${destdir}/${file_name}
 	delete_from_chroot ${destdir}/chroot-functions.sh
-- 
1.8.3.2

[gentoo-catalyst] Re: [PATCH] Remove some troublesome trailing slashes from paths

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 3388 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]