[gentoo-catalyst] Patches to fix rebased patches already applied to current master

[gentoo-catalyst] Patches to fix rebased patches already applied to current master

[Brian Dolbec]

The following patches complete the mounts and mountmap changes already applied.
Some of them fix rebase errors introduced in the split, 
others were commits that were later in my rewrite.

After this set of patches, all test runs of catalyst up to and including stage3
process without error.

[gentoo-catalyst] [PATCH 1/6] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[Brian Dolbec]

If snapcache is enabled, self.mounts and self.mountmap had not been modified
like they were if it was not enabled.
---
 modules/generic_stage_target.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3e60f72..d7cffa9 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,10 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
-			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
-				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
-				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
+			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
+				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
+				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 1/6] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:05PM -0800, Brian Dolbec wrote:
> Subject: Re: [gentoo-catalyst] [PATCH 1/6] modules/generic_stage_target.py:
>   USE portdir, distdir,... instead of paths for keys

I'd prefer “: Use” instead of “:  USE”.

> -			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
> -			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
> -				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
> -				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
> +			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> +			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> +				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> +				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}

No need for the backslash after “"/portage",”.  There should also
probably be a space before “"port_tmpdir"”.  Style issues asside, the
content looks good to me.

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 --]

[gentoo-catalyst] [PATCH 2/6] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
---
 modules/generic_stage_target.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index d7cffa9..f881ca0 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -176,7 +176,8 @@ class generic_stage_target(generic_target):
 		if "SNAPCACHE" in self.settings:
 			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
 			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
+				"portdir": normpath(self.settings["snapshot_cache_path"] + "/" +
+					self.settings["repo_name"]),
 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 2/6] Fix a missed self.settings["repo_name"] migration

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:06PM -0800, Brian Dolbec wrote:
> -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> +				"portdir": normpath(self.settings["snapshot_cache_path"] + "/" +
> +					self.settings["repo_name"]),
>  				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}

Ah, this is getting rid of that trailing backslash, so I'm fine if it
stays in the previous commit.  We're still missing the space before
“"port_tmpdir"”.  The use of normpath and the repo_name setting look
good.

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 --]

Re: [gentoo-catalyst] [PATCH 2/6] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

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

On Sat, 2013-12-28 at 18:19 -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 05:57:06PM -0800, Brian Dolbec wrote:
> > -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > +				"portdir": normpath(self.settings["snapshot_cache_path"] + "/" +
> > +					self.settings["repo_name"]),
> >  				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> 
> Ah, this is getting rid of that trailing backslash, so I'm fine if it
> stays in the previous commit.  We're still missing the space before
> “"port_tmpdir"”.  The use of normpath and the repo_name setting look
> good.
> 
> Cheers,
> Trevor
> 

That too is not part of what this commit is changing.  That line is a
context line.   This commit is touching the previous line. This commit
is also not a whitespace cleanup.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] [PATCH 3/6] Add "local_overlay" to configdefaults.

[Brian Dolbec]

Fixes commit 463d98fe94041313e0cb4497081858ab5d9121f3
---
 catalyst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/catalyst b/catalyst
index 4ea4248..82b0942 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
 		"portdir": "/usr/portage",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 3/6] Add "local_overlay" to configdefaults.

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:07PM -0800, Brian Dolbec wrote:
> Fixes commit 463d98fe94041313e0cb4497081858ab5d9121f3

In the Git history (git://github.com/gitster/git.git) the convention
seems to be:

  ${SHORT_HASH} (${SUBJECT}, ${AUTHOR_DATE})

so:

  Fixes commit 463d98f (modules/generic_stage_target.py: Use a
  'local_overlay' setting instead of hard-coding '/usr/local/portage',
  2012-12-19).

The benefits of including human-readable content are:

* Folks don't have to look up the hash to get an idea about the
  referenced commit's changes.
* If this commit is cherry-picked into an alternate history, the
  subject and author-date will be preserved, but the hash changes.

>  	confdefaults = {
>  		"distdir": "/usr/portage/distfiles",
>  		"hash_function": "crc32",
> +		"local_overlay": "/usr/local/portage",

Oops :p.  Looks good to me.

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 --]

[gentoo-catalyst] [PATCH 4/6] Fix a missed self.mounts key change from a path as key.

[Brian Dolbec]

original patch: 27b751ffd9594e0128a3d70f83c7af18dab03838
---
 modules/generic_stage_target.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index f881ca0..ce28ab8 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -905,7 +905,8 @@ class generic_stage_target(generic_target):
 					os.makedirs(self.mountmap[x],0755)
 
 			src=self.mountmap[x]
-			if "SNAPCACHE" in self.settings and x == "/usr/portage":
+			#print "bind(); src =", src
+			if "SNAPCACHE" in self.settings and x == "portdir":
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 4/6] Fix a missed self.mounts key change from a path as key.

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:08PM -0800, Brian Dolbec wrote:
> original patch: 27b751ffd9594e0128a3d70f83c7af18dab03838

Actually, I don't think this is related to 27b751f
(modules/generic_stage_target.py: Use 'portdir' instead of hard-coding
'/usr/portage', 2012-12-19), which was only about
self.settings["portdir"].

> ---
>  			src=self.mountmap[x]
> -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> +			#print "bind(); src =", src
> +			if "SNAPCACHE" in self.settings and x == "portdir":
>  				self.snapshot_lock_object.read_lock()

Maybe squash this into patch #1 (modules/generic_stage_target.py: USE
portdir, distdir,... instead of paths for keys, 2013-12-15), because
it's fixing the currently-broken SNAPCACHE mountmap keys.

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 --]

[gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

Cleans up all self.mounts, self.mountmap usage.
Replace multiple path additions with one instance at the beginning of the function, reuse the result multiple times.
Add some extra debug prints (to be converted to logging later)
---
 modules/generic_stage_target.py | 94 ++++++++++++++++++++++++++---------------
 modules/stage1_target.py        |  5 ++-
 2 files changed, 63 insertions(+), 36 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index ce28ab8..c8d31e1 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -4,6 +4,23 @@ from generic_target import *
 from stat import *
 import catalyst_lock
 
+# temporary location.  It will be moved to a
+# new defaults file in a later comit
+TARGET_MOUNTS_DEFAULTS = {
+	"proc": "/proc",
+	"dev": "/dev",
+	"devpts": "/dev/pts",
+	"portdir": "/usr/portage",
+	"distdir": "/usr/portage/distfiles",
+	"packagedir": "/usr/portage/packages",
+	"port_tmpdir": "/var/tmp/portage",
+	"kerncache": "/tmp/kerncache",
+	"ccache": "/var/tmp/ccache",
+	"icecream": "/usr/lib/icecc/bin",
+	"port_logdir": "/var/log/portage",
+	}
+
+
 class generic_stage_target(generic_target):
 	"""
 	This class does all of the chroot setup, copying of files, etc. It is
@@ -173,6 +190,10 @@ class generic_stage_target(generic_target):
 			file_locate(self.settings,["portage_confdir"],expand=0)
 
 		""" Setup our mount points """
+		# initialize our target mounts.
+		# later I plan to make these configurable so the
+		# defaults can be easily overridden if desired.
+		self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()
 		if "SNAPCACHE" in self.settings:
 			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
 			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
@@ -219,17 +240,18 @@ class generic_stage_target(generic_target):
 			self.mounts.append("ccache")
 			self.mountmap["ccache"] = ccdir
 			""" for the chroot: """
-			self.env["CCACHE_DIR"]="/var/tmp/ccache"
+			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
 			self.mounts.append("/var/cache/icecream")
 			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
-			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
+			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
+				self.env["PATH"]
 
 		if "port_logdir" in self.settings:
-			self.mounts.append("/var/log/portage")
-			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
-			self.env["PORT_LOGDIR"]="/var/log/portage"
+			self.mounts.append("port_logdir")
+			self.mountmap["port_logdir"]=self.settings["port_logdir"]
+			self.env["PORT_LOGDIR"]=self.target_mounts["port_logdir"]
 			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
 
 	def override_cbuild(self):
@@ -603,33 +625,34 @@ class generic_stage_target(generic_target):
 				"kill-chroot-pids script failed.",env=self.env)
 
 	def mount_safety_check(self):
-		mypath=self.settings["chroot_path"]
-
 		"""
 		Check and verify that none of our paths in mypath are mounted. We don't
 		want to clean up with things still mounted, and this allows us to check.
 		Returns 1 on ok, 0 on "something is still mounted" case.
 		"""
 
-		if not os.path.exists(mypath):
+		if not os.path.exists(self.settings["chroot_path"]):
 			return
 
+		print "self.mounts =", self.mounts
 		for x in self.mounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			print "mount_safety_check() x =", x, target
+			if not os.path.exists(target):
 				continue
 
-			if ismount(mypath + self.mountmap[x]):
+			if ismount(target):
 				""" Something is still mounted "" """
 				try:
-					print self.mountmap[x] + " is still mounted; performing auto-bind-umount...",
+					print target + " is still mounted; performing auto-bind-umount...",
 					""" Try to umount stuff ourselves """
 					self.unbind()
-					if ismount(mypath + self.mountmap[x]):
-						raise CatalystError, "Auto-unbind failed for " + self.mountmap[x]
+					if ismount(target):
+						raise CatalystError, "Auto-unbind failed for " + target
 					else:
 						print "Auto-unbind successful..."
 				except CatalystError:
-					raise CatalystError, "Unable to auto-unbind " + self.mountmap[x]
+					raise CatalystError, "Unable to auto-unbind " + target
 
 	def unpack(self):
 		unpack=True
@@ -897,12 +920,14 @@ class generic_stage_target(generic_target):
 
 	def bind(self):
 		for x in self.mounts:
-			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
-				os.makedirs(self.settings["chroot_path"]+x,0755)
+			#print "bind(); x =", x
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			if not os.path.exists(target):
+				os.makedirs(target, 0755)
 
 			if not os.path.exists(self.mountmap[x]):
 				if not self.mountmap[x] == "tmpfs":
-					os.makedirs(self.mountmap[x],0755)
+					os.makedirs(self.mountmap[x], 0755)
 
 			src=self.mountmap[x]
 			#print "bind(); src =", src
@@ -910,20 +935,22 @@ class generic_stage_target(generic_target):
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-					retval = os.system("mount -t devfs none " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount -t devfs none " + target
+					retval=os.system(cmd)
 				else:
-					retval = os.system("mount_nullfs " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount_nullfs " + src + " " + target
+					retval=os.system(cmd)
 			else:
 				if src == "tmpfs":
 					if "var_tmpfs_portage" in self.settings:
-						retval=os.system("mount -t tmpfs -o size="+\
-							self.settings["var_tmpfs_portage"]+"G "+src+" "+\
-							self.settings["chroot_path"]+x)
+						cmd = "mount -t tmpfs -o size=" + \
+							self.settings["var_tmpfs_portage"] + "G " + \
+							src + " " + target
+						retval=os.system(cmd)
 				else:
-					retval = os.system("mount --bind " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount --bind " + src + " " + target
+					#print "bind(); cmd =", cmd
+					retval=os.system(cmd)
 			if retval!=0:
 				self.unbind()
 				raise CatalystError,"Couldn't bind mount " + src
@@ -935,26 +962,25 @@ class generic_stage_target(generic_target):
 		myrevmounts.reverse()
 		""" Unmount in reverse order for nested bind-mounts """
 		for x in myrevmounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(mypath + self.target_mounts[x])
+			if not os.path.exists(target):
 				continue
 
-			if not ismount(mypath + self.mountmap[x]):
+			if not ismount(target):
 				continue
 
-			retval=os.system("umount "+\
-				os.path.join(mypath, self.mountmap[x].lstrip(os.path.sep)))
+			retval=os.system("umount " + target)
 
 			if retval!=0:
-				warn("First attempt to unmount: " + mypath +
-					self.mountmap[x] +" failed.")
+				warn("First attempt to unmount: " + target + " failed.")
 				warn("Killing any pids still running in the chroot")
 
 				self.kill_chroot_pids()
 
-				retval2 = os.system("umount " + mypath + self.mountmap[x])
+				retval2 = os.system("umount " + target)
 				if retval2!=0:
 					ouch=1
-					warn("Couldn't umount bind mount: " + mypath + self.mountmap[x])
+					warn("Couldn't umount bind mount: " + target)
 
 			if "SNAPCACHE" in self.settings and x == "/usr/portage":
 				try:
diff --git a/modules/stage1_target.py b/modules/stage1_target.py
index aa43926..1e0d874 100644
--- a/modules/stage1_target.py
+++ b/modules/stage1_target.py
@@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
 			os.makedirs(self.settings["stage_path"]+"/proc")
 
 		# alter the mount mappings to bind mount proc onto it
-		self.mounts.append("/tmp/stage1root/proc")
-		self.mountmap["/tmp/stage1root/proc"]="/proc"
+		self.mounts.append("stage1root/proc")
+		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
+		self.mountmap["stage1root/proc"]="/proc"
 
 def register(foo):
 	foo.update({"stage1":stage1_target})
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
> +# temporary location.  It will be moved to a
> +# new defaults file in a later comit

Hopefully those later commits will land, but maybe they won't.  I'd
rather comment on future goals in the Git commit instead of in the
source code.

> +		# initialize our target mounts.
> +		# later I plan to make these configurable so the
> +		# defaults can be easily overridden if desired.
> +		self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()

See above about future goals in the Git commit.

> -			self.mounts.append("/var/log/portage")
> -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> -			self.env["PORT_LOGDIR"]="/var/log/portage"
> +			self.mounts.append("port_logdir")
> +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> +			self.env["PORT_LOGDIR"]=self.target_mounts["port_logdir"]

Looks good.  The mounts/mountmap changes are orthogonal to the
target_mounts addition, but it's probably not worth splitting them out
into a separate fixup.

>  	def mount_safety_check(self):
> …
>  	def bind(self):
> …
> @@ -935,26 +962,25 @@ class generic_stage_target(generic_target):
>  		myrevmounts.reverse()
>  		""" Unmount in reverse order for nested bind-mounts """
> …

Hooray, sanity :).

> -		self.mounts.append("/tmp/stage1root/proc")
> -		self.mountmap["/tmp/stage1root/proc"]="/proc"
> +		self.mounts.append("stage1root/proc")
> +		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
> +		self.mountmap["stage1root/proc"]="/proc"

Hmm.  I'm not how this is handled at the moment, but don't we want:

  mount -t proc none /tmp/stage1root/proc

instead of:

  mount --bind /proc /tmp/stage1root/proc

The handbook is currently using --rbind as well [1].

Maybe food for a future series :p.

Cheers,
Trevor

[1]: http://www.gentoo.org/doc/en/handbook/handbook-x86.xml?part=1&chap=6#doc_chap1_sect3

-- 
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 --]

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
> > -		self.mounts.append("/tmp/stage1root/proc")
> > -		self.mountmap["/tmp/stage1root/proc"]="/proc"
> > +		self.mounts.append("stage1root/proc")
> > +		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
> > +		self.mountmap["stage1root/proc"]="/proc"
> 
> Hmm.  I'm not how this is handled at the moment, but don't we want:
> 
>   mount -t proc none /tmp/stage1root/proc
> 
> instead of:
> 
>   mount --bind /proc /tmp/stage1root/proc
> 
> The handbook is currently using --rbind as well [1].
> 
> Maybe food for a future series :p.

After Douglas Freed talked me off the devtmpfs ledge on
#gentoo-releng, my current suggested mount changes are:

  -t:
    proc
  --rbind:
    sys
    dev
    portdir    
    port_logdir
    port_tmpdir
    kerncache
    ccache
    icecream
  drop:
    devpts (now that we --rbind /dev)
    distdir (now that we --rbind portdir)
    packagedir (now that we --rbind portdir)

For distdir/packagedir, I'd be find if the source defaulted to None
(meaning “don't mount anything”) but could be configured to a path
(meaning “--rbind this after you've --rbind-mounted portdir”).

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 --]

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

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

On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
> > On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
> > > -		self.mounts.append("/tmp/stage1root/proc")
> > > -		self.mountmap["/tmp/stage1root/proc"]="/proc"
> > > +		self.mounts.append("stage1root/proc")
> > > +		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
> > > +		self.mountmap["stage1root/proc"]="/proc"
> > 
> > Hmm.  I'm not how this is handled at the moment, but don't we want:
> > 
> >   mount -t proc none /tmp/stage1root/proc
> > 
> > instead of:
> > 
> >   mount --bind /proc /tmp/stage1root/proc
> > 
> > The handbook is currently using --rbind as well [1].
> > 
> > Maybe food for a future series :p.
> 
> After Douglas Freed talked me off the devtmpfs ledge on
> #gentoo-releng, my current suggested mount changes are:
> 
>   -t:
>     proc
>   --rbind:
>     sys
>     dev
>     portdir    
>     port_logdir
>     port_tmpdir
>     kerncache
>     ccache
>     icecream
>   drop:
>     devpts (now that we --rbind /dev)
>     distdir (now that we --rbind portdir)
>     packagedir (now that we --rbind portdir)
> 
> For distdir/packagedir, I'd be find if the source defaulted to None
> (meaning “don't mount anything”) but could be configured to a path
> (meaning “--rbind this after you've --rbind-mounted portdir”).
> 
> Cheers,
> Trevor
> 

This series of patches was to just fix the migration from hard coded
paths to centrally configurable ones.  These mount changes should be
commits unto themselves.  Please don't confuse the issues... we have
lots more changes to get through.   I'm not saying anything against your
proposed changes, just they're not for this commit.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] mounts changes was: [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

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

On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
> > On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
> > > -		self.mounts.append("/tmp/stage1root/proc")
> > > -		self.mountmap["/tmp/stage1root/proc"]="/proc"
> > > +		self.mounts.append("stage1root/proc")
> > > +		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
> > > +		self.mountmap["stage1root/proc"]="/proc"
> > 
> > Hmm.  I'm not how this is handled at the moment, but don't we want:
> > 
> >   mount -t proc none /tmp/stage1root/proc
> > 
> > instead of:
> > 
> >   mount --bind /proc /tmp/stage1root/proc
> > 
> > The handbook is currently using --rbind as well [1].
> > 
> > Maybe food for a future series :p.
> 
> After Douglas Freed talked me off the devtmpfs ledge on
> #gentoo-releng, my current suggested mount changes are:
> 
>   -t:
>     proc
>   --rbind:
>     sys
>     dev
>     portdir    
>     port_logdir
>     port_tmpdir
>     kerncache
>     ccache
>     icecream
>   drop:
>     devpts (now that we --rbind /dev)
>     distdir (now that we --rbind portdir)
>     packagedir (now that we --rbind portdir)
> 
> For distdir/packagedir, I'd be find if the source defaulted to None
> (meaning “don't mount anything”) but could be configured to a path
> (meaning “--rbind this after you've --rbind-mounted portdir”).
> 
> Cheers,
> Trevor
> 

No, we can't drop distdir and packagedir.  The default locations for
those are waiting for the catalyst changes to be completed.  That is the
reason I got started rewriting catalyst a year ago.  Those 2 directories
are being relocated OUT of the tree.  Like they should have been all
along.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] Re: mounts changes

[W. Trevor King]

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

On Mon, Dec 30, 2013 at 06:54:40PM -0800, Brian Dolbec wrote:
> On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
> > After Douglas Freed talked me off the devtmpfs ledge on
> > #gentoo-releng, my current suggested mount changes are:
> > 
> >   -t:
> >     proc
> >   --rbind:
> >     sys
> >     dev
> >     portdir    
> >     port_logdir
> >     port_tmpdir
> >     kerncache
> >     ccache
> >     icecream
> >   drop:
> >     devpts (now that we --rbind /dev)
> >     distdir (now that we --rbind portdir)
> >     packagedir (now that we --rbind portdir)
> > 
> > For distdir/packagedir, I'd be find if the source defaulted to None
> > (meaning “don't mount anything”) but could be configured to a path
> > (meaning “--rbind this after you've --rbind-mounted portdir”).
> 
> No, we can't drop distdir and packagedir.  The default locations for
> those are waiting for the catalyst changes to be completed.  That is
> the reason I got started rewriting catalyst a year ago.  Those 2
> directories are being relocated OUT of the tree.  Like they should
> have been all along.

If they're moving out of portdir, it makes sense to mount them by
default.  I'm fine with that.  And I'm looking forward to ConfigParser
spec files with sections like:

  [mounts]
  icecream = no
  ubuntu_shm = yes

  [mount source]
  portdir = /home/wking/src/portage
  port_logdir = /home/wking/src/portage-logs
  ubuntu_shm = /run/shm

  [mount targets]
  ubuntu_shm = /run/shm

And to chaining spec files so that I can share basic stuff like this
between all my stages…  Hopefully soon :).

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 --]

Re: [gentoo-catalyst] mounts changes was: [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Jorge Manuel B. S. Vicetto]

On Mon, 30 Dec 2013, Brian Dolbec wrote:

> On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
>> On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
>>> On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
>>>> -		self.mounts.append("/tmp/stage1root/proc")
>>>> -		self.mountmap["/tmp/stage1root/proc"]="/proc"
>>>> +		self.mounts.append("stage1root/proc")
>>>> +		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
>>>> +		self.mountmap["stage1root/proc"]="/proc"
>>>
>>> Hmm.  I'm not how this is handled at the moment, but don't we want:
>>>
>>>   mount -t proc none /tmp/stage1root/proc
>>>
>>> instead of:
>>>
>>>   mount --bind /proc /tmp/stage1root/proc
>>>
>>> The handbook is currently using --rbind as well [1].
>>>
>>> Maybe food for a future series :p.
>>
>> After Douglas Freed talked me off the devtmpfs ledge on
>> #gentoo-releng, my current suggested mount changes are:
>>
>>   -t:
>>     proc
>>   --rbind:
>>     sys
>>     dev
>>     portdir
>>     port_logdir
>>     port_tmpdir
>>     kerncache
>>     ccache
>>     icecream
>>   drop:
>>     devpts (now that we --rbind /dev)
>>     distdir (now that we --rbind portdir)
>>     packagedir (now that we --rbind portdir)
>>
>> For distdir/packagedir, I'd be find if the source defaulted to None
>> (meaning “don't mount anything”) but could be configured to a path
>> (meaning “--rbind this after you've --rbind-mounted portdir”).
>>
>> Cheers,
>> Trevor
>>
>
> No, we can't drop distdir and packagedir.  The default locations for
> those are waiting for the catalyst changes to be completed.  That is the
> reason I got started rewriting catalyst a year ago.  Those 2 directories
> are being relocated OUT of the tree.  Like they should have been all
> along.

As I've already mentioned on #gentoo-releng, there are also 2 reasons this 
is a no-go for me:

1. it's unlikely we want to share pkgdir between the host and the targets 
we build

2. sharing pkgdir between incompatible targets would fail miserably. For 
example, skimmer does amd64 builds for hardened and non-hardened, multilib 
and no-multilib. It also does x86 builds. Sharing a package between any or 
at least most of those targets would fail and cause broken stages.

Regards,
Jorge

Re: [gentoo-catalyst] mounts changes was: [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Wed, Jan 01, 2014 at 06:05:35AM +0000, Jorge Manuel B. S. Vicetto wrote:
> On Mon, 30 Dec 2013, Brian Dolbec wrote:
> > On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
> >> On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
> >>> Hmm.  I'm not how this is handled at the moment, but don't we want:
> >>>
> >>>   mount -t proc none /tmp/stage1root/proc
> >>>
> >>> instead of:
> >>>
> >>>   mount --bind /proc /tmp/stage1root/proc
> >>>
> >>> The handbook is currently using --rbind as well [1].
> >>>
> >>> Maybe food for a future series :p.
> >>
> >> After Douglas Freed talked me off the devtmpfs ledge on
> >> #gentoo-releng, my current suggested mount changes are:
> >>
> >>   -t:
> >>     proc
> >>   --rbind:
> >>     sys
> >>     dev
> >>     portdir
> >>     port_logdir
> >>     port_tmpdir
> >>     kerncache
> >>     ccache
> >>     icecream
> >>   drop:
> >>     devpts (now that we --rbind /dev)
> >>     distdir (now that we --rbind portdir)
> >>     packagedir (now that we --rbind portdir)
> >>
> >> For distdir/packagedir, I'd be find if the source defaulted to None
> >> (meaning “don't mount anything”) but could be configured to a path
> >> (meaning “--rbind this after you've --rbind-mounted portdir”).
> >
> > No, we can't drop distdir and packagedir.  The default locations for
> > those are waiting for the catalyst changes to be completed.  That is the
> > reason I got started rewriting catalyst a year ago.  Those 2 directories
> > are being relocated OUT of the tree.  Like they should have been all
> > along.
> 
> As I've already mentioned on #gentoo-releng, there are also 2 reasons this 
> is a no-go for me:
> 
> 1. it's unlikely we want to share pkgdir between the host and the targets 
> we build
> 
> 2. sharing pkgdir between incompatible targets would fail miserably. For 
> example, skimmer does amd64 builds for hardened and non-hardened, multilib 
> and no-multilib. It also does x86 builds. Sharing a package between any or 
> at least most of those targets would fail and cause broken stages.

So packagedir should just come from the pkgcache.  That makes sense.
I still think /dev should be mounted via --rbind, and likely other
user-configurable sources as well.  If we remove a user-configurable
packagedir source and keep the rest of this --rbind proposal, does
that address all of your concerns?

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 --]

Re: [gentoo-catalyst] mounts changes was: [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Jorge Manuel B. S. Vicetto]

On Wed, 1 Jan 2014, W. Trevor King wrote:

> On Wed, Jan 01, 2014 at 06:05:35AM +0000, Jorge Manuel B. S. Vicetto wrote:
>> On Mon, 30 Dec 2013, Brian Dolbec wrote:
>>> On Sat, 2013-12-28 at 19:52 -0800, W. Trevor King wrote:
>>>> On Sat, Dec 28, 2013 at 06:49:11PM -0800, W. Trevor King wrote:
>>>>> Hmm.  I'm not how this is handled at the moment, but don't we want:
>>>>>
>>>>>   mount -t proc none /tmp/stage1root/proc
>>>>>
>>>>> instead of:
>>>>>
>>>>>   mount --bind /proc /tmp/stage1root/proc
>>>>>
>>>>> The handbook is currently using --rbind as well [1].
>>>>>
>>>>> Maybe food for a future series :p.
>>>>
>>>> After Douglas Freed talked me off the devtmpfs ledge on
>>>> #gentoo-releng, my current suggested mount changes are:
>>>>
>>>>   -t:
>>>>     proc
>>>>   --rbind:
>>>>     sys
>>>>     dev
>>>>     portdir
>>>>     port_logdir
>>>>     port_tmpdir
>>>>     kerncache
>>>>     ccache
>>>>     icecream
>>>>   drop:
>>>>     devpts (now that we --rbind /dev)
>>>>     distdir (now that we --rbind portdir)
>>>>     packagedir (now that we --rbind portdir)
>>>>
>>>> For distdir/packagedir, I'd be find if the source defaulted to None
>>>> (meaning “don't mount anything”) but could be configured to a path
>>>> (meaning “--rbind this after you've --rbind-mounted portdir”).
>>>
>>> No, we can't drop distdir and packagedir.  The default locations for
>>> those are waiting for the catalyst changes to be completed.  That is the
>>> reason I got started rewriting catalyst a year ago.  Those 2 directories
>>> are being relocated OUT of the tree.  Like they should have been all
>>> along.
>>
>> As I've already mentioned on #gentoo-releng, there are also 2 reasons this
>> is a no-go for me:
>>
>> 1. it's unlikely we want to share pkgdir between the host and the targets
>> we build
>>
>> 2. sharing pkgdir between incompatible targets would fail miserably. For
>> example, skimmer does amd64 builds for hardened and non-hardened, multilib
>> and no-multilib. It also does x86 builds. Sharing a package between any or
>> at least most of those targets would fail and cause broken stages.
>
> So packagedir should just come from the pkgcache.  That makes sense.
> I still think /dev should be mounted via --rbind, and likely other
> user-configurable sources as well.  If we remove a user-configurable
> packagedir source and keep the rest of this --rbind proposal, does
> that address all of your concerns?

Sorry, I should have been clearer. I meant the proposal to use --rbind for 
portdir. The rest seems ok.

> Cheers,
> Trevor

Regards,
Jorge.

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

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

On Sat, 2013-12-28 at 18:49 -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 05:57:09PM -0800, Brian Dolbec wrote:
> > +# temporary location.  It will be moved to a
> > +# new defaults file in a later comit
> 
> Hopefully those later commits will land, but maybe they won't.  I'd
> rather comment on future goals in the Git commit instead of in the
> source code.

Fair enough...

> 
> > +		# initialize our target mounts.
> > +		# later I plan to make these configurable so the
> > +		# defaults can be easily overridden if desired.
> > +		self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()
> 
> See above about future goals in the Git commit.
> 
> > -			self.mounts.append("/var/log/portage")
> > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > +			self.mounts.append("port_logdir")
> > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > +			self.env["PORT_LOGDIR"]=self.target_mounts["port_logdir"]
> 
> Looks good.  The mounts/mountmap changes are orthogonal to the
> target_mounts addition, but it's probably not worth splitting them out
> into a separate fixup.


Yeah, I missed separating some of that.  Looks like I have to rebase the
hell out of these again anyway, so, maybe I will...


> Cheers,
> Trevor


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Alec Warner]

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

On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec <dolsen@gentoo.org> wrote:

> Cleans up all self.mounts, self.mountmap usage.
> Replace multiple path additions with one instance at the beginning of the
> function, reuse the result multiple times.
> Add some extra debug prints (to be converted to logging later)
> ---
>  modules/generic_stage_target.py | 94
> ++++++++++++++++++++++++++---------------
>  modules/stage1_target.py        |  5 ++-
>  2 files changed, 63 insertions(+), 36 deletions(-)
>
> diff --git a/modules/generic_stage_target.py
> b/modules/generic_stage_target.py
> index ce28ab8..c8d31e1 100644
> --- a/modules/generic_stage_target.py
> +++ b/modules/generic_stage_target.py
> @@ -4,6 +4,23 @@ from generic_target import *
>  from stat import *
>  import catalyst_lock
>
> +# temporary location.  It will be moved to a
> +# new defaults file in a later comit
> +TARGET_MOUNTS_DEFAULTS = {
> +       "proc": "/proc",
> +       "dev": "/dev",
> +       "devpts": "/dev/pts",
> +       "portdir": "/usr/portage",
> +       "distdir": "/usr/portage/distfiles",
> +       "packagedir": "/usr/portage/packages",
> +       "port_tmpdir": "/var/tmp/portage",
> +       "kerncache": "/tmp/kerncache",
> +       "ccache": "/var/tmp/ccache",
> +       "icecream": "/usr/lib/icecc/bin",
> +       "port_logdir": "/var/log/portage",
> +       }
> +
> +
>  class generic_stage_target(generic_target):
>         """
>         This class does all of the chroot setup, copying of files, etc. It
> is
> @@ -173,6 +190,10 @@ class generic_stage_target(generic_target):
>
> file_locate(self.settings,["portage_confdir"],expand=0)
>
>                 """ Setup our mount points """
> +               # initialize our target mounts.
> +               # later I plan to make these configurable so the
> +               # defaults can be easily overridden if desired.
> +               self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()
>                 if "SNAPCACHE" in self.settings:
>                         self.mounts=["proc", "dev", "portdir", "distdir",
> "port_tmpdir"]
>                         self.mountmap={"proc":"/proc", "dev":"/dev",
> "devpts":"/dev/pts",
> @@ -219,17 +240,18 @@ class generic_stage_target(generic_target):
>                         self.mounts.append("ccache")
>                         self.mountmap["ccache"] = ccdir
>                         """ for the chroot: """
> -                       self.env["CCACHE_DIR"]="/var/tmp/ccache"
> +                       self.env["CCACHE_DIR"] =
> self.target_mounts["ccache"]
>
>                 if "ICECREAM" in self.settings:
>                         self.mounts.append("/var/cache/icecream")
>
> self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
> -
> self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
> +                       self.env["PATH"] = self.target_mounts["icecream"]
> + ":" + \
> +                               self.env["PATH"]
>
                if "port_logdir" in self.settings:
> -                       self.mounts.append("/var/log/portage")
> -
> self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> -                       self.env["PORT_LOGDIR"]="/var/log/portage"
> +                       self.mounts.append("port_logdir")
> +
> self.mountmap["port_logdir"]=self.settings["port_logdir"]
> +
> self.env["PORT_LOGDIR"]=self.target_mounts["port_logdir"]
>                         self.env["PORT_LOGDIR_CLEAN"]='find
> "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
>

I've avoided commenting on + in strings, as my understanding was that we
were not doing style fixups in these CLs. I would argue that in general,
any time you use string + string in python, you are doing it wrong (e.g.
your code is not 'pythonic')

You can use

"%s:%s" % (string1, string2) or ":".join(string1, string2)

Either are fine, and are nominally better than string + string + string.



>
>         def override_cbuild(self):
> @@ -603,33 +625,34 @@ class generic_stage_target(generic_target):
>                                 "kill-chroot-pids script
> failed.",env=self.env)
>
>         def mount_safety_check(self):
> -               mypath=self.settings["chroot_path"]
> -
>                 """
>                 Check and verify that none of our paths in mypath are
> mounted. We don't
>                 want to clean up with things still mounted, and this
> allows us to check.
>                 Returns 1 on ok, 0 on "something is still mounted" case.
>                 """
>
> -               if not os.path.exists(mypath):
> +               if not os.path.exists(self.settings["chroot_path"]):
>                         return
>
> +               print "self.mounts =", self.mounts
>                 for x in self.mounts:
> -                       if not os.path.exists(mypath + self.mountmap[x]):
> +                       target = normpath(self.settings["chroot_path"] +
> self.target_mounts[x])
> +                       print "mount_safety_check() x =", x, target
> +                       if not os.path.exists(target):
>                                 continue
>
> -                       if ismount(mypath + self.mountmap[x]):
> +                       if ismount(target):
>                                 """ Something is still mounted "" """
>                                 try:
> -                                       print self.mountmap[x] + " is
> still mounted; performing auto-bind-umount...",
> +                                       print target + " is still mounted;
> performing auto-bind-umount...",
>                                         """ Try to umount stuff ourselves
> """
>                                         self.unbind()
> -                                       if ismount(mypath +
> self.mountmap[x]):
> -                                               raise CatalystError,
> "Auto-unbind failed for " + self.mountmap[x]
> +                                       if ismount(target):
> +                                               raise CatalystError,
> "Auto-unbind failed for " + target
>                                         else:
>                                                 print "Auto-unbind
> successful..."
>                                 except CatalystError:
> -                                       raise CatalystError, "Unable to
> auto-unbind " + self.mountmap[x]
> +                                       raise CatalystError, "Unable to
> auto-unbind " + target
>
>         def unpack(self):
>                 unpack=True
> @@ -897,12 +920,14 @@ class generic_stage_target(generic_target):
>
>         def bind(self):
>                 for x in self.mounts:
> -                       if not os.path.exists(self.settings["chroot_path"]
> + self.mountmap[x]):
> -
> os.makedirs(self.settings["chroot_path"]+x,0755)
> +                       #print "bind(); x =", x
> +                       target = normpath(self.settings["chroot_path"] +
> self.target_mounts[x])
> +                       if not os.path.exists(target):
> +                               os.makedirs(target, 0755)
>
>                         if not os.path.exists(self.mountmap[x]):
>                                 if not self.mountmap[x] == "tmpfs":
> -                                       os.makedirs(self.mountmap[x],0755)
> +                                       os.makedirs(self.mountmap[x], 0755)
>
>                         src=self.mountmap[x]
>                         #print "bind(); src =", src
> @@ -910,20 +935,22 @@ class generic_stage_target(generic_target):
>                                 self.snapshot_lock_object.read_lock()
>                         if os.uname()[0] == "FreeBSD":
>                                 if src == "/dev":
> -                                       retval = os.system("mount -t devfs
> none " +
> -
> self.settings["chroot_path"] + src)
> +                                       cmd = "mount -t devfs none " +
> target
> +                                       retval=os.system(cmd)
>

"because the code use to do it" aside, is there some compelling reason to
not use subprocess.Popen here? os.system is probably the worst syscall you
can use. It spawns a shell, it will select things like './mount' over
'/usr/bin/mount' because you failed to specify the full path to the binary,
its not clear what environment the shell is using.

Even something like:

import subprocess
retval = subprocess.call(cmd.split())

would likely avoid these problems unless you are relying on shell expansion
in cmd?




>                                 else:
> -                                       retval = os.system("mount_nullfs "
> + src + " " +
> -
> self.settings["chroot_path"] + src)
> +                                       cmd = "mount_nullfs " + src + " "
> + target
> +                                       retval=os.system(cmd)
>                         else:
>                                 if src == "tmpfs":
>                                         if "var_tmpfs_portage" in
> self.settings:
> -                                               retval=os.system("mount -t
> tmpfs -o size="+\
> -
> self.settings["var_tmpfs_portage"]+"G "+src+" "+\
> -
> self.settings["chroot_path"]+x)
> +                                               cmd = "mount -t tmpfs -o
> size=" + \
> +
> self.settings["var_tmpfs_portage"] + "G " + \
> +                                                       src + " " + target
> +                                               retval=os.system(cmd)
>                                 else:
> -                                       retval = os.system("mount --bind "
> + src + " " +
> -
> self.settings["chroot_path"] + src)
> +                                       cmd = "mount --bind " + src + " "
> + target
> +                                       #print "bind(); cmd =", cmd
> +                                       retval=os.system(cmd)
>                         if retval!=0:
>                                 self.unbind()
>                                 raise CatalystError,"Couldn't bind mount "
> + src
> @@ -935,26 +962,25 @@ class generic_stage_target(generic_target):
>                 myrevmounts.reverse()
>                 """ Unmount in reverse order for nested bind-mounts """
>                 for x in myrevmounts:
> -                       if not os.path.exists(mypath + self.mountmap[x]):
> +                       target = normpath(mypath + self.target_mounts[x])
> +                       if not os.path.exists(target):
>                                 continue
>
> -                       if not ismount(mypath + self.mountmap[x]):
> +                       if not ismount(target):
>                                 continue
>
> -                       retval=os.system("umount "+\
> -                               os.path.join(mypath,
> self.mountmap[x].lstrip(os.path.sep)))
> +                       retval=os.system("umount " + target)
>
>                         if retval!=0:
> -                               warn("First attempt to unmount: " + mypath
> +
> -                                       self.mountmap[x] +" failed.")
> +                               warn("First attempt to unmount: " + target
> + " failed.")
>                                 warn("Killing any pids still running in
> the chroot")
>
>                                 self.kill_chroot_pids()
>
> -                               retval2 = os.system("umount " + mypath +
> self.mountmap[x])
> +                               retval2 = os.system("umount " + target)
>                                 if retval2!=0:
>                                         ouch=1
> -                                       warn("Couldn't umount bind mount:
> " + mypath + self.mountmap[x])
> +                                       warn("Couldn't umount bind mount:
> " + target)
>
>                         if "SNAPCACHE" in self.settings and x ==
> "/usr/portage":
>                                 try:
> diff --git a/modules/stage1_target.py b/modules/stage1_target.py
> index aa43926..1e0d874 100644
> --- a/modules/stage1_target.py
> +++ b/modules/stage1_target.py
> @@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
>                         os.makedirs(self.settings["stage_path"]+"/proc")
>
>                 # alter the mount mappings to bind mount proc onto it
> -               self.mounts.append("/tmp/stage1root/proc")
> -               self.mountmap["/tmp/stage1root/proc"]="/proc"
> +               self.mounts.append("stage1root/proc")
> +               self.target_mounts["stage1root/proc"] =
> "/tmp/stage1root/proc"
> +               self.mountmap["stage1root/proc"]="/proc"
>
>  def register(foo):
>         foo.update({"stage1":stage1_target})
> --
> 1.8.3.2
>
>
>

[-- Attachment #2: Type: text/html, Size: 18467 bytes --]

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

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

On Sat, 2013-12-28 at 21:26 -0800, Alec Warner wrote:
> On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec <dolsen@gentoo.org> 
> 
> 
> I've avoided commenting on + in strings, as my understanding was that
> we were not doing style fixups in these CLs. I would argue that in
> general, any time you use string + string in python, you are doing it
> wrong (e.g. your code is not 'pythonic')
> 
> 
> You can use
> 
> 
> "%s:%s" % (string1, string2) or ":".join(string1, string2)
> 
> 
> Either are fine, and are nominally better than string + string +
> string.
> 
> 

Actually string + string is faster than "%s%s" %(string1, string2)  for
simple string adition like this example.  However %s substitution
performs better for larger multiples which occur many, many times
throughout the code.  So, yes we should clean this up after the current
rewrite code is merged in one form or another.


>  
>         
>         
>         self.snapshot_lock_object.read_lock()
>                                 if os.uname()[0] == "FreeBSD":
>                                         if src == "/dev":
>         -                                       retval =
>         os.system("mount -t devfs none " +
>         -
>         self.settings["chroot_path"] + src)
>         +                                       cmd = "mount -t devfs
>         none " + target
>         +                                       retval=os.system(cmd)
> 
> 
> "because the code use to do it" aside, is there some compelling reason
> to not use subprocess.Popen here? os.system is probably the worst
> syscall you can use. It spawns a shell, it will select things like
> './mount' over '/usr/bin/mount' because you failed to specify the full
> path to the binary, its not clear what environment the shell is using.
> 
> 
> Even something like:
> 
> 
> import subprocess
> retval = subprocess.call(cmd.split())
> 
> 
> would likely avoid these problems unless you are relying on shell
> expansion in cmd?
> 
> 
> 

Yes, I have changed the primary chroot run from the old cloned portage
spawn()'s to using subprocess.Popen.  Coming to you in a future commit
waiting it's turn here.  I have not yet tackled these os.system() calls.

These changes just moved the string addition out of the function call as
a preparation for it's replacement.

There is no shortage of things that need fixing...
 There are lots more uses of os.system() below...

> 
>  
>                                         else:
>         -                                       retval =
>         os.system("mount_nullfs " + src + " " +
>         -
>         self.settings["chroot_path"] + src)
>         +                                       cmd = "mount_nullfs "
>         + src + " " + target
>         +                                       retval=os.system(cmd)
>                                 else:
>                                         if src == "tmpfs":
>                                                 if "var_tmpfs_portage"
>         in self.settings:
>         -
>         retval=os.system("mount -t tmpfs -o size="+\
>         -
>         self.settings["var_tmpfs_portage"]+"G "+src+" "+\
>         -
>         self.settings["chroot_path"]+x)
>         +                                               cmd = "mount
>         -t tmpfs -o size=" + \
>         +
>         self.settings["var_tmpfs_portage"] + "G " + \
>         +                                                       src +
>         " " + target
>         +
>         retval=os.system(cmd)
>                                         else:
>         -                                       retval =
>         os.system("mount --bind " + src + " " +
>         -
>         self.settings["chroot_path"] + src)
>         +                                       cmd = "mount --bind "
>         + src + " " + target
>         +                                       #print "bind(); cmd
>         =", cmd
>         +                                       retval=os.system(cmd)
>                                 if retval!=0:
>                                         self.unbind()
>                                         raise CatalystError,"Couldn't
>         bind mount " + src
>         @@ -935,26 +962,25 @@ class
>         generic_stage_target(generic_target):
>                         myrevmounts.reverse()
>                         """ Unmount in reverse order for nested
>         bind-mounts """
>                         for x in myrevmounts:
>         -                       if not os.path.exists(mypath +
>         self.mountmap[x]):
>         +                       target = normpath(mypath +
>         self.target_mounts[x])
>         +                       if not os.path.exists(target):
>                                         continue
>         
>         -                       if not ismount(mypath +
>         self.mountmap[x]):
>         +                       if not ismount(target):
>                                         continue
>         
>         -                       retval=os.system("umount "+\
>         -                               os.path.join(mypath,
>         self.mountmap[x].lstrip(os.path.sep)))
>         +                       retval=os.system("umount " + target)
>         
>                                 if retval!=0:
>         -                               warn("First attempt to
>         unmount: " + mypath +
>         -                                       self.mountmap[x] +"
>         failed.")
>         +                               warn("First attempt to
>         unmount: " + target + " failed.")
>                                         warn("Killing any pids still
>         running in the chroot")
>         
>                                         self.kill_chroot_pids()
>         
>         -                               retval2 = os.system("umount "
>         + mypath + self.mountmap[x])
>         +                               retval2 = os.system("umount "
>         + target)
>                                         if retval2!=0:
>                                                 ouch=1
>         -                                       warn("Couldn't umount
>         bind mount: " + mypath + self.mountmap[x])
>         +                                       warn("Couldn't umount
>         bind mount: " + target)
>         
>                                 if "SNAPCACHE" in self.settings and x
>         == "/usr/portage":
>                                         try:
>         diff --git a/modules/stage1_target.py
>         b/modules/stage1_target.py
>         index aa43926..1e0d874 100644
>         --- a/modules/stage1_target.py
>         +++ b/modules/stage1_target.py
>         @@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
>         
>         os.makedirs(self.settings["stage_path"]+"/proc")
>         
>                         # alter the mount mappings to bind mount proc
>         onto it
>         -               self.mounts.append("/tmp/stage1root/proc")
>         -               self.mountmap["/tmp/stage1root/proc"]="/proc"
>         +               self.mounts.append("stage1root/proc")
>         +               self.target_mounts["stage1root/proc"] =
>         "/tmp/stage1root/proc"
>         +               self.mountmap["stage1root/proc"]="/proc"
>         
>          def register(foo):
>                 foo.update({"stage1":stage1_target})
>         --
>         1.8.3.2
>         
>         
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] [PATCH 6/6] Set mountmap["icecream"] from settings for now.

[Brian Dolbec]

mounts and mountmap are source paths.
I plan to add a source_mounts to settings later to compliment the target_mounts paths.
---
 catalyst                        | 1 +
 modules/generic_stage_target.py | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/catalyst b/catalyst
index 82b0942..cb6c022 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"icecream": "/var/cache/icecream",
 		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index c8d31e1..43df545 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -243,8 +243,8 @@ class generic_stage_target(generic_target):
 			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
-			self.mounts.append("/var/cache/icecream")
-			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
+			self.mounts.append("icecream")
+			self.mountmap["icecream"] = self.settings["icecream"]
 			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
 				self.env["PATH"]
 
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 6/6] Set mountmap["icecream"] from settings for now.

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 05:57:10PM -0800, Brian Dolbec wrote:
> mounts and mountmap are source paths.

“mountmap” is for source paths.  “mounts” is for path-agnostic keys.

> -			self.mounts.append("/var/cache/icecream")
> -			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
> +			self.mounts.append("icecream")
> +			self.mountmap["icecream"] = self.settings["icecream"]

Looks good to me.

I haven't actually tested this series, but I did test two commits
further along in Brian's “pending” branch (8dfe5c7, Remove an extra
slash in the path, 2013-12-17).  Everything worked well there.  I'll
retest this series once we've worked the style kinks out of v1 ;).

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 --]

Re: [gentoo-catalyst] [PATCH 6/6] Set mountmap["icecream"] from settings for now.

[W. Trevor King]

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

On Sat, Dec 28, 2013 at 06:53:16PM -0800, W. Trevor King wrote:
> I haven't actually tested this series, but I did test two commits
> further along in Brian's “pending” branch (8dfe5c7, Remove an extra
> slash in the path, 2013-12-17).  Everything worked well there.  I'll
> retest this series once we've worked the style kinks out of v1 ;).

Oops, I actually tested five commits further along (8066333, chmod +x
targets/stage1/build.py, 2013-12-17), building trough stage3.  If
we're retesting after this series settles down, I suppose my earlier
testing may not matter much, but I thought I'd clarify anyway ;).

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 --]

Re: [gentoo-catalyst] Patches to fix rebased patches already applied to current master. Version-2

[Brian Dolbec]

These patches have all previous review suggestions applied.
They also now have _ALL_ style considerations appplied if the line has been touched.
Patch numbers do not necessarily correspond to the previous ones.
The prevoius patch #4 has been sqaushed into patch #1.
There is a new patch #4, which has been split out of patch #5

[gentoo-catalyst] [PATCH 1/6] modules/generic_stage_target.py: Use portdir, distdir,... instead of paths for keys

[Brian Dolbec]

If snapcache is enabled, self.mounts and self.mountmap had not been modified
like they were if it was not enabled.
Fix a missed self.mounts key change from a path as key.
original patch: 27b751ffd9594e0128a3d70f83c7af18dab03838
Added a print statement for debugging to be converted to logging in the future.
---
 modules/generic_stage_target.py | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3e60f72..6c8cdbc 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,10 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
-			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
-				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
-				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
+			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
+				"portdir": self.settings["snapshot_cache_path"] + "/portage",
+				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
@@ -904,7 +904,8 @@ class generic_stage_target(generic_target):
 					os.makedirs(self.mountmap[x],0755)
 
 			src=self.mountmap[x]
-			if "SNAPCACHE" in self.settings and x == "/usr/portage":
+			#print "bind(); src =", src
+			if "SNAPCACHE" in self.settings and x == "portdir":
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 1/6] modules/generic_stage_target.py: Use portdir, distdir,... instead of paths for keys

[W. Trevor King]

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

There's still an extra space in the commit summary:

  “: Use” vs. “:  Use”

Not a big deal, but it's an easy fix ;).

On Sun, Dec 29, 2013 at 05:50:48PM -0800, Brian Dolbec wrote:
> Fix a missed self.mounts key change from a path as key.
> original patch: 27b751ffd9594e0128a3d70f83c7af18dab03838

As I mentioned in my comments on v1#4:

On Sat, Dec 28, 2013 at 06:30:15PM -0800, W. Trevor King wrote:
> On Sat, Dec 28, 2013 at 05:57:08PM -0800, Brian Dolbec wrote:
> > original patch: 27b751ffd9594e0128a3d70f83c7af18dab03838
>
> Actually, I don't think this is related to 27b751f
> (modules/generic_stage_target.py: Use 'portdir' instead of hard-coding
> '/usr/portage', 2012-12-19), which was only about
> self.settings["portdir"].

All of the stuff in here (from both v1#1 and v1#4) is just fixing the
mountmap keys and matching mounts entries for the SNAPCACHE case.  If
you want to blame a commit, I'd blame the whole:

  Use '*' instead of '*' as the mountmap key

series for skipping the SNAPCACHE cases.

On Sun, Dec 29, 2013 at 05:50:48PM -0800, Brian Dolbec wrote:
> -			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
> -			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
> -				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
> -				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
> +			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> +			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
> +				"portdir": self.settings["snapshot_cache_path"] + "/portage",
> +				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
> …
> -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> +			#print "bind(); src =", src
> +			if "SNAPCACHE" in self.settings and x == "portdir":

The code changes look great :).

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 --]

[gentoo-catalyst] [PATCH 2/6] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
Fix the coding style in the lines touched.
---
 modules/generic_stage_target.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 6c8cdbc..51375c0 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,17 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
-			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
-				"portdir": self.settings["snapshot_cache_path"] + "/portage",
-				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
+			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap = {
+				"proc": "/proc",
+				"dev": "/dev",
+				"devpts": "/dev/pts",
+				"portdir": normpath("/".join([
+					self.settings["snapshot_cache_path"],
+					self.settings["repo_name"],
+					])),
+				"distdir": self.settings["distdir"],
+				"port_tmpdir": "tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 2/6] Fix a missed self.settings["repo_name"] migration

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 05:50:49PM -0800, Brian Dolbec wrote:
> -			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> -			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
> -				"portdir": self.settings["snapshot_cache_path"] + "/portage",
> -				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
> +			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> +			self.mountmap = {
> +				"proc": "/proc",
> +				"dev": "/dev",
> +				"devpts": "/dev/pts",
> +				"portdir": normpath("/".join([
> +					self.settings["snapshot_cache_path"],
> +					self.settings["repo_name"],
> +					])),
> +				"distdir": self.settings["distdir"],
> +				"port_tmpdir": "tmpfs"}

Love it :).

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 --]

[gentoo-catalyst] [PATCH 3/6] Fixes commit 463d98f (modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage', 2012-12-19).Add "local_overlay" to configdefaults.

[Brian Dolbec]

---
 catalyst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/catalyst b/catalyst
index 4ea4248..82b0942 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
 		"portdir": "/usr/portage",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 3/6] Fixes commit 463d98f (modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage', 2012-12-19).Add "local_overlay" to configdefaults.

[W. Trevor King]

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

That's a long subject ;).  How about:

  Add "local_overlay" to configdefaults

  Fixes commit 463d98f (modules/generic_stage_target.py: Use a
  'local_overlay' setting instead of hard-coding '/usr/local/portage',
  2012-12-19).

That gives you both a concise summary and a more detailed body without
making wrapping in `git log --oneline`.

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 --]

Re: [gentoo-catalyst] [PATCH 3/6] Fixes commit 463d98f (modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage', 2012-12-19).Add "local_overlay" to configdefaults.

[Brian Dolbec]

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

On Sun, 2013-12-29 at 20:02 -0800, W. Trevor King wrote:
> That's a long subject ;).  How about:
> 
>   Add "local_overlay" to configdefaults
> 
>   Fixes commit 463d98f (modules/generic_stage_target.py: Use a
>   'local_overlay' setting instead of hard-coding '/usr/local/portage',
>   2012-12-19).
> 
> That gives you both a concise summary and a more detailed body without
> making wrapping in `git log --oneline`.
> 
> Cheers,
> Trevor
> 

First: I'm not feeling well, so please don't take this too personally.

I committed exactly what you had in the previous review (copy/paste).  I
knew it was too long, but when I tried breaking it up, git puked on it.
so I thought what the hell and left it.  I wasn't going to "waste" more
time on it.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 3/6] Fixes commit 463d98f (modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage', 2012-12-19).Add "local_overlay" to configdefaults.

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 09:32:35PM -0800, Brian Dolbec wrote:
> On Sun, 2013-12-29 at 20:02 -0800, W. Trevor King wrote:
> > That's a long subject ;).  How about:
> > 
> >   Add "local_overlay" to configdefaults
> > 
> >   Fixes commit 463d98f (modules/generic_stage_target.py: Use a
> >   'local_overlay' setting instead of hard-coding '/usr/local/portage',
> >   2012-12-19).
> > 
> > That gives you both a concise summary and a more detailed body without
> > making wrapping in `git log --oneline`.
> 
> First: I'm not feeling well, so please don't take this too personally.

No problem.  I don't think there's a particular hurry to address
everything either, so if you want to just let things cook for a week
and then get back to them when you're feeling better, that's certainly
fine with me.

> I committed exactly what you had in the previous review
> (copy/paste).  I knew it was too long, but when I tried breaking it
> up, git puked on it.  so I thought what the hell and left it.  I
> wasn't going to "waste" more time on it.

I've pushed:

  git://tremily.us/catalyst.git pending

with the commit-message cleanups I've suggested on v2.  If you want me
to help figure out why Git didn't like your earlier attempt, just let
me know.

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 --]

Re: [gentoo-catalyst] [PATCH 3/6] Fixes commit 463d98f (modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage', 2012-12-19).Add "local_overlay" to configdefaults.

[Brian Dolbec]

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

On Sun, 2013-12-29 at 22:35 -0800, W. Trevor King wrote:

> I've pushed:
> 
>   git://tremily.us/catalyst.git pending
> 
> with the commit-message cleanups I've suggested on v2.  If you want me
> to help figure out why Git didn't like your earlier attempt, just let
> me know.
> 
> Cheers,
> Trevor
> 

hey :)  I already changed it.  This time it worked :)

I think it had to do with the ('s being split to different lines.  For
whatever reason, git was trying to run part of the message as a command
call.

Anyway, here's the current corrected message:


Add "local_overlay" to configdefaults.

Fixes commit 463d98f
  (modules/generic_stage_target.py: Use a 'local_overlay'
   setting instead of hard-coding '/usr/local/portage',
   2012-12-19)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] [PATCH 4/6] Fix mounts and mountmap port_logdir code block.

[Brian Dolbec]

---
 modules/generic_stage_target.py | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 51375c0..8f7654e 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -4,6 +4,11 @@ from generic_target import *
 from stat import *
 import catalyst_lock
 
+
+PORT_LOGDIR_CLEAN = \
+	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
+
+
 class generic_stage_target(generic_target):
 	"""
 	This class does all of the chroot setup, copying of files, etc. It is
@@ -233,10 +238,10 @@ class generic_stage_target(generic_target):
 			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
 
 		if "port_logdir" in self.settings:
-			self.mounts.append("/var/log/portage")
-			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
-			self.env["PORT_LOGDIR"]="/var/log/portage"
-			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
+			self.mounts.append("port_logdir")
+			self.mountmap["port_logdir"] = self.settings["port_logdir"]
+			self.env["PORT_LOGDIR"] = self.settings["port_logdir"]
+			self.env["PORT_LOGDIR_CLEAN"] = PORT_LOGDIR_CLEAN
 
 	def override_cbuild(self):
 		if "CBUILD" in self.makeconf:
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 4/6] Fix mounts and mountmap port_logdir code block.

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 05:50:51PM -0800, Brian Dolbec wrote:
> +PORT_LOGDIR_CLEAN = \
> +	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'

Pulling out this global variable is new for v2.  I don't think I'm a
fan.  I think it should either be hard-coded where we setup
env["PORT_LOGDIR_CLEAN"] (as it was before) or pulled out of
self.settings (like PORT_LOGDIR).

> -			self.mounts.append("/var/log/portage")
> -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> -			self.env["PORT_LOGDIR"]="/var/log/portage"
> -			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
> +			self.mounts.append("port_logdir")
> +			self.mountmap["port_logdir"] = self.settings["port_logdir"]
> +			self.env["PORT_LOGDIR"] = self.settings["port_logdir"]
> +			self.env["PORT_LOGDIR_CLEAN"] = PORT_LOGDIR_CLEAN

Other than the newly-global PORT_LOGDIR_CLEAN, this looks good to me.

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 --]

Re: [gentoo-catalyst] [PATCH 4/6] Fix mounts and mountmap port_logdir code block.

[Brian Dolbec]

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

On Sun, 2013-12-29 at 20:09 -0800, W. Trevor King wrote:
> On Sun, Dec 29, 2013 at 05:50:51PM -0800, Brian Dolbec wrote:
> > +PORT_LOGDIR_CLEAN = \
> > +	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
> 
> Pulling out this global variable is new for v2.  I don't think I'm a
> fan.  I think it should either be hard-coded where we setup
> env["PORT_LOGDIR_CLEAN"] (as it was before) or pulled out of
> self.settings (like PORT_LOGDIR).
> 
> > -			self.mounts.append("/var/log/portage")
> > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > -			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
> > +			self.mounts.append("port_logdir")
> > +			self.mountmap["port_logdir"] = self.settings["port_logdir"]
> > +			self.env["PORT_LOGDIR"] = self.settings["port_logdir"]
> > +			self.env["PORT_LOGDIR_CLEAN"] = PORT_LOGDIR_CLEAN
> 
> Other than the newly-global PORT_LOGDIR_CLEAN, this looks good to me.
> 
> Cheers,
> Trevor
> 

PORT_LOGDIR_CLEAN is a copy of the setting directly from portage.  I
fully intend on importing it later when it makes more sense to actually
make use of some of portage's api.  In the meantime this moves it out of
a long line situation in the code block that would have meant cutting
and splicing it back together.  When the commit that makes the new
defaults.py file is merged.  It will be moved there.  Later when we
import some info/code from portage. We can assign it accordingly.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

Temporary location to define TARGET_MOUNTS, IT will be moved to a new defaults.py file in a later commit.
I also plan to make them configurable.
Cleans up all self.mounts, self.mountmap usage.
Replace multiple path additions with one instance at the beginning of the function, reuse the result multiple times.
Add some extra debug prints (to be converted to logging later)
---
 modules/generic_stage_target.py | 83 ++++++++++++++++++++++++++---------------
 modules/stage1_target.py        |  5 ++-
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 8f7654e..8d98e58 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -8,6 +8,20 @@ import catalyst_lock
 PORT_LOGDIR_CLEAN = \
 	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
 
+TARGET_MOUNTS_DEFAULTS = {
+	"ccache": "/var/tmp/ccache",
+	"dev": "/dev",
+	"devpts": "/dev/pts",
+	"distdir": "/usr/portage/distfiles",
+	"icecream": "/usr/lib/icecc/bin",
+	"kerncache": "/tmp/kerncache",
+	"packagedir": "/usr/portage/packages",
+	"portdir": "/usr/portage",
+	"port_tmpdir": "/var/tmp/portage",
+	"port_logdir": "/var/log/portage",
+	"proc": "/proc",
+	}
+
 
 class generic_stage_target(generic_target):
 	"""
@@ -178,6 +192,8 @@ class generic_stage_target(generic_target):
 			file_locate(self.settings,["portage_confdir"],expand=0)
 
 		""" Setup our mount points """
+		# initialize our target mounts.
+		self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()
 		if "SNAPCACHE" in self.settings:
 			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
 			self.mountmap = {
@@ -230,12 +246,13 @@ class generic_stage_target(generic_target):
 			self.mounts.append("ccache")
 			self.mountmap["ccache"] = ccdir
 			""" for the chroot: """
-			self.env["CCACHE_DIR"]="/var/tmp/ccache"
+			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
 			self.mounts.append("/var/cache/icecream")
 			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
-			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
+			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
+				self.env["PATH"]
 
 		if "port_logdir" in self.settings:
 			self.mounts.append("port_logdir")
@@ -614,33 +631,34 @@ class generic_stage_target(generic_target):
 				"kill-chroot-pids script failed.",env=self.env)
 
 	def mount_safety_check(self):
-		mypath=self.settings["chroot_path"]
-
 		"""
 		Check and verify that none of our paths in mypath are mounted. We don't
 		want to clean up with things still mounted, and this allows us to check.
 		Returns 1 on ok, 0 on "something is still mounted" case.
 		"""
 
-		if not os.path.exists(mypath):
+		if not os.path.exists(self.settings["chroot_path"]):
 			return
 
+		print "self.mounts =", self.mounts
 		for x in self.mounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			print "mount_safety_check() x =", x, target
+			if not os.path.exists(target):
 				continue
 
-			if ismount(mypath + self.mountmap[x]):
+			if ismount(target):
 				""" Something is still mounted "" """
 				try:
-					print self.mountmap[x] + " is still mounted; performing auto-bind-umount...",
+					print target + " is still mounted; performing auto-bind-umount...",
 					""" Try to umount stuff ourselves """
 					self.unbind()
-					if ismount(mypath + self.mountmap[x]):
-						raise CatalystError, "Auto-unbind failed for " + self.mountmap[x]
+					if ismount(target):
+						raise CatalystError, "Auto-unbind failed for " + target
 					else:
 						print "Auto-unbind successful..."
 				except CatalystError:
-					raise CatalystError, "Unable to auto-unbind " + self.mountmap[x]
+					raise CatalystError, "Unable to auto-unbind " + target
 
 	def unpack(self):
 		unpack=True
@@ -908,12 +926,14 @@ class generic_stage_target(generic_target):
 
 	def bind(self):
 		for x in self.mounts:
-			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
-				os.makedirs(self.settings["chroot_path"]+x,0755)
+			#print "bind(); x =", x
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			if not os.path.exists(target):
+				os.makedirs(target, 0755)
 
 			if not os.path.exists(self.mountmap[x]):
 				if not self.mountmap[x] == "tmpfs":
-					os.makedirs(self.mountmap[x],0755)
+					os.makedirs(self.mountmap[x], 0755)
 
 			src=self.mountmap[x]
 			#print "bind(); src =", src
@@ -921,20 +941,22 @@ class generic_stage_target(generic_target):
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-					retval = os.system("mount -t devfs none " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount -t devfs none " + target
+					retval=os.system(cmd)
 				else:
-					retval = os.system("mount_nullfs " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount_nullfs " + src + " " + target
+					retval=os.system(cmd)
 			else:
 				if src == "tmpfs":
 					if "var_tmpfs_portage" in self.settings:
-						retval=os.system("mount -t tmpfs -o size="+\
-							self.settings["var_tmpfs_portage"]+"G "+src+" "+\
-							self.settings["chroot_path"]+x)
+						cmd = "mount -t tmpfs -o size=" + \
+							self.settings["var_tmpfs_portage"] + "G " + \
+							src + " " + target
+						retval=os.system(cmd)
 				else:
-					retval = os.system("mount --bind " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount --bind " + src + " " + target
+					#print "bind(); cmd =", cmd
+					retval=os.system(cmd)
 			if retval!=0:
 				self.unbind()
 				raise CatalystError,"Couldn't bind mount " + src
@@ -946,26 +968,25 @@ class generic_stage_target(generic_target):
 		myrevmounts.reverse()
 		""" Unmount in reverse order for nested bind-mounts """
 		for x in myrevmounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(mypath + self.target_mounts[x])
+			if not os.path.exists(target):
 				continue
 
-			if not ismount(mypath + self.mountmap[x]):
+			if not ismount(target):
 				continue
 
-			retval=os.system("umount "+\
-				os.path.join(mypath, self.mountmap[x].lstrip(os.path.sep)))
+			retval=os.system("umount " + target)
 
 			if retval!=0:
-				warn("First attempt to unmount: " + mypath +
-					self.mountmap[x] +" failed.")
+				warn("First attempt to unmount: " + target + " failed.")
 				warn("Killing any pids still running in the chroot")
 
 				self.kill_chroot_pids()
 
-				retval2 = os.system("umount " + mypath + self.mountmap[x])
+				retval2 = os.system("umount " + target)
 				if retval2!=0:
 					ouch=1
-					warn("Couldn't umount bind mount: " + mypath + self.mountmap[x])
+					warn("Couldn't umount bind mount: " + target)
 
 			if "SNAPCACHE" in self.settings and x == "/usr/portage":
 				try:
diff --git a/modules/stage1_target.py b/modules/stage1_target.py
index aa43926..5f4ffa0 100644
--- a/modules/stage1_target.py
+++ b/modules/stage1_target.py
@@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
 			os.makedirs(self.settings["stage_path"]+"/proc")
 
 		# alter the mount mappings to bind mount proc onto it
-		self.mounts.append("/tmp/stage1root/proc")
-		self.mountmap["/tmp/stage1root/proc"]="/proc"
+		self.mounts.append("stage1root/proc")
+		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
+		self.mountmap["stage1root/proc"] = "/proc"
 
 def register(foo):
 	foo.update({"stage1":stage1_target})
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 05:50:52PM -0800, Brian Dolbec wrote:
> Temporary location to define TARGET_MOUNTS, IT will be moved to a new defaults.py file in a later commit.

“TARGET_MOUNTS, IT” → “TARGET_MOUNTS_DEFAULTS.  It”.

I'd also wrap the long lines into paragraphs (just like email :), but
I'm fine if you'd rather stick with one line per sentence.

> +TARGET_MOUNTS_DEFAULTS = {
> +	"ccache": "/var/tmp/ccache",
> +	"dev": "/dev",
> +	"devpts": "/dev/pts",
> +	"distdir": "/usr/portage/distfiles",
> +	"icecream": "/usr/lib/icecc/bin",
> +	"kerncache": "/tmp/kerncache",
> +	"packagedir": "/usr/portage/packages",
> +	"portdir": "/usr/portage",
> +	"port_tmpdir": "/var/tmp/portage",
> +	"port_logdir": "/var/log/portage",
> +	"proc": "/proc",
> +	}
> +

+1 for alphebetizing this.

The rest of the patch matches v1#5 (less the v2#4 changes) with the
future-predicting comments pulled out.  All of that looks good too.

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 --]

[gentoo-catalyst] [PATCH 6/6] Set mountmap["icecream"] from settings for now.

[Brian Dolbec]

“mountmap” is for source paths.  “mounts” is for path-agnostic keys.
I plan to add a source_mounts to settings later to compliment the target_mounts paths.
---
 catalyst                        | 1 +
 modules/generic_stage_target.py | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/catalyst b/catalyst
index 82b0942..cb6c022 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"icecream": "/var/cache/icecream",
 		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 8d98e58..bbea57a 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -249,8 +249,8 @@ class generic_stage_target(generic_target):
 			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
-			self.mounts.append("/var/cache/icecream")
-			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
+			self.mounts.append("icecream")
+			self.mountmap["icecream"] = self.settings["icecream"]
 			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
 				self.env["PATH"]
 
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 6/6] Set mountmap["icecream"] from settings for now.

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 05:50:53PM -0800, Brian Dolbec wrote:
> “mountmap” is for source paths.  “mounts” is for path-agnostic keys.

Looks good to me ;).

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 --]

Re: [gentoo-catalyst] Patches to fix rebased patches already applied to current master. Version-2

[W. Trevor King]

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

On Sun, Dec 29, 2013 at 05:50:47PM -0800, Brian Dolbec wrote:
> These patches have all previous review suggestions applied.

You can use:

  $ git format-patch -v2 …

to get [PATCH v2] tags in your subjects so you don't have to say
“Version-2” by hand.  I see you did use --in-reply-to, thanks :).

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 --]

[PATCH v3 0/6] "Re: [gentoo-catalyst] Patches to fix rebased patches already applied to current master. Version-2

[Brian Dolbec]

Changes since version 2:

Patch 1/6  commit message fixes
Patch 2/6  sort the dictionary
Patch 3/6  fix commit message (was too long)
Patch 5/6  commit message typo fixes
Patch 6/6  same



Brian Dolbec (6):
  modules/generic_stage_target.py: Use portdir, distdir, ... instead of
    paths for keys
  Fix a missed self.settings["repo_name"] migration
  Add "local_overlay" to configdefaults.
  Fix mounts and mountmap port_logdir code block.
  modules/generic_stage_target.py, modules/stage1_target.py: Add a
    target_mounts dictionary
  Set mountmap["icecream"] from settings for now.

 catalyst                        |   2 +
 modules/generic_stage_target.py | 118 ++++++++++++++++++++++++++--------------
 modules/stage1_target.py        |   5 +-
 3 files changed, 81 insertions(+), 44 deletions(-)

-- 
1.8.3.2

[gentoo-catalyst] [PATCH v3 1/6] modules/generic_stage_target.py: Use portdir, distdir, ... instead of paths for keys

[Brian Dolbec]

If snapcache is enabled, self.mounts and self.mountmap had not been
modified like they were if it was not enabled.

Added a print statement for debugging to be converted to logging in
the future.
---
 modules/generic_stage_target.py | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3e60f72..6c8cdbc 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,10 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
-			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
-				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
-				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
+			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
+				"portdir": self.settings["snapshot_cache_path"] + "/portage",
+				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
@@ -904,7 +904,8 @@ class generic_stage_target(generic_target):
 					os.makedirs(self.mountmap[x],0755)
 
 			src=self.mountmap[x]
-			if "SNAPCACHE" in self.settings and x == "/usr/portage":
+			#print "bind(); src =", src
+			if "SNAPCACHE" in self.settings and x == "portdir":
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-- 
1.8.3.2

[gentoo-catalyst] Re: [PATCH v3 1/6] modules/generic_stage_target.py: Use portdir, distdir, ... instead of paths for keys

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:17PM -0800, Brian Dolbec wrote:
> If snapcache is enabled, self.mounts and self.mountmap had not been
> modified like they were if it was not enabled.
> 
> Added a print statement for debugging to be converted to logging in
> the future.

Subject and commit message look great this round.

> index 3e60f72..6c8cdbc 100644

And the content change matches v2.

I think this is ready to merge.

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 --]

[gentoo-catalyst] [PATCH v3 2/6] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
Fix the coding style in the lines touched.
---
 modules/generic_stage_target.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 6c8cdbc..3d6a45e 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,17 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
-			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
-				"portdir": self.settings["snapshot_cache_path"] + "/portage",
-				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
+			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap = {
+				"dev": "/dev",
+				"devpts": "/dev/pts",
+				"distdir": self.settings["distdir"],
+				"portdir": normpath("/".join([
+					self.settings["snapshot_cache_path"],
+					self.settings["repo_name"],
+					])),
+				"port_tmpdir": "tmpfs"}
+				"proc": "/proc",
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH v3 2/6] Fix a missed self.settings["repo_name"] migration

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:18PM -0800, Brian Dolbec wrote:
> -			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> -			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
> -				"portdir": self.settings["snapshot_cache_path"] + "/portage",
> -				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
> +			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> +			self.mountmap = {
> +				"dev": "/dev",
> +				"devpts": "/dev/pts",
> +				"distdir": self.settings["distdir"],
> +				"portdir": normpath("/".join([
> +					self.settings["snapshot_cache_path"],
> +					self.settings["repo_name"],
> +					])),
> +				"port_tmpdir": "tmpfs"}
> +				"proc": "/proc",

Oops, trailing paren should follow the "proc" entry.  A good reason to
have it on it's own line:

		self.mountmap = {
			"dev": "/dev",
			"devpts": "/dev/pts",
			"distdir": self.settings["distdir"],
			"portdir": normpath("/".join([
				self.settings["snapshot_cache_path"],
				self.settings["repo_name"],
				])),
			"port_tmpdir": "tmpfs",
			"proc": "/proc",
      }

Otherwise looks good.

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 --]

[gentoo-catalyst] [PATCH v4 1/1] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
Fix the coding style in the lines touched.
---
 modules/generic_stage_target.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 6c8cdbc..3d6a45e 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,17 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
-			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
-				"portdir": self.settings["snapshot_cache_path"] + "/portage",
-				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
+			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap = {
+				"dev": "/dev",
+				"devpts": "/dev/pts",
+				"distdir": self.settings["distdir"],
+				"portdir": normpath("/".join([
+					self.settings["snapshot_cache_path"],
+					self.settings["repo_name"],
+					])),
+				"port_tmpdir": "tmpfs"}
+				"proc": "/proc",
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

[gentoo-catalyst] [PATCH v4 1/1] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
Fix the coding style in the lines touched.
---
 modules/generic_stage_target.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 6c8cdbc..3d6a45e 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,17 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
-			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
-				"portdir": self.settings["snapshot_cache_path"] + "/portage",
-				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
+			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap = {
+				"dev": "/dev",
+				"devpts": "/dev/pts",
+				"distdir": self.settings["distdir"],
+				"portdir": normpath("/".join([
+					self.settings["snapshot_cache_path"],
+					self.settings["repo_name"],
+					])),
+				"port_tmpdir": "tmpfs"}
+				"proc": "/proc",
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

[gentoo-catalyst] [PATCH v5 1/1] Fix a missed self.settings["repo_name"] migration

[Brian Dolbec]

Use normpath() on it as well.
Fix the coding style in the lines touched.
---
 modules/generic_stage_target.py | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 6c8cdbc..ce43c79 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,18 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
-			self.mountmap={"proc": "/proc", "dev": "/dev", "devpts": "/dev/pts",
-				"portdir": self.settings["snapshot_cache_path"] + "/portage",
-				"distdir": self.settings["distdir"], "port_tmpdir": "tmpfs"}
+			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap = {
+				"dev": "/dev",
+				"devpts": "/dev/pts",
+				"distdir": self.settings["distdir"],
+				"portdir": normpath("/".join([
+					self.settings["snapshot_cache_path"],
+					self.settings["repo_name"],
+					])),
+				"port_tmpdir": "tmpfs",
+				"proc": "/proc",
+				}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

[gentoo-catalyst] [PATCH v3 3/6] Add "local_overlay" to configdefaults.

[Brian Dolbec]

Fixes commit 463d98f
  (modules/generic_stage_target.py: Use a 'local_overlay'
   setting instead of hard-coding '/usr/local/portage',
   2012-12-19)
---
 catalyst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/catalyst b/catalyst
index 4ea4248..82b0942 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
 		"portdir": "/usr/portage",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH v3 3/6] Add "local_overlay" to configdefaults.

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:19PM -0800, Brian Dolbec wrote:
> Fixes commit 463d98f
>   (modules/generic_stage_target.py: Use a 'local_overlay'
>    setting instead of hard-coding '/usr/local/portage',
>    2012-12-19)

Subject/body splitting looks good.

> index 4ea4248..82b0942 100755

Content hasn't changed.

I think this is ready to merge.

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 --]

[gentoo-catalyst] [PATCH v3 4/6] Fix mounts and mountmap port_logdir code block.

[Brian Dolbec]

---
 modules/generic_stage_target.py | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3d6a45e..55b336f 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -4,6 +4,11 @@ from generic_target import *
 from stat import *
 import catalyst_lock
 
+
+PORT_LOGDIR_CLEAN = \
+	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
+
+
 class generic_stage_target(generic_target):
 	"""
 	This class does all of the chroot setup, copying of files, etc. It is
@@ -233,10 +238,10 @@ class generic_stage_target(generic_target):
 			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
 
 		if "port_logdir" in self.settings:
-			self.mounts.append("/var/log/portage")
-			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
-			self.env["PORT_LOGDIR"]="/var/log/portage"
-			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
+			self.mounts.append("port_logdir")
+			self.mountmap["port_logdir"] = self.settings["port_logdir"]
+			self.env["PORT_LOGDIR"] = self.settings["port_logdir"]
+			self.env["PORT_LOGDIR_CLEAN"] = PORT_LOGDIR_CLEAN
 
 	def override_cbuild(self):
 		if "CBUILD" in self.makeconf:
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH v3 4/6] Fix mounts and mountmap port_logdir code block.

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:20PM -0800, Brian Dolbec wrote:
>  modules/generic_stage_target.py | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

No changes since v2.  Still ready to merge, but we'll want to fix #2
first.

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 --]

[gentoo-catalyst] [PATCH v3 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

Temporary location to define TARGET_MOUNTS_DEFAULTS.
It will be moved to a new defaults.py file in a later commit.
I also plan to make them configurable.

Also:
* Clean up all self.mounts, self.mountmap usage.
* Replace multiple path additions with one instance at the
  beginning of the function, reuse the result multiple times.
* Add some extra debug prints (to be converted to logging later)
---
 modules/generic_stage_target.py | 83 ++++++++++++++++++++++++++---------------
 modules/stage1_target.py        |  5 ++-
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 55b336f..c57c504 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -8,6 +8,20 @@ import catalyst_lock
 PORT_LOGDIR_CLEAN = \
 	'find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
 
+TARGET_MOUNTS_DEFAULTS = {
+	"ccache": "/var/tmp/ccache",
+	"dev": "/dev",
+	"devpts": "/dev/pts",
+	"distdir": "/usr/portage/distfiles",
+	"icecream": "/usr/lib/icecc/bin",
+	"kerncache": "/tmp/kerncache",
+	"packagedir": "/usr/portage/packages",
+	"portdir": "/usr/portage",
+	"port_tmpdir": "/var/tmp/portage",
+	"port_logdir": "/var/log/portage",
+	"proc": "/proc",
+	}
+
 
 class generic_stage_target(generic_target):
 	"""
@@ -178,6 +192,8 @@ class generic_stage_target(generic_target):
 			file_locate(self.settings,["portage_confdir"],expand=0)
 
 		""" Setup our mount points """
+		# initialize our target mounts.
+		self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy()
 		if "SNAPCACHE" in self.settings:
 			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
 			self.mountmap = {
@@ -230,12 +246,13 @@ class generic_stage_target(generic_target):
 			self.mounts.append("ccache")
 			self.mountmap["ccache"] = ccdir
 			""" for the chroot: """
-			self.env["CCACHE_DIR"]="/var/tmp/ccache"
+			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
 			self.mounts.append("/var/cache/icecream")
 			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
-			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
+			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
+				self.env["PATH"]
 
 		if "port_logdir" in self.settings:
 			self.mounts.append("port_logdir")
@@ -614,33 +631,34 @@ class generic_stage_target(generic_target):
 				"kill-chroot-pids script failed.",env=self.env)
 
 	def mount_safety_check(self):
-		mypath=self.settings["chroot_path"]
-
 		"""
 		Check and verify that none of our paths in mypath are mounted. We don't
 		want to clean up with things still mounted, and this allows us to check.
 		Returns 1 on ok, 0 on "something is still mounted" case.
 		"""
 
-		if not os.path.exists(mypath):
+		if not os.path.exists(self.settings["chroot_path"]):
 			return
 
+		print "self.mounts =", self.mounts
 		for x in self.mounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			print "mount_safety_check() x =", x, target
+			if not os.path.exists(target):
 				continue
 
-			if ismount(mypath + self.mountmap[x]):
+			if ismount(target):
 				""" Something is still mounted "" """
 				try:
-					print self.mountmap[x] + " is still mounted; performing auto-bind-umount...",
+					print target + " is still mounted; performing auto-bind-umount...",
 					""" Try to umount stuff ourselves """
 					self.unbind()
-					if ismount(mypath + self.mountmap[x]):
-						raise CatalystError, "Auto-unbind failed for " + self.mountmap[x]
+					if ismount(target):
+						raise CatalystError, "Auto-unbind failed for " + target
 					else:
 						print "Auto-unbind successful..."
 				except CatalystError:
-					raise CatalystError, "Unable to auto-unbind " + self.mountmap[x]
+					raise CatalystError, "Unable to auto-unbind " + target
 
 	def unpack(self):
 		unpack=True
@@ -908,12 +926,14 @@ class generic_stage_target(generic_target):
 
 	def bind(self):
 		for x in self.mounts:
-			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
-				os.makedirs(self.settings["chroot_path"]+x,0755)
+			#print "bind(); x =", x
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			if not os.path.exists(target):
+				os.makedirs(target, 0755)
 
 			if not os.path.exists(self.mountmap[x]):
 				if not self.mountmap[x] == "tmpfs":
-					os.makedirs(self.mountmap[x],0755)
+					os.makedirs(self.mountmap[x], 0755)
 
 			src=self.mountmap[x]
 			#print "bind(); src =", src
@@ -921,20 +941,22 @@ class generic_stage_target(generic_target):
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-					retval = os.system("mount -t devfs none " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount -t devfs none " + target
+					retval=os.system(cmd)
 				else:
-					retval = os.system("mount_nullfs " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount_nullfs " + src + " " + target
+					retval=os.system(cmd)
 			else:
 				if src == "tmpfs":
 					if "var_tmpfs_portage" in self.settings:
-						retval=os.system("mount -t tmpfs -o size="+\
-							self.settings["var_tmpfs_portage"]+"G "+src+" "+\
-							self.settings["chroot_path"]+x)
+						cmd = "mount -t tmpfs -o size=" + \
+							self.settings["var_tmpfs_portage"] + "G " + \
+							src + " " + target
+						retval=os.system(cmd)
 				else:
-					retval = os.system("mount --bind " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount --bind " + src + " " + target
+					#print "bind(); cmd =", cmd
+					retval=os.system(cmd)
 			if retval!=0:
 				self.unbind()
 				raise CatalystError,"Couldn't bind mount " + src
@@ -946,26 +968,25 @@ class generic_stage_target(generic_target):
 		myrevmounts.reverse()
 		""" Unmount in reverse order for nested bind-mounts """
 		for x in myrevmounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(mypath + self.target_mounts[x])
+			if not os.path.exists(target):
 				continue
 
-			if not ismount(mypath + self.mountmap[x]):
+			if not ismount(target):
 				continue
 
-			retval=os.system("umount "+\
-				os.path.join(mypath, self.mountmap[x].lstrip(os.path.sep)))
+			retval=os.system("umount " + target)
 
 			if retval!=0:
-				warn("First attempt to unmount: " + mypath +
-					self.mountmap[x] +" failed.")
+				warn("First attempt to unmount: " + target + " failed.")
 				warn("Killing any pids still running in the chroot")
 
 				self.kill_chroot_pids()
 
-				retval2 = os.system("umount " + mypath + self.mountmap[x])
+				retval2 = os.system("umount " + target)
 				if retval2!=0:
 					ouch=1
-					warn("Couldn't umount bind mount: " + mypath + self.mountmap[x])
+					warn("Couldn't umount bind mount: " + target)
 
 			if "SNAPCACHE" in self.settings and x == "/usr/portage":
 				try:
diff --git a/modules/stage1_target.py b/modules/stage1_target.py
index aa43926..5f4ffa0 100644
--- a/modules/stage1_target.py
+++ b/modules/stage1_target.py
@@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
 			os.makedirs(self.settings["stage_path"]+"/proc")
 
 		# alter the mount mappings to bind mount proc onto it
-		self.mounts.append("/tmp/stage1root/proc")
-		self.mountmap["/tmp/stage1root/proc"]="/proc"
+		self.mounts.append("stage1root/proc")
+		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
+		self.mountmap["stage1root/proc"] = "/proc"
 
 def register(foo):
 	foo.update({"stage1":stage1_target})
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH v3 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:21PM -0800, Brian Dolbec wrote:
> Temporary location to define TARGET_MOUNTS_DEFAULTS.
> It will be moved to a new defaults.py file in a later commit.
> I also plan to make them configurable.
> 
> Also:
> * Clean up all self.mounts, self.mountmap usage.
> * Replace multiple path additions with one instance at the
>   beginning of the function, reuse the result multiple times.
> * Add some extra debug prints (to be converted to logging later)

Commit message looks great.  The content is the same as v2, so I like
it.

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 --]

Re: [gentoo-catalyst] [PATCH v3 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

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

On Thu, Jan 02, 2014 at 08:16:21PM -0800, Brian Dolbec wrote:
> +TARGET_MOUNTS_DEFAULTS = {

I just noticed this is doubly pluralized.  I'd rather fix it now, but
if we just want to merge this before hitting v6, I'm happy to submit a
patch to fix it later.

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 --]

Re: [gentoo-catalyst] [PATCH v3 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

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

On Thu, 2014-01-02 at 22:08 -0800, W. Trevor King wrote:
> On Thu, Jan 02, 2014 at 08:16:21PM -0800, Brian Dolbec wrote:
> > +TARGET_MOUNTS_DEFAULTS = {
> 
> I just noticed this is doubly pluralized.  I'd rather fix it now, but
> if we just want to merge this before hitting v6, I'm happy to submit a
> patch to fix it later.
> 
> Cheers,
> Trevor
> 

Yeah, I didn't notice it either to be honest.  It will be easy to change
them when they are moved to a new defaults.py file coming in some other
commits soon.  Those files will all need some rebasing already due to
all the changes made to the ones preceding them.

This minor change is not worth rebasing everything again.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[gentoo-catalyst] [PATCH v3 6/6] Set mountmap["icecream"] from settings for now.

[Brian Dolbec]

“mountmap” is for source paths.  “mounts” is for path-agnostic keys.
I plan to add a source_mounts to settings later to compliment the target_mounts paths.
---
 catalyst                        | 1 +
 modules/generic_stage_target.py | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/catalyst b/catalyst
index 82b0942..cb6c022 100755
--- a/catalyst
+++ b/catalyst
@@ -66,6 +66,7 @@ def parse_config(myconfig):
 	confdefaults = {
 		"distdir": "/usr/portage/distfiles",
 		"hash_function": "crc32",
+		"icecream": "/var/cache/icecream",
 		"local_overlay": "/usr/local/portage",
 		"options": "",
 		"packagedir": "/usr/portage/packages",
diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index c57c504..c6a06b6 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -249,8 +249,8 @@ class generic_stage_target(generic_target):
 			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
-			self.mounts.append("/var/cache/icecream")
-			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
+			self.mounts.append("icecream")
+			self.mountmap["icecream"] = self.settings["icecream"]
 			self.env["PATH"] = self.target_mounts["icecream"] + ":" + \
 				self.env["PATH"]
 
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH v3 6/6] Set mountmap["icecream"] from settings for now.

[W. Trevor King]

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

Same as v2, so still looks good to me.

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 --]