[gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops

public inbox for gentoo-catalyst@lists.gentoo.org
 help / color / mirror / Atom feed

* [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops
@ 2016-05-20 14:27 Mike Frysinger
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly Mike Frysinger
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Mike Frysinger @ 2016-05-20 14:27 UTC (permalink / raw
  To: gentoo-catalyst

Rather than shell out to cp/rm/mv, use python native code to do things.
---
 catalyst/base/stagebase.py | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 2009ab6..0b25516 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -1013,8 +1013,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		else:
 			log.notice('Setting up chroot...')
 
-			cmd("cp /etc/resolv.conf " + self.settings["chroot_path"] + "/etc/",
-				env=self.env)
+			shutil.copy('/etc/resolv.conf', self.settings['chroot_path'] + '/etc/')
 
 			# Copy over the envscript, if applicable
 			if "envscript" in self.settings:
@@ -1030,18 +1029,15 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					'Catalyst Maintainers use VERY minimal envscripts, if used at all.\n'
 					'You have been warned.')
 
-				cmd("cp "+self.settings["envscript"]+" "+\
-					self.settings["chroot_path"]+"/tmp/envscript",\
-					env=self.env)
+				shutil.copy(self.settings['envscript'],
+					self.settings['chroot_path'] + '/tmp/envscript')
 
 			# Copy over /etc/hosts from the host in case there are any
 			# specialties in there
-			if os.path.exists(self.settings["chroot_path"]+"/etc/hosts"):
-				cmd("mv "+self.settings["chroot_path"]+"/etc/hosts "+\
-					self.settings["chroot_path"]+"/etc/hosts.catalyst",\
-					env=self.env)
-				cmd("cp /etc/hosts "+self.settings["chroot_path"]+"/etc/hosts",\
-					env=self.env)
+			hosts_file = self.settings['chroot_path'] + '/etc/hosts'
+			if os.path.exists(hosts_file):
+				os.rename(hosts_file, hosts_file + '.catalyst')
+				shutil.copy('/etc/hosts', hosts_file)
 
 			# Modify and write out make.conf (for the chroot)
 			makepath = normpath(self.settings["chroot_path"] +
@@ -1162,10 +1158,9 @@ class StageBase(TargetBase, ClearBase, GenBase):
 				clear_path(self.settings["destpath"] + x)
 
 		# Put /etc/hosts back into place
-		if os.path.exists(self.settings["chroot_path"]+"/etc/hosts.catalyst"):
-			cmd("mv -f "+self.settings["chroot_path"]+"/etc/hosts.catalyst "+\
-				self.settings["chroot_path"]+"/etc/hosts",\
-				env=self.env)
+		hosts_file = self.settings['chroot_path'] + '/etc/hosts'
+		if os.path.exists(hosts_file + '.catalyst'):
+			os.rename(hosts_file + '.catalyst', hosts_file)
 
 		# Remove our overlay
 		if os.path.exists(self.settings["chroot_path"] + self.settings["local_overlay"]):
@@ -1565,11 +1560,8 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			env=self.env)
 
 		if "boot/kernel/"+kname+"/initramfs_overlay" in self.settings:
-			if os.path.exists(self.settings["chroot_path"]+\
-				"/tmp/initramfs_overlay/"):
-				log.notice('Cleaning up temporary overlay dir')
-				cmd("rm -R "+self.settings["chroot_path"]+\
-					"/tmp/initramfs_overlay/",env=self.env)
+			log.notice('Cleaning up temporary overlay dir')
+			clear_dir(self.settings['chroot_path'] + '/tmp/initramfs_overlay/')
 
 		self.resume.is_enabled("build_kernel_"+kname)
 
@@ -1586,11 +1578,10 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					self.settings[key])
 
 			try:
-				cmd('cp ' + self.settings[key] + ' ' +
-					self.settings['chroot_path'] + '/var/tmp/' + kname + '.config',
-					env=self.env)
+				shutil.copy(self.settings[key],
+					self.settings['chroot_path'] + '/var/tmp/' + kname + '.config')
 
-			except CatalystError:
+			except IOError:
 				self.unbind()
 
 	def _copy_initramfs_overlay(self, kname):
-- 
2.8.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly
  2016-05-20 14:27 [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Mike Frysinger
@ 2016-05-20 14:27 ` Mike Frysinger
  2016-05-20 15:48   ` Brian Dolbec
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir Mike Frysinger
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Mike Frysinger @ 2016-05-20 14:27 UTC (permalink / raw
  To: gentoo-catalyst

Very few of the commands we execute actually need to be run through a
shell.  Extend the cmd helper to take an argv and convert the majority
of calls over to that.
---
 catalyst/base/stagebase.py    | 94 +++++++++++++++++++------------------------
 catalyst/support.py           | 48 +++++++++++-----------
 catalyst/targets/grp.py       |  9 +++--
 catalyst/targets/netboot.py   | 28 ++++++-------
 catalyst/targets/netboot2.py  | 14 +++----
 catalyst/targets/snapshot.py  | 14 +++++--
 catalyst/targets/tinderbox.py |  6 +--
 7 files changed, 104 insertions(+), 109 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 0b25516..78e5b94 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -13,7 +13,7 @@ from catalyst import log
 from catalyst.defaults import (SOURCE_MOUNT_DEFAULTS, TARGET_MOUNT_DEFAULTS,
 	PORT_LOGDIR_CLEAN)
 from catalyst.support import (CatalystError, file_locate, normpath,
-	cmd, list_bashify, read_makeconf, ismount, file_check)
+	cmd, read_makeconf, ismount, file_check)
 from catalyst.base.targetbase import TargetBase
 from catalyst.base.clearbase import ClearBase
 from catalyst.base.genbase import GenBase
@@ -648,7 +648,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		killcmd = normpath(self.settings["sharedir"] +
 			self.settings["shdir"] + "/support/kill-chroot-pids.sh")
 		if os.path.exists(killcmd):
-			cmd(killcmd, env=self.env)
+			cmd([killcmd], env=self.env)
 
 	def mount_safety_check(self):
 		"""
@@ -873,10 +873,9 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			clear_path(self.settings['chroot_path'] +
 				self.settings['port_conf'] + '/make.profile')
 			ensure_dirs(self.settings['chroot_path'] + self.settings['port_conf'])
-			cmd("ln -sf ../.." + self.settings["portdir"] + "/profiles/" +
-				self.settings["target_profile"] + " " +
-				self.settings["chroot_path"] +
-				self.settings["port_conf"] + "/make.profile",
+			cmd(['ln', '-sf',
+				'../..' + self.settings['portdir'] + '/profiles/' + self.settings['target_profile'],
+				self.settings['chroot_path'] + self.settings['port_conf'] + '/make.profile'],
 				env=self.env)
 			self.resume.enable("config_profile_link")
 
@@ -892,7 +891,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 				# The trailing slashes on both paths are important:
 				# We want to make sure rsync copies the dirs into each
 				# other and not as subdirs.
-				cmd('rsync -a %s/ %s/' % (self.settings['portage_confdir'], dest),
+				cmd(['rsync', '-a', self.settings['portage_confdir'] + '/', dest + '/'],
 					env=self.env)
 				self.resume.enable("setup_confdir")
 
@@ -914,7 +913,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 				"/root_overlay"]:
 				if os.path.exists(x):
 					log.info('Copying root_overlay: %s', x)
-					cmd('rsync -a ' + x + '/ ' + self.settings['chroot_path'],
+					cmd(['rsync', '-a', x + '/', self.settings['chroot_path']],
 						env=self.env)
 
 	def base_dirs(self):
@@ -922,7 +921,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
 	def bind(self):
 		for x in self.mounts:
-			_cmd = ''
 			log.debug('bind(); x = %s', x)
 			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
 			ensure_dirs(target, mode=0o755)
@@ -935,23 +933,25 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			log.debug('bind(); src = %s', src)
 			if "snapcache" in self.settings["options"] and x == "portdir":
 				self.snapcache_lock.read_lock()
+			_cmd = None
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-					_cmd = "mount -t devfs none " + target
+					_cmd = ['mount', '-t', 'devfs', 'none', target]
 				else:
-					_cmd = "mount_nullfs " + src + " " + target
+					_cmd = ['mount_nullfs', src, target]
 			else:
 				if src == "tmpfs":
 					if "var_tmpfs_portage" in self.settings:
-						_cmd = "mount -t tmpfs -o size=" + \
-							self.settings["var_tmpfs_portage"] + "G " + \
-							src + " " + target
+						_cmd = ['mount', '-t', 'tmpfs',
+							'-o', 'size=' + self.settings['var_tmpfs_portage'] + 'G',
+							src, target]
 				elif src == "shmfs":
-					_cmd = "mount -t tmpfs -o noexec,nosuid,nodev shm " + target
+					_cmd = ['mount', '-t', 'tmpfs', '-o', 'noexec,nosuid,nodev', 'shm', target]
 				else:
-					_cmd = "mount --bind " + src + " " + target
-			log.debug('bind(); _cmd = %s', _cmd)
-			cmd(_cmd, env=self.env, fail_func=self.unbind)
+					_cmd = ['mount', '--bind', src, target]
+			if _cmd:
+				log.debug('bind(); _cmd = %s', _cmd)
+				cmd(_cmd, env=self.env, fail_func=self.unbind)
 		log.debug('bind(); finished :D')
 
 	def unbind(self):
@@ -1134,7 +1134,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		else:
 			if "fsscript" in self.settings:
 				if os.path.exists(self.settings["controller_file"]):
-					cmd(self.settings['controller_file'] + ' fsscript',
+					cmd([self.settings['controller_file'], 'fsscript'],
 						env=self.env)
 					self.resume.enable("fsscript")
 
@@ -1144,7 +1144,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			log.notice('Resume point detected, skipping rcupdate operation...')
 		else:
 			if os.path.exists(self.settings["controller_file"]):
-				cmd(self.settings["controller_file"]+" rc-update",\
+				cmd([self.settings['controller_file'], 'rc-update'],
 					env=self.env)
 				self.resume.enable("rcupdate")
 
@@ -1178,12 +1178,12 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
 		# Clean up old and obsoleted files in /etc
 		if os.path.exists(self.settings["stage_path"]+"/etc"):
-			cmd("find "+self.settings["stage_path"]+\
-				"/etc -maxdepth 1 -name \"*-\" | xargs rm -f",\
+			cmd(['find', self.settings['stage_path'] + '/etc',
+				'-maxdepth', '1', '-name', '*-', '-delete'],
 				env=self.env)
 
 		if os.path.exists(self.settings["controller_file"]):
-			cmd(self.settings['controller_file'] + ' clean', env=self.env)
+			cmd([self.settings['controller_file'], 'clean'], env=self.env)
 			self.resume.enable("clean")
 
 	def empty(self):
@@ -1224,7 +1224,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					clear_path(self.settings["chroot_path"] + x)
 				try:
 					if os.path.exists(self.settings["controller_file"]):
-						cmd(self.settings['controller_file'] + ' clean',
+						cmd([self.settings['controller_file'], 'clean'],
 							env=self.env)
 						self.resume.enable("remove")
 				except:
@@ -1238,7 +1238,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		else:
 			try:
 				if os.path.exists(self.settings["controller_file"]):
-					cmd(self.settings['controller_file'] + ' preclean',
+					cmd([self.settings['controller_file'], 'preclean'],
 						env=self.env)
 					self.resume.enable("preclean")
 
@@ -1295,7 +1295,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			try:
 				if os.path.exists(self.settings["controller_file"]):
 					log.info('run_local() starting controller script...')
-					cmd(self.settings["controller_file"]+" run",\
+					cmd([self.settings['controller_file'], 'run'],
 						env=self.env)
 					self.resume.enable("run_local")
 				else:
@@ -1422,19 +1422,12 @@ class StageBase(TargetBase, ClearBase, GenBase):
 				if isinstance(self.settings[self.settings['spec_prefix']+'/unmerge'], str):
 					self.settings[self.settings["spec_prefix"]+"/unmerge"]=\
 						[self.settings[self.settings["spec_prefix"]+"/unmerge"]]
-				myunmerge=\
-					self.settings[self.settings["spec_prefix"]+"/unmerge"][:]
-
-				for x in range(0,len(myunmerge)):
-					# Surround args with quotes for passing to bash, allows
-					# things like "<" to remain intact
-					myunmerge[x]="'"+myunmerge[x]+"'"
-				myunmerge = ' '.join(myunmerge)
 
 				# Before cleaning, unmerge stuff
 				try:
-					cmd(self.settings['controller_file'] +
-						' unmerge ' + myunmerge, env=self.env)
+					cmd([self.settings['controller_file'], 'unmerge'] +
+						self.settings[self.settings['spec_prefix'] + '/unmerge'],
+						env=self.env)
 					log.info('unmerge shell script')
 				except CatalystError:
 					self.unbind()
@@ -1447,9 +1440,8 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			log.notice('Resume point detected, skipping target_setup operation...')
 		else:
 			log.notice('Setting up filesystems per filesystem type')
-			cmd(self.settings["controller_file"]+\
-				" target_image_setup "+ self.settings["target_path"],\
-				env=self.env)
+			cmd([self.settings['controller_file'], 'target_image_setup',
+				self.settings['target_path']], env=self.env)
 			self.resume.enable("target_setup")
 
 	def setup_overlay(self):
@@ -1460,7 +1452,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			if self.settings["spec_prefix"]+"/overlay" in self.settings:
 				for x in self.settings[self.settings["spec_prefix"]+"/overlay"]:
 					if os.path.exists(x):
-						cmd('rsync -a ' + x + '/ ' + self.settings['target_path'],
+						cmd(['rsync', '-a', x + '/', self.settings['target_path']],
 							env=self.env)
 				self.resume.enable("setup_overlay")
 
@@ -1471,8 +1463,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		else:
 			# Create the ISO
 			if "iso" in self.settings:
-				cmd(self.settings["controller_file"]+" iso "+\
-					self.settings['iso'],
+				cmd([self.settings['controller_file'], 'iso', self.settings['iso']],
 					env=self.env)
 				self.gen_contents_file(self.settings["iso"])
 				self.gen_digest_file(self.settings["iso"])
@@ -1492,12 +1483,9 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					and self.resume.is_enabled("build_packages"):
 					log.notice('Resume point detected, skipping build_packages operation...')
 				else:
-					mypack=\
-						list_bashify(self.settings[self.settings["spec_prefix"]\
-						+"/packages"])
 					try:
-						cmd(self.settings["controller_file"]+\
-							" build_packages "+mypack,\
+						cmd([self.settings['controller_file'], 'build_packages'] +
+							self.settings[self.settings["spec_prefix"] + '/packages'],
 							env=self.env)
 						fileutils.touch(build_packages_resume)
 						self.resume.enable("build_packages")
@@ -1518,7 +1506,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					if isinstance(mynames, str):
 						mynames=[mynames]
 					# Execute the script that sets up the kernel build environment
-					cmd(self.settings['controller_file'] + ' pre-kmerge',
+					cmd([self.settings['controller_file'], 'pre-kmerge'],
 						env=self.env)
 					for kname in mynames:
 						self._build_kernel(kname=kname)
@@ -1556,7 +1544,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		self._copy_initramfs_overlay(kname=kname)
 
 		# Execute the script that builds the kernel
-		cmd(self.settings['controller_file'] + ' kernel ' + kname,
+		cmd([self.settings['controller_file'], 'kernel', kname],
 			env=self.env)
 
 		if "boot/kernel/"+kname+"/initramfs_overlay" in self.settings:
@@ -1566,7 +1554,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		self.resume.is_enabled("build_kernel_"+kname)
 
 		# Execute the script that cleans up the kernel build environment
-		cmd(self.settings['controller_file'] + ' post-kmerge',
+		cmd([self.settings['controller_file'], 'post-kmerge'],
 			env=self.env)
 
 	def _copy_kernel_config(self, kname):
@@ -1604,8 +1592,8 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			log.notice('Resume point detected, skipping bootloader operation...')
 		else:
 			try:
-				cmd(self.settings["controller_file"]+\
-					" bootloader " + self.settings["target_path"].rstrip('/'),\
+				cmd([self.settings['controller_file'], 'bootloader',
+					self.settings['target_path'].rstrip('/')],
 					env=self.env)
 				self.resume.enable("bootloader")
 			except CatalystError:
@@ -1618,7 +1606,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			log.notice('Resume point detected, skipping build_packages operation...')
 		else:
 			try:
-				cmd(self.settings['controller_file'] + ' livecd-update',
+				cmd([self.settings['controller_file'], 'livecd-update'],
 					env=self.env)
 				self.resume.enable("livecd_update")
 
diff --git a/catalyst/support.py b/catalyst/support.py
index 0ce49d2..f2ae5bb 100644
--- a/catalyst/support.py
+++ b/catalyst/support.py
@@ -12,18 +12,6 @@ from catalyst.defaults import valid_config_file_values
 BASH_BINARY             = "/bin/bash"
 
 
-def list_bashify(mylist):
-	if isinstance(mylist, str):
-		mypack=[mylist]
-	else:
-		mypack=mylist[:]
-	for x in range(0,len(mypack)):
-		# surround args with quotes for passing to bash,
-		# allows things like "<" to remain intact
-		mypack[x]="'"+mypack[x]+"'"
-	return ' '.join(mypack)
-
-
 class CatalystError(Exception):
 	def __init__(self, message, print_traceback=False):
 		if message:
@@ -31,26 +19,38 @@ class CatalystError(Exception):
 
 
 def cmd(mycmd, env=None, debug=False, fail_func=None):
-	if env is None:
-		env = {}
+	"""Run the external |mycmd|.
+
+	If |mycmd| is a string, then it's assumed to be a bash snippet and will
+	be run through bash.  Otherwise, it's a standalone command line and will
+	be run directly.
+	"""
 	log.debug('cmd: %r', mycmd)
 	sys.stdout.flush()
-	args=[BASH_BINARY]
-	if "BASH_ENV" not in env:
+
+	if env is None:
+		env = {}
+	if 'BASH_ENV' not in env:
 		env = env.copy()
-		env["BASH_ENV"] = "/etc/spork/is/not/valid/profile.env"
-	if debug:
-		args.append("-x")
-	args.append("-c")
-	args.append(mycmd)
+		env['BASH_ENV'] = '/etc/spork/is/not/valid/profile.env'
+
+	args = []
+	if isinstance(mycmd, str):
+		args.append(BASH_BINARY)
+		if debug:
+			args.append('-x')
+		args.extend(['-c', mycmd])
+	else:
+		args.extend(mycmd)
 
 	log.debug('args: %r', args)
 	proc = Popen(args, env=env)
-	if proc.wait() != 0:
+	ret = proc.wait()
+	if ret:
 		if fail_func:
-			log.error('CMD(), NON-Zero command return.  Running fail_func().')
+			log.error('cmd(%r) exited %s; running fail_func().', args, ret)
 			fail_func()
-		raise CatalystError("cmd() NON-zero return value from: %s" % args,
+		raise CatalystError('cmd(%r) exited %s' % (args, ret),
 			print_traceback=False)
 
 
diff --git a/catalyst/targets/grp.py b/catalyst/targets/grp.py
index c1bd2dd..049bc55 100644
--- a/catalyst/targets/grp.py
+++ b/catalyst/targets/grp.py
@@ -7,7 +7,7 @@ import os
 import glob
 
 from catalyst import log
-from catalyst.support import (CatalystError, normpath, cmd, list_bashify)
+from catalyst.support import (CatalystError, normpath, cmd)
 from catalyst.base.stagebase import StageBase
 
 
@@ -41,10 +41,11 @@ class grp(StageBase):
 	def run_local(self):
 		for pkgset in self.settings["grp"]:
 			# example call: "grp.sh run pkgset cd1 xmms vim sys-apps/gleep"
-			mypackages=list_bashify(self.settings["grp/"+pkgset+"/packages"])
 			try:
-				cmd(self.settings["controller_file"]+" run "+self.settings["grp/"+pkgset+"/type"]\
-					+" "+pkgset+" "+mypackages,env=self.env)
+				cmd([self.settings['controller_file'], 'run',
+					self.settings['grp/' + pkgset + '/type'],
+					pkgset] + self.settings['grp/' + pkgset + '/packages'],
+					env=self.env)
 
 			except CatalystError:
 				self.unbind()
diff --git a/catalyst/targets/netboot.py b/catalyst/targets/netboot.py
index 5285c05..161300d 100644
--- a/catalyst/targets/netboot.py
+++ b/catalyst/targets/netboot.py
@@ -7,7 +7,7 @@ import os
 
 from catalyst import log
 from catalyst.support import (CatalystError, normpath,
-	cmd, list_bashify, file_locate)
+	cmd, file_locate)
 
 from catalyst.base.stagebase import StageBase
 
@@ -62,22 +62,22 @@ class netboot(StageBase):
 #	def build_packages(self):
 #		# build packages
 #		if "netboot/packages" in self.settings:
-#			mypack=list_bashify(self.settings["netboot/packages"])
-#		try:
-#			cmd(self.settings["controller_file"]+" packages "+mypack,env=self.env)
-#		except CatalystError:
-#			self.unbind()
-#			raise CatalystError("netboot build aborting due to error.",
-#					print_traceback=True)
+#			try:
+#				cmd([self.settings['controller_file'], 'packages'] +
+#					self.settings['netboot/packages'], env=self.env)
+#			except CatalystError:
+#				self.unbind()
+#				raise CatalystError('netboot build aborting due to error.',
+#						print_traceback=True)
 
 	def build_busybox(self):
 		# build busybox
 		if "netboot/busybox_config" in self.settings:
-			mycmd = self.settings["netboot/busybox_config"]
+			mycmd = [self.settings['netboot/busybox_config']]
 		else:
-			mycmd = ""
+			mycmd = []
 		try:
-			cmd(self.settings["controller_file"]+" busybox "+ mycmd,env=self.env)
+			cmd([self.settings['controller_file'], 'busybox'] + mycmd, env=self.env)
 		except CatalystError:
 			self.unbind()
 			raise CatalystError("netboot build aborting due to error.",
@@ -106,8 +106,8 @@ class netboot(StageBase):
 				myfiles.append(self.settings["netboot/extra_files"])
 
 		try:
-			cmd(self.settings["controller_file"]+\
-				" image " + list_bashify(myfiles),env=self.env)
+			cmd([self.settings['controller_file'], 'image'] + myfiles,
+				env=self.env)
 		except CatalystError:
 			self.unbind()
 			raise CatalystError("netboot build aborting due to error.",
@@ -116,7 +116,7 @@ class netboot(StageBase):
 	def create_netboot_files(self):
 		# finish it all up
 		try:
-			cmd(self.settings["controller_file"]+" finish",env=self.env)
+			cmd([self.settings['controller_file'], 'finish'], env=self.env)
 		except CatalystError:
 			self.unbind()
 			raise CatalystError("netboot build aborting due to error.",
diff --git a/catalyst/targets/netboot2.py b/catalyst/targets/netboot2.py
index 568b791..da856ba 100644
--- a/catalyst/targets/netboot2.py
+++ b/catalyst/targets/netboot2.py
@@ -8,7 +8,7 @@ import shutil
 from stat import ST_UID, ST_GID, ST_MODE
 
 from catalyst import log
-from catalyst.support import (CatalystError, normpath, cmd, list_bashify)
+from catalyst.support import (CatalystError, normpath, cmd)
 from catalyst.fileops import ensure_dirs, clear_path
 
 from catalyst.base.stagebase import StageBase
@@ -89,8 +89,8 @@ class netboot2(StageBase):
 					myfiles.append(self.settings["netboot2/extra_files"])
 
 			try:
-				cmd(self.settings["controller_file"]+\
-					" image " + list_bashify(myfiles),env=self.env)
+				cmd([self.settings['controller_file'], 'image'] +
+					myfiles, env=self.env)
 			except CatalystError:
 				self.unbind()
 				raise CatalystError("Failed to copy files to image!",
@@ -106,8 +106,9 @@ class netboot2(StageBase):
 			if "netboot2/overlay" in self.settings:
 				for x in self.settings["netboot2/overlay"]:
 					if os.path.exists(x):
-						cmd("rsync -a "+x+"/ "+\
-							self.settings["chroot_path"] + self.settings["merge_path"], env=self.env)
+						cmd(['rsync', '-a', x + '/',
+							self.settings['chroot_path'] + self.settings['merge_path']],
+							env=self.env)
 				self.resume.enable("setup_overlay")
 
 	def move_kernels(self):
@@ -115,8 +116,7 @@ class netboot2(StageBase):
 		# no auto resume here as we always want the
 		# freshest images moved
 		try:
-			cmd(self.settings["controller_file"]+\
-				" final",env=self.env)
+			cmd([self.settings['controller_file'], 'final'], env=self.env)
 			log.notice('Netboot Build Finished!')
 		except CatalystError:
 			self.unbind()
diff --git a/catalyst/targets/snapshot.py b/catalyst/targets/snapshot.py
index 196166a..7ba94b2 100644
--- a/catalyst/targets/snapshot.py
+++ b/catalyst/targets/snapshot.py
@@ -56,10 +56,16 @@ class snapshot(TargetBase, GenBase):
 		mytmp=self.settings["tmp_path"]
 		ensure_dirs(mytmp)
 
-		target_snapshot = self.settings["portdir"] + "/ " + mytmp + "/%s/" % self.settings["repo_name"]
-		cmd("rsync -a --no-o --no-g --delete --exclude /packages/ --exclude /distfiles/ " +
-			"--exclude /local/ --exclude CVS/ --exclude .svn --filter=H_**/files/digest-* " +
-			target_snapshot, env=self.env)
+		cmd(['rsync', '-a', '--no-o', '--no-g', '--delete',
+			'--exclude=/packages/',
+			'--exclude=/distfiles/',
+			'--exclude=/local/',
+			'--exclude=CVS/',
+			'--exclude=.svn',
+			'--filter=H_**/files/digest-*',
+			self.settings['portdir'] + '/',
+			mytmp + '/' + self.settings['repo_name'] + '/'],
+			env=self.env)
 
 		log.notice('Compressing Portage snapshot tarball ...')
 		compressor = CompressMap(self.settings["compress_definitions"],
diff --git a/catalyst/targets/tinderbox.py b/catalyst/targets/tinderbox.py
index f7895de..85a939f 100644
--- a/catalyst/targets/tinderbox.py
+++ b/catalyst/targets/tinderbox.py
@@ -5,7 +5,7 @@ Tinderbox target
 
 import os
 
-from catalyst.support import cmd, list_bashify, CatalystError
+from catalyst.support import cmd, CatalystError
 from catalyst.base.stagebase import StageBase
 
 
@@ -24,8 +24,8 @@ class tinderbox(StageBase):
 		# example call: "grp.sh run xmms vim sys-apps/gleep"
 		try:
 			if os.path.exists(self.settings["controller_file"]):
-				cmd(self.settings["controller_file"]+" run "+\
-				list_bashify(self.settings["tinderbox/packages"]),env=self.env)
+				cmd([self.settings['controller_file'], 'run'] +
+					self.settings['tinderbox/packages'], env=self.env)
 
 		except CatalystError:
 			self.unbind()
-- 
2.8.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly Mike Frysinger
@ 2016-05-20 15:48   ` Brian Dolbec
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Dolbec @ 2016-05-20 15:48 UTC (permalink / raw
  To: gentoo-catalyst

On Fri, 20 May 2016 10:27:44 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> Very few of the commands we execute actually need to be run through a
> shell.  Extend the cmd helper to take an argv and convert the majority
> of calls over to that.

Looks good to me :)  Thank you

-- 
Brian Dolbec <dolsen>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir
  2016-05-20 14:27 [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Mike Frysinger
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly Mike Frysinger
@ 2016-05-20 14:27 ` Mike Frysinger
  2016-05-20 15:49   ` Brian Dolbec
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd Mike Frysinger
  2016-05-20 15:46 ` [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Brian Dolbec
  3 siblings, 1 reply; 8+ messages in thread
From: Mike Frysinger @ 2016-05-20 14:27 UTC (permalink / raw
  To: gentoo-catalyst

A few places copy & paste the logic we have in the clear_dir func.
Punt them with a simple call to clear_dir.
---
 catalyst/base/stagebase.py   |  9 +--------
 catalyst/targets/netboot2.py | 10 ++--------
 catalyst/targets/snapshot.py | 20 ++------------------
 3 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 78e5b94..f0f3ba9 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -3,7 +3,6 @@ import os
 import imp
 import shutil
 import sys
-from stat import ST_UID, ST_GID, ST_MODE
 
 from snakeoil import fileutils
 
@@ -1202,13 +1201,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 						log.warning('not a directory or does not exist, skipping "empty" operation: %s', x)
 						continue
 					log.info('Emptying directory %s', x)
-					# stat the dir, delete the dir, recreate the dir and set
-					# the proper perms and ownership
-					mystat=os.stat(myemp)
-					shutil.rmtree(myemp)
-					ensure_dirs(myemp, mode=0o755)
-					os.chown(myemp,mystat[ST_UID],mystat[ST_GID])
-					os.chmod(myemp,mystat[ST_MODE])
+					clear_dir(myemp)
 			self.resume.enable("empty")
 
 	def remove(self):
diff --git a/catalyst/targets/netboot2.py b/catalyst/targets/netboot2.py
index da856ba..87dada3 100644
--- a/catalyst/targets/netboot2.py
+++ b/catalyst/targets/netboot2.py
@@ -4,12 +4,10 @@ netboot target, version 2
 # NOTE: That^^ docstring has influence catalyst-spec(5) man page generation.
 
 import os
-import shutil
-from stat import ST_UID, ST_GID, ST_MODE
 
 from catalyst import log
 from catalyst.support import (CatalystError, normpath, cmd)
-from catalyst.fileops import ensure_dirs, clear_path
+from catalyst.fileops import (ensure_dirs, clear_dir, clear_path)
 
 from catalyst.base.stagebase import StageBase
 
@@ -152,11 +150,7 @@ class netboot2(StageBase):
 					log.info('Emptying directory %s', x)
 					# stat the dir, delete the dir, recreate the dir and set
 					# the proper perms and ownership
-					mystat=os.stat(myemp)
-					shutil.rmtree(myemp)
-					ensure_dirs(myemp, mode=0o755)
-					os.chown(myemp,mystat[ST_UID],mystat[ST_GID])
-					os.chmod(myemp,mystat[ST_MODE])
+					clear_dir(myemp)
 		self.resume.enable("empty")
 
 	def set_action_sequence(self):
diff --git a/catalyst/targets/snapshot.py b/catalyst/targets/snapshot.py
index 7ba94b2..8a9acdd 100644
--- a/catalyst/targets/snapshot.py
+++ b/catalyst/targets/snapshot.py
@@ -2,17 +2,13 @@
 Snapshot target
 """
 
-import os
-import shutil
-from stat import ST_UID, ST_GID, ST_MODE
-
 from DeComp.compress import CompressMap
 
 from catalyst import log
 from catalyst.support import normpath, cmd
 from catalyst.base.targetbase import TargetBase
 from catalyst.base.genbase import GenBase
-from catalyst.fileops import ensure_dirs
+from catalyst.fileops import (clear_dir, ensure_dirs)
 
 
 class snapshot(TargetBase, GenBase):
@@ -102,16 +98,4 @@ class snapshot(TargetBase, GenBase):
 		log.info('Cleaning up ...')
 
 	def purge(self):
-		myemp=self.settings["tmp_path"]
-		if os.path.isdir(myemp):
-			log.notice('Emptying directory %s', myemp)
-			# stat the dir, delete the dir, recreate the dir and set
-			# the proper perms and ownership
-			mystat=os.stat(myemp)
-			# There's no easy way to change flags recursively in python
-			if os.uname()[0] == "FreeBSD":
-				os.system("chflags -R noschg "+myemp)
-			shutil.rmtree(myemp)
-			ensure_dirs(myemp, mode=0o755)
-			os.chown(myemp,mystat[ST_UID],mystat[ST_GID])
-			os.chmod(myemp,mystat[ST_MODE])
+		clear_dir(self.settings['tmp_path'])
-- 
2.8.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir Mike Frysinger
@ 2016-05-20 15:49   ` Brian Dolbec
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Dolbec @ 2016-05-20 15:49 UTC (permalink / raw
  To: gentoo-catalyst

On Fri, 20 May 2016 10:27:45 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> A few places copy & paste the logic we have in the clear_dir func.
> Punt them with a simple call to clear_dir.


Slowly punting all the duped code :)

Push it please

-- 
Brian Dolbec <dolsen>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd
  2016-05-20 14:27 [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Mike Frysinger
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly Mike Frysinger
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir Mike Frysinger
@ 2016-05-20 14:27 ` Mike Frysinger
  2016-05-20 15:51   ` Brian Dolbec
  2016-05-20 15:46 ` [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Brian Dolbec
  3 siblings, 1 reply; 8+ messages in thread
From: Mike Frysinger @ 2016-05-20 14:27 UTC (permalink / raw
  To: gentoo-catalyst

Use the existing cmd() helper for running external programs.
---
 catalyst/base/stagebase.py | 11 ++++++-----
 catalyst/fileops.py        |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index f0f3ba9..6695ac4 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -967,16 +967,17 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			if not ismount(target):
 				continue
 
-			retval=os.system("umount " + target)
-
-			if retval!=0:
+			try:
+				cmd(['umount', target])
+			except CatalystError:
 				log.warning('First attempt to unmount failed: %s', target)
 				log.warning('Killing any pids still running in the chroot')
 
 				self.kill_chroot_pids()
 
-				retval2 = os.system("umount " + target)
-				if retval2!=0:
+				try:
+					cmd(['umount', target])
+				except CatalystError:
 					ouch=1
 					log.warning("Couldn't umount bind mount: %s", target)
 
diff --git a/catalyst/fileops.py b/catalyst/fileops.py
index 4b9e200..6971911 100644
--- a/catalyst/fileops.py
+++ b/catalyst/fileops.py
@@ -22,7 +22,7 @@ from snakeoil.osutils import (ensure_dirs as snakeoil_ensure_dirs,
 # pylint: enable=unused-import
 
 from catalyst import log
-from catalyst.support import CatalystError
+from catalyst.support import (cmd, CatalystError)
 
 
 def ensure_dirs(path, gid=-1, uid=-1, mode=0o755, minimal=True,
@@ -79,7 +79,7 @@ def clear_dir(target, mode=0o755, chg_flags=False, remove=False,
 			mystat = os.stat(target)
 			# There's no easy way to change flags recursively in python
 			if chg_flags and os.uname()[0] == "FreeBSD":
-				os.system("chflags -R noschg " + target)
+				cmd(['chflags', '-R', 'noschg', target])
 			log.debug('shutil.rmtree()')
 			shutil.rmtree(target)
 		except Exception:
-- 
2.8.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd Mike Frysinger
@ 2016-05-20 15:51   ` Brian Dolbec
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Dolbec @ 2016-05-20 15:51 UTC (permalink / raw
  To: gentoo-catalyst

On Fri, 20 May 2016 10:27:46 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> Use the existing cmd() helper for running external programs.
> ---

these have been bugging me for quite a while, thank you for getting to
it.

push approved :D

-- 
Brian Dolbec <dolsen>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops
  2016-05-20 14:27 [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Mike Frysinger
                   ` (2 preceding siblings ...)
  2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd Mike Frysinger
@ 2016-05-20 15:46 ` Brian Dolbec
  3 siblings, 0 replies; 8+ messages in thread
From: Brian Dolbec @ 2016-05-20 15:46 UTC (permalink / raw
  To: gentoo-catalyst

On Fri, 20 May 2016 10:27:43 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> Rather than shell out to cp/rm/mv, use python native code to do
> things. ---
>  catalyst/base/stagebase.py | 39
> +++++++++++++++------------------------ 1 file changed, 15
> insertions(+), 24 deletions(-)
> 


Yes, looks good, and about time ;)

-- 
Brian Dolbec <dolsen>



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-05-20 15:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-20 14:27 [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Mike Frysinger
2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 2/4] cmd: add support for running commands directly Mike Frysinger
2016-05-20 15:48   ` Brian Dolbec
2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 3/4] replace ad-hoc implementations of clear_dir Mike Frysinger
2016-05-20 15:49   ` Brian Dolbec
2016-05-20 14:27 ` [gentoo-catalyst] [PATCH 4/4] replace os.system with cmd Mike Frysinger
2016-05-20 15:51   ` Brian Dolbec
2016-05-20 15:46 ` [gentoo-catalyst] [PATCH 1/4] use native code in more places for file ops Brian Dolbec

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox