* [gentoo-catalyst] [PATCH] Fix autoresume file paths to only be configured once.
@ 2014-03-02 23:05 Brian Dolbec
2014-03-05 4:35 ` [gentoo-catalyst] " W. Trevor King
0 siblings, 1 reply; 5+ messages in thread
From: Brian Dolbec @ 2014-03-02 23:05 UTC (permalink / raw
To: gentoo-catalyst
Use: pjoin as a shorter alias to os.path.join()
---
catalyst/targets/generic_stage_target.py | 175 +++++++++++++++++--------------
1 file changed, 95 insertions(+), 80 deletions(-)
diff --git a/catalyst/targets/generic_stage_target.py b/catalyst/targets/generic_stage_target.py
index 2b3d7ce..095327a 100644
--- a/catalyst/targets/generic_stage_target.py
+++ b/catalyst/targets/generic_stage_target.py
@@ -33,6 +33,9 @@ SOURCE_MOUNTS_DEFAULTS = {
"shm": "shmfs",
}
+# for convienience
+pjoin = os.path.join
+
class generic_stage_target(generic_target):
"""
@@ -334,9 +337,10 @@ class generic_stage_target(generic_target):
def set_target_path(self):
self.settings["target_path"]=normpath(self.settings["storedir"]+\
"/builds/"+self.settings["target_subpath"]+".tar.bz2")
- if "AUTORESUME" in self.settings\
- and os.path.exists(self.settings["autoresume_path"]+\
- "setup_target_path"):
+ setup_target_path_resume = pjoin(self.settings["autoresume_path"],
+ "setup_target_path")
+ if "AUTORESUME" in self.settings and \
+ os.path.exists(setup_target_path_resume):
print \
"Resume point detected, skipping target path setup operation..."
else:
@@ -348,7 +352,7 @@ class generic_stage_target(generic_target):
# cmd("rm -f "+self.settings["target_path"],\
# "Could not remove existing file: "\
# +self.settings["target_path"],env=self.env)
- touch(self.settings["autoresume_path"]+"setup_target_path")
+ touch(setup_target_path_resume)
if not os.path.exists(self.settings["storedir"]+"/builds/"):
os.makedirs(self.settings["storedir"]+"/builds/")
@@ -484,10 +488,12 @@ class generic_stage_target(generic_target):
self.chroot_lock=LockDir(self.settings["chroot_path"])
def set_autoresume_path(self):
- self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\
- "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\
- self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
- self.settings["version_stamp"]+"/")
+ self.settings["autoresume_path"] = normpath(pjoin(
+ self.settings["storedir"], "tmp", self.settings["rel_type"],
+ ".autoresume-%s-%s-%s"
+ %(self.settings["target"], self.settings["subarch"],
+ self.settings["version_stamp"])
+ ))
if "AUTORESUME" in self.settings:
print "The autoresume path is " + self.settings["autoresume_path"]
if not os.path.exists(self.settings["autoresume_path"]):
@@ -673,8 +679,8 @@ class generic_stage_target(generic_target):
def unpack(self):
unpack=True
- clst_unpack_hash=read_from_clst(self.settings["autoresume_path"]+\
- "unpack")
+ unpack_resume = pjoin(self.settings["autoresume_path"], "unpack")
+ clst_unpack_hash=read_from_clst(unpack_resume)
if "SEEDCACHE" in self.settings:
if os.path.isdir(self.settings["source_path"]):
@@ -720,7 +726,7 @@ class generic_stage_target(generic_target):
if "AUTORESUME" in self.settings:
if os.path.isdir(self.settings["source_path"]) \
- and os.path.exists(self.settings["autoresume_path"]+"unpack"):
+ and os.path.exists(unpack_resume):
""" Autoresume is valid, SEEDCACHE is valid """
unpack=False
invalid_snapshot=False
@@ -732,8 +738,7 @@ class generic_stage_target(generic_target):
invalid_snapshot=True
elif os.path.isdir(self.settings["source_path"]) \
- and not os.path.exists(self.settings["autoresume_path"]+\
- "unpack"):
+ and not os.path.exists(unpack_resume):
""" Autoresume is invalid, SEEDCACHE """
unpack=True
invalid_snapshot=False
@@ -793,18 +798,19 @@ class generic_stage_target(generic_target):
cmd(unpack_cmd,error_msg,env=self.env)
if "source_path_hash" in self.settings:
- myf=open(self.settings["autoresume_path"]+"unpack","w")
+ myf=open(unpack_resume,"w")
myf.write(self.settings["source_path_hash"])
myf.close()
else:
- touch(self.settings["autoresume_path"]+"unpack")
+ touch(unpack_resume)
else:
print "Resume point detected, skipping unpack operation..."
def unpack_snapshot(self):
unpack=True
- snapshot_hash=read_from_clst(self.settings["autoresume_path"]+\
+ unpack_portage_resume = pjoin(self.settings["autoresume_path"],
"unpack_portage")
+ snapshot_hash=read_from_clst(unpack_portage_resume)
if "SNAPCACHE" in self.settings:
snapshot_cache_hash=\
@@ -841,8 +847,7 @@ class generic_stage_target(generic_target):
if "AUTORESUME" in self.settings \
and os.path.exists(self.settings["chroot_path"]+\
self.settings["portdir"]) \
- and os.path.exists(self.settings["autoresume_path"]\
- +"unpack_portage") \
+ and os.path.exists(unpack_portage_resume) \
and self.settings["snapshot_path_hash"] == snapshot_hash:
print \
"Valid Resume point detected, skipping unpack of portage tree..."
@@ -868,7 +873,7 @@ class generic_stage_target(generic_target):
myf.close()
else:
print "Setting snapshot autoresume point"
- myf=open(self.settings["autoresume_path"]+"unpack_portage","w")
+ myf=open(unpack_portage_resume,"w")
myf.write(self.settings["snapshot_path_hash"])
myf.close()
@@ -876,9 +881,10 @@ class generic_stage_target(generic_target):
self.snapshot_lock_object.unlock()
def config_profile_link(self):
+ config_protect_link_resume = pjoin(self.settings["autoresume_path"],
+ "config_profile_link")
if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+\
- "config_profile_link"):
+ and os.path.exists():
print \
"Resume point detected, skipping config_profile_link operation..."
else:
@@ -892,12 +898,13 @@ class generic_stage_target(generic_target):
self.settings["target_profile"]+" "+\
self.settings["chroot_path"]+"/etc/portage/make.profile",\
"Error creating profile link",env=self.env)
- touch(self.settings["autoresume_path"]+"config_profile_link")
+ touch(config_protect_link_resume)
def setup_confdir(self):
+ setup_confdir_resume = pjoin(self.settings["autoresume_path"],
+ "setup_confdir")
if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+\
- "setup_confdir"):
+ and os.path.exists(setup_confdir_resume):
print "Resume point detected, skipping setup_confdir operation..."
else:
if "portage_confdir" in self.settings:
@@ -905,7 +912,7 @@ class generic_stage_target(generic_target):
cmd("rsync -a "+self.settings["portage_confdir"]+"/ "+\
self.settings["chroot_path"]+"/etc/portage/",\
"Error copying /etc/portage",env=self.env)
- touch(self.settings["autoresume_path"]+"setup_confdir")
+ touch(setup_confdir_resume)
def portage_overlay(self):
""" We copy the contents of our overlays to /usr/local/portage """
@@ -1029,8 +1036,9 @@ class generic_stage_target(generic_target):
self.override_cflags()
self.override_cxxflags()
self.override_ldflags()
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"chroot_setup"):
+ chroot_setup_resume = pjoin(self.settings["autoresume_path"],
+ "chroot_setup")
+ if "AUTORESUME" in self.settings and os.path.exists(chroot_setup_resume):
print "Resume point detected, skipping chroot_setup operation..."
else:
print "Setting up chroot..."
@@ -1126,32 +1134,34 @@ class generic_stage_target(generic_target):
cmd("cp "+self.settings["chroot_path"]+"/etc/portage/make.conf "+\
self.settings["chroot_path"]+"/etc/portage/make.conf.catalyst",\
"Could not backup /etc/portage/make.conf",env=self.env)
- touch(self.settings["autoresume_path"]+"chroot_setup")
+ touch(chroot_setup_resume)
def fsscript(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"fsscript"):
+ fsscript_resume = pjoin(self.settings["autoresume_path"], "fsscript")
+ if "AUTORESUME" in self.settings and os.path.exists(fsscript_resume):
print "Resume point detected, skipping fsscript operation..."
else:
if "fsscript" in self.settings:
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+\
" fsscript","fsscript script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"fsscript")
+ touch(fsscript_resume)
def rcupdate(self):
+ rcupdate_resume = pjoin(self.settings["autoresume_path"], "rcupdate")
if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"rcupdate"):
+ and os.path.exists(rcupdate_resume):
print "Resume point detected, skipping rcupdate operation..."
else:
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+" rc-update",\
"rc-update script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"rcupdate")
+ touch(rcupdate_resume)
def clean(self):
+ clean_resume = pjoin(self.settings["autoresume_path"], "clean")
if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"clean"):
+ and os.path.exists(clean_resume):
print "Resume point detected, skipping clean operation..."
else:
for x in self.settings["cleanables"]:
@@ -1182,11 +1192,11 @@ class generic_stage_target(generic_target):
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+" clean",\
"clean script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"clean")
+ touch(clean_resume)
def empty(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"empty"):
+ empty_resume = pjoin(self.settings["autoresume_path"], "empty")
+ if "AUTORESUME" in self.settings and os.path.exists(empty_resume):
print "Resume point detected, skipping empty operation..."
else:
if self.settings["spec_prefix"]+"/empty" in self.settings:
@@ -1210,11 +1220,11 @@ class generic_stage_target(generic_target):
os.makedirs(myemp,0755)
os.chown(myemp,mystat[ST_UID],mystat[ST_GID])
os.chmod(myemp,mystat[ST_MODE])
- touch(self.settings["autoresume_path"]+"empty")
+ touch(empty_resume)
def remove(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"remove"):
+ remove_resume = pjoin(self.settings["autoresume_path"], "remove")
+ if "AUTORESUME" in self.settings and os.path.exists(remove_resume):
print "Resume point detected, skipping remove operation..."
else:
if self.settings["spec_prefix"]+"/rm" in self.settings:
@@ -1229,29 +1239,29 @@ class generic_stage_target(generic_target):
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+\
" clean","Clean failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"remove")
+ touch(remove_resume)
except:
self.unbind()
raise
def preclean(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"preclean"):
+ preclean_resume = pjoin(self.settings["autoresume_path"], "preclean")
+ if "AUTORESUME" in self.settings and os.path.exists(preclean_resume):
print "Resume point detected, skipping preclean operation..."
else:
try:
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+\
" preclean","preclean script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"preclean")
+ touch(preclean_resume)
except:
self.unbind()
raise CatalystError, "Build failed, could not execute preclean"
def capture(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"capture"):
+ capture_resume = pjoin(self.settings["autoresume_path"], "capture")
+ if "AUTORESUME" in self.settings and os.path.exists(capture_resume):
print "Resume point detected, skipping capture operation..."
else:
""" Capture target in a tarball """
@@ -1272,18 +1282,18 @@ class generic_stage_target(generic_target):
self.gen_contents_file(self.settings["target_path"])
self.gen_digest_file(self.settings["target_path"])
- touch(self.settings["autoresume_path"]+"capture")
+ touch(capture_resume)
def run_local(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"run_local"):
+ run_local_resume = pjoin(self.settings["autoresume_path"], "run_local")
+ if "AUTORESUME" in self.settings and os.path.exists(run_local_resume):
print "Resume point detected, skipping run_local operation..."
else:
try:
if os.path.exists(self.settings["controller_file"]):
cmd("/bin/bash "+self.settings["controller_file"]+" run",\
"run script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"run_local")
+ touch(run_local_resume)
except CatalystError:
self.unbind()
@@ -1350,8 +1360,8 @@ class generic_stage_target(generic_target):
self.chroot_lock.unlock()
def unmerge(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"unmerge"):
+ unmerge_resume = pjoin(self.settings["autoresume_path"], "unmerge")
+ if "AUTORESUME" in self.settings and os.path.exists(unmerge_resume):
print "Resume point detected, skipping unmerge operation..."
else:
if self.settings["spec_prefix"]+"/unmerge" in self.settings:
@@ -1379,22 +1389,25 @@ class generic_stage_target(generic_target):
except CatalystError:
self.unbind()
raise
- touch(self.settings["autoresume_path"]+"unmerge")
+ touch(unmerge_resume)
def target_setup(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"target_setup"):
+ target_setup_resume = pjoin(self.settings["autoresume_path"],
+ "target_setup")
+ if "AUTORESUME" in self.settings and os.path.exists(target_setup_resume):
print "Resume point detected, skipping target_setup operation..."
else:
print "Setting up filesystems per filesystem type"
cmd("/bin/bash "+self.settings["controller_file"]+\
" target_image_setup "+ self.settings["target_path"],\
"target_image_setup script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"target_setup")
+ touch(target_setup_resume)
def setup_overlay(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"setup_overlay"):
+ setup_overlay_resume = pjoin(self.settings["autoresume_path"],
+ "setup_overlay")
+ if "AUTORESUME" in self.settings and \
+ os.path.exists(setup_overlay_resume):
print "Resume point detected, skipping setup_overlay operation..."
else:
if self.settings["spec_prefix"]+"/overlay" in self.settings:
@@ -1404,11 +1417,11 @@ class generic_stage_target(generic_target):
self.settings["target_path"],\
self.settings["spec_prefix"]+"overlay: "+x+\
" copy failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"setup_overlay")
+ touch(setup_overlay_resume)
def create_iso(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"create_iso"):
+ create_iso_resume = pjoin(self.settings["autoresume_path"], "create_iso")
+ if "AUTORESUME" in self.settings and os.path.exists(create_iso_resume):
print "Resume point detected, skipping create_iso operation..."
else:
""" Create the ISO """
@@ -1418,15 +1431,16 @@ class generic_stage_target(generic_target):
env=self.env)
self.gen_contents_file(self.settings["iso"])
self.gen_digest_file(self.settings["iso"])
- touch(self.settings["autoresume_path"]+"create_iso")
+ touch(create_iso_resume)
else:
print "WARNING: livecd/iso was not defined."
print "An ISO Image will not be created."
def build_packages(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+\
- "build_packages"):
+ build_packages_resume = pjoin(self.settings["autoresume_path"],
+ "build_packages")
+ if "AUTORESUME" in self.settings and \
+ os.path.exists(build_packages_resume):
print "Resume point detected, skipping build_packages operation..."
else:
if self.settings["spec_prefix"]+"/packages" in self.settings:
@@ -1442,7 +1456,7 @@ class generic_stage_target(generic_target):
cmd("/bin/bash "+self.settings["controller_file"]+\
" build_packages "+mypack,\
"Error in attempt to build packages",env=self.env)
- touch(self.settings["autoresume_path"]+"build_packages")
+ touch(build_packages_resume)
except CatalystError:
self.unbind()
raise CatalystError,self.settings["spec_prefix"]+\
@@ -1450,8 +1464,9 @@ class generic_stage_target(generic_target):
def build_kernel(self):
"Build all configured kernels"
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"build_kernel"):
+ build_kernel_resume = pjoin(self.settings["autoresume_path"],
+ "build_kernel")
+ if "AUTORESUME" in self.settings and os.path.exists(build_kernel_resume):
print "Resume point detected, skipping build_kernel operation..."
else:
if "boot/kernel" in self.settings:
@@ -1467,7 +1482,7 @@ class generic_stage_target(generic_target):
env=self.env)
for kname in mynames:
self._build_kernel(kname=kname)
- touch(self.settings["autoresume_path"]+"build_kernel")
+ touch(build_kernel_resume)
except CatalystError:
self.unbind()
raise CatalystError,\
@@ -1475,9 +1490,9 @@ class generic_stage_target(generic_target):
def _build_kernel(self, kname):
"Build a single configured kernel by name"
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]\
- +"build_kernel_"+kname):
+ kname_resume = pjoin(self.settings["autoresume_path"],
+ "build_kernel_" + kname)
+ if "AUTORESUME" in self.settings and os.path.exists(kname_resume):
print "Resume point detected, skipping build_kernel for "+kname+" operation..."
return
self._copy_kernel_config(kname=kname)
@@ -1519,8 +1534,7 @@ class generic_stage_target(generic_target):
cmd("rm -R "+self.settings["chroot_path"]+\
"/tmp/initramfs_overlay/",env=self.env)
- touch(self.settings["autoresume_path"]+\
- "build_kernel_"+kname)
+ touch(kname_resume)
"""
Execute the script that cleans up the kernel build
@@ -1573,29 +1587,30 @@ class generic_stage_target(generic_target):
"/initramfs_overlay"],env=self.env)
def bootloader(self):
- if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+"bootloader"):
+ bootloader_resume = pjoin(self.settings["autoresume_path"], "bootloader")
+ if "AUTORESUME" in self.settings and os.path.exists(bootloader_resume):
print "Resume point detected, skipping bootloader operation..."
else:
try:
cmd("/bin/bash "+self.settings["controller_file"]+\
" bootloader " + self.settings["target_path"],\
"Bootloader script failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"bootloader")
+ touch(bootloader_resume)
except CatalystError:
self.unbind()
raise CatalystError,"Script aborting due to error."
def livecd_update(self):
+ livecd_update_resume = pjoin(self.settings["autoresume_path"],
+ "livecd_update")
if "AUTORESUME" in self.settings \
- and os.path.exists(self.settings["autoresume_path"]+\
- "livecd_update"):
+ and os.path.exists(livecd_update_resume):
print "Resume point detected, skipping build_packages operation..."
else:
try:
cmd("/bin/bash "+self.settings["controller_file"]+\
" livecd-update","livecd-update failed.",env=self.env)
- touch(self.settings["autoresume_path"]+"livecd_update")
+ touch(livecd_update_resume)
except CatalystError:
self.unbind()
--
1.8.5.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
2014-03-02 23:05 [gentoo-catalyst] [PATCH] Fix autoresume file paths to only be configured once Brian Dolbec
@ 2014-03-05 4:35 ` W. Trevor King
2014-03-05 5:07 ` Brian Dolbec
0 siblings, 1 reply; 5+ messages in thread
From: W. Trevor King @ 2014-03-05 4:35 UTC (permalink / raw
To: gentoo-catalyst
[-- Attachment #1: Type: text/plain, Size: 4369 bytes --]
On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> Use: pjoin as a shorter alias to os.path.join()
I think the saved characters are not worth the mental overhead of an
additional layer of indirection.
> - if "AUTORESUME" in self.settings\
> - and os.path.exists(self.settings["autoresume_path"]+\
> - "setup_target_path"):
> + setup_target_path_resume = pjoin(self.settings["autoresume_path"],
> + "setup_target_path")
> + if "AUTORESUME" in self.settings and \
> + os.path.exists(setup_target_path_resume):
How about:
setup_target_path_resume = os.path.join(
self.settings["autoresume_path"],
"setup_target_path")
if ("AUTORESUME" in self.settings and
os.path.exists(setup_target_path_resume)):
I don't think that's particularly unweildy, it avoids the need for
line-continuation characters [1], and doesn't leave new readers
wondering “what's pjoin?”.
That's all minor stuff though. I think the creation of a
setup_target_path_resume variable is good.
> - self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\
> - "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\
> - self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
> - self.settings["version_stamp"]+"/")
> + self.settings["autoresume_path"] = normpath(pjoin(
> + self.settings["storedir"], "tmp", self.settings["rel_type"],
> + ".autoresume-%s-%s-%s"
> + %(self.settings["target"], self.settings["subarch"],
> + self.settings["version_stamp"])
> + ))
+1. My personal preference is for:
'.autoresume-{}-{}-{}'.format(
self.settings['target'],
self.settings['subarch'],
self.settings['version_stamp'])))
but whatever ;).
> if os.path.isdir(self.settings["source_path"]) \
> - and os.path.exists(self.settings["autoresume_path"]+"unpack"):
> + and os.path.exists(unpack_resume):
Another place where I'd follow PEP8 [1,2] and use:
if (os.path.isdir(self.settings["source_path"]) and
and os.path.exists(unpack_resume)):
> elif os.path.isdir(self.settings["source_path"]) \
> - and not os.path.exists(self.settings["autoresume_path"]+\
> - "unpack"):
> + and not os.path.exists(unpack_resume):
I'd use:
elif (os.path.isdir(self.settings["source_path"]) and
not os.path.exists(unpack_resume)):
> @@ -876,9 +881,10 @@ class generic_stage_target(generic_target):
> self.snapshot_lock_object.unlock()
>
> def config_profile_link(self):
> + config_protect_link_resume = pjoin(self.settings["autoresume_path"],
> + "config_profile_link")
> if "AUTORESUME" in self.settings \
> - and os.path.exists(self.settings["autoresume_path"]+\
> - "config_profile_link"):
> + and os.path.exists():
Oops, you dropped the argument from exists(). How about:
def config_profile_link(self):
config_profile_link_resume = pjoin(
self.settings['autoresume_path'], 'config_profile_link')
if ('AUTORESUME' in self.settings and
os.path.exists(config_profile_link_resume)):
I also switched 'protect' to 'profile' to match the method name, but I
don't actually know what this file is marking ;).
> - touch(self.settings["autoresume_path"]+"config_profile_link")
> + touch(config_protect_link_resume)
Here's another place I'd use 'profile' over 'protect'.
There are a whole lot of these things with very boilerplate code. Can
we abstract this out to reduce that repetition? I'm not sure what
that would look like at the moment though, and a larger overhaul
shouldn't stand in the way of this patch.
Cheers,
Trevor
[1]: In PEP 8 [3]:
The preferred way of wrapping long lines is by using Python's
implied line continuation inside parentheses, brackets and
braces. Long lines can be broken over multiple lines by
wrapping expressions in parentheses. These should be used in
preference to using a backslash for line continuation.
[2]: In PEP 8 [3]:
The preferred place to break around a binary operator is after
the operator, not before it.
[3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length
--
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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
2014-03-05 4:35 ` [gentoo-catalyst] " W. Trevor King
@ 2014-03-05 5:07 ` Brian Dolbec
2014-03-05 6:27 ` W. Trevor King
0 siblings, 1 reply; 5+ messages in thread
From: Brian Dolbec @ 2014-03-05 5:07 UTC (permalink / raw
To: gentoo-catalyst
On Tue, 4 Mar 2014 20:35:28 -0800
Well for starters this patch was not part of the 3.0 branch. When
working with patches in the pending branch I was running into some
troubles fixed in 3.0 that of course were going to be awhile before
coming to pending. So when I started fixing a couple, I thought what
the hell, and did them all. The unfortunate part is that a bunch of
these changes will be replaced by other code later in the patch series.
"W. Trevor King" <wking@tremily.us> wrote:
> On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> > Use: pjoin as a shorter alias to os.path.join()
>
> I think the saved characters are not worth the mental overhead of an
> additional layer of indirection.
>
> > - if "AUTORESUME" in self.settings\
> > - and os.path.exists(self.settings["autoresume_path"]+\
> > - "setup_target_path"):
> > + setup_target_path_resume = pjoin(self.settings["autoresume_path"],
> > + "setup_target_path")
> > + if "AUTORESUME" in self.settings and \
> > + os.path.exists(setup_target_path_resume):
>
> How about:
>
> setup_target_path_resume = os.path.join(
> self.settings["autoresume_path"],
> "setup_target_path")
> if ("AUTORESUME" in self.settings and
> os.path.exists(setup_target_path_resume)):
>
> I don't think that's particularly unweildy, it avoids the need for
> line-continuation characters [1], and doesn't leave new readers
> wondering “what's pjoin?”.
pjoin is something from snakeoil too, depending on the python version
being run, it does some optimization changes if I remember correctly.
But yes, it is a minor change.
>
> That's all minor stuff though. I think the creation of a
> setup_target_path_resume variable is good.
>
> > - self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\
> > - "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\
> > - self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
> > - self.settings["version_stamp"]+"/")
> > + self.settings["autoresume_path"] = normpath(pjoin(
> > + self.settings["storedir"], "tmp", self.settings["rel_type"],
> > + ".autoresume-%s-%s-%s"
> > + %(self.settings["target"], self.settings["subarch"],
> > + self.settings["version_stamp"])
> > + ))
>
> +1. My personal preference is for:
>
> '.autoresume-{}-{}-{}'.format(
> self.settings['target'],
> self.settings['subarch'],
> self.settings['version_stamp'])))
>
> but whatever ;).
>
I never think of the new .format method. My brain is used to %s, so is
stuck there.
> > if os.path.isdir(self.settings["source_path"]) \
> > - and os.path.exists(self.settings["autoresume_path"]+"unpack"):
> > + and os.path.exists(unpack_resume):
>
> Another place where I'd follow PEP8 [1,2] and use:
>
> if (os.path.isdir(self.settings["source_path"]) and
> and os.path.exists(unpack_resume)):
>
> > elif os.path.isdir(self.settings["source_path"]) \
> > - and not os.path.exists(self.settings["autoresume_path"]+\
> > - "unpack"):
> > + and not os.path.exists(unpack_resume):
>
> I'd use:
>
> elif (os.path.isdir(self.settings["source_path"]) and
> not os.path.exists(unpack_resume)):
>
To be honest, I never thought about that during these changes, I was
just doing what was needed to fix some old issues I had fixed
differently in 3.0, but were going to be awhile.
Easy to fix...
> > @@ -876,9 +881,10 @@ class generic_stage_target(generic_target):
> > self.snapshot_lock_object.unlock()
> >
> > def config_profile_link(self):
> > + config_protect_link_resume = pjoin(self.settings["autoresume_path"],
> > + "config_profile_link")
> > if "AUTORESUME" in self.settings \
> > - and os.path.exists(self.settings["autoresume_path"]+\
> > - "config_profile_link"):
> > + and os.path.exists():
>
> Oops, you dropped the argument from exists().
oops, missed that :/ hmm, why did I not discover that in testing...
I must not have run the right combination of things to discover it.
> How about:
>
> def config_profile_link(self):
> config_profile_link_resume = pjoin(
> self.settings['autoresume_path'], 'config_profile_link')
> if ('AUTORESUME' in self.settings and
> os.path.exists(config_profile_link_resume)):
>
> I also switched 'protect' to 'profile' to match the method name, but I
> don't actually know what this file is marking ;).
>
> > - touch(self.settings["autoresume_path"]+"config_profile_link")
> > + touch(config_protect_link_resume)
>
> Here's another place I'd use 'profile' over 'protect'.
Yeah, My brain must have been stuck in "config_protect" which is a
common portage variable and control.
>
> There are a whole lot of these things with very boilerplate code. Can
> we abstract this out to reduce that repetition? I'm not sure what
> that would look like at the moment though, and a larger overhaul
> shouldn't stand in the way of this patch.
>
> Cheers,
> Trevor
Like I said at the top of the reply.
The autoresume stuff is later replaced with a self contained class.
I was never sure if I should submit it, but it did fix a couple small
issues. I can go either way with this one.
>
> [1]: In PEP 8 [3]:
>
> The preferred way of wrapping long lines is by using Python's
> implied line continuation inside parentheses, brackets and
> braces. Long lines can be broken over multiple lines by
> wrapping expressions in parentheses. These should be used in
> preference to using a backslash for line continuation.
>
> [2]: In PEP 8 [3]:
>
> The preferred place to break around a binary operator is after
> the operator, not before it.
>
> [3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length
>
--
Brian Dolbec <dolsen>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
2014-03-05 5:07 ` Brian Dolbec
@ 2014-03-05 6:27 ` W. Trevor King
2014-03-23 0:19 ` Brian Dolbec
0 siblings, 1 reply; 5+ messages in thread
From: W. Trevor King @ 2014-03-05 6:27 UTC (permalink / raw
To: gentoo-catalyst
[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]
On Tue, Mar 04, 2014 at 09:07:03PM -0800, Brian Dolbec wrote:
> The unfortunate part is that a bunch of these changes will be
> replaced by other code later in the patch series.
I don't think there are any critical fixes here. If it's easier to
drop this cleanup and just jump straight to the final version, that's
fine with me too.
> > On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> > > Use: pjoin as a shorter alias to os.path.join()
> >
> > I think the saved characters are not worth the mental overhead of
> > an additional layer of indirection.
> > …
>
> pjoin is something from snakeoil too, depending on the python
> version being run, it does some optimization changes if I remember
> correctly. But yes, it is a minor change.
I don't see anything in the their docs claiming optimizations [1], but
they do have a C implementation [2], so there must be something they
don't like about os.path.join. I doubt any optimizations would matter
to us, since Python-side path joins are going to be a miniscule part
of any catalyst run.
> > +1. My personal preference is for:
> >
> > '.autoresume-{}-{}-{}'.format(
> > self.settings['target'],
> > self.settings['subarch'],
> > self.settings['version_stamp'])))
> >
> > but whatever ;).
>
> I never think of the new .format method. My brain is used to %s, so
> is stuck there.
I don't think it really matters.
> To be honest, I never thought about that during these changes, I was
> just doing what was needed to fix some old issues I had fixed
> differently in 3.0, but were going to be awhile.
Which issues? Just assigning the paths to new variables? I'm fine
with this commit doing that, regardless of style, and will happily
submit any PEP 8 cleanups I think we need after master has finished
swallowing 3.0 ;).
> The autoresume stuff is later replaced with a self contained class.
> I was never sure if I should submit it, but it did fix a couple
> small issues. I can go either way with this one.
Pulling it out into a class sounds good to me, but if it's easier to
land this first (after fixing the exists() typo) and submit the
class-based autoresume later, I'm fine with that. I wanted to point
out possible style changes, but none of that needs to be settled by
this particular patch. I think the removal of duplicate path
calculations is a step in the right direction.
Cheers,
Trevor
[1]: http://docs.snakeoil.googlecode.com/git/api/snakeoil.osutils.html#snakeoil.osutils.pjoin
[2]: https://code.google.com/p/snakeoil/source/browse/src/posix.c#127
--
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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
2014-03-05 6:27 ` W. Trevor King
@ 2014-03-23 0:19 ` Brian Dolbec
0 siblings, 0 replies; 5+ messages in thread
From: Brian Dolbec @ 2014-03-23 0:19 UTC (permalink / raw
To: gentoo-catalyst
On Tue, 4 Mar 2014 22:27:44 -0800
"W. Trevor King" <wking@tremily.us> wrote:
> On Tue, Mar 04, 2014 at 09:07:03PM -0800, Brian Dolbec wrote:
> > The unfortunate part is that a bunch of these changes will be
> > replaced by other code later in the patch series.
>
> I don't think there are any critical fixes here. If it's easier to
> drop this cleanup and just jump straight to the final version, that's
> fine with me too.
Yeah, they aren't critical, but I'd like to land them now anyway.
>
> > > On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> > > > Use: pjoin as a shorter alias to os.path.join()
> > >
> > > I think the saved characters are not worth the mental overhead of
> > > an additional layer of indirection.
> > > …
> >
I'll look at the pjoin use again.
> > The autoresume stuff is later replaced with a self contained class.
> > I was never sure if I should submit it, but it did fix a couple
> > small issues. I can go either way with this one.
>
> Pulling it out into a class sounds good to me, but if it's easier to
> land this first (after fixing the exists() typo) and submit the
> class-based autoresume later, I'm fine with that. I wanted to point
> out possible style changes, but none of that needs to be settled by
> this particular patch. I think the removal of duplicate path
> calculations is a step in the right direction.
>
> Cheers,
> Trevor
>
--
Brian Dolbec <dolsen>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-23 0:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-02 23:05 [gentoo-catalyst] [PATCH] Fix autoresume file paths to only be configured once Brian Dolbec
2014-03-05 4:35 ` [gentoo-catalyst] " W. Trevor King
2014-03-05 5:07 ` Brian Dolbec
2014-03-05 6:27 ` W. Trevor King
2014-03-23 0:19 ` Brian Dolbec
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox