On Thu, Jan 31, 2013 at 01:39:22PM -0500, W. Trevor King wrote: > I'll see if I can get through 968d818, … sometime later today. I had a second wind ;). Here are some comments on the remaining commits: 968d818 Initial creation of a defaults file. Split out hash and contents to their own classes, files After this, it's not clear to me what the difference is between catalyst.support and catalyst.util. Perhaps they should be merged. I'd also use catalyst.targets.__all__ instead of coding a list of targets in the apparently unrelated catalyst.defaults.valid_build_targets. a4ef493 re-version to "git-rewrite branch" Why? 9d752a7 move confdefaults out of main.py Looks good, except, I'm not sure why you changed from `confdefaults.keys()` to `list(confdefaults)` in parse_config() (which should probably be living in catalyst.config anyway). c303dae some options cleanup, unifying their use, reducing redundancy. While I like the general thrust of this, I'd be happier with explicit boolean options instead of a set of boolean options. For example: confdefaults = { 'autoresume': False, 'ccache': False, … } 6aeb5e3 Move LockInUse from support.py to lock.py, fix bad execption raising, pyflakes cleanup With respect to the exception raising, see my comments on 63a25eb in the previous email. Ahh, it looks like you finish that off with 04068a1. 04068a1 massive pyflakes import cleanup and broken CatalystError calls A lot of this is great stuff (Hooray no `import *`!). However, I'd split the `print_traceback=True` changes out into their own commit (or just change the default value for CatalystError while you're testing). It looks like an unrelated change to catalyst.defaults.required_build_targets snuck into this commit. The hash_map changes should probably be squashed into the earlier hash reorganization, rather than going into this commit. c672887 begin splitting up generic_stage_target into smaller code blocks so snapshot_target does not need to import it since most of it was not used or initialized properly. I'd shift “so snapshot_target…” into the commit body for a shorter summary. Not much to say about the code, since I haven't dug though it yet. 2b29212 some spacing and comment cleanup, etc. Looks ok. 28888a7 remove redundant /bin/bash additions in cmd() calls Looks good. 97802b6 clean out the old spawn functions, use subprocess.call() instead Hooray. Since you're doing this, I'd drop the earlier commits that referenced this change (e.g. d23d96b and 1b2597e). 624eb0d move base stage and target files to thier own sub-pkg s/thier/their/ The diff looks good. 5c689a8 update module loading for the new python structure, rename snapshot_target to snapshot I think I like this. It's good to use __import__. You're missing a blank line after your import_module docstring summary [1]. I think that the import_module API should match Python 3's importlib.import_module [2]. 669451d fix options being reset by a config file See my comments on c303dae. We should really be using ConfigParser's defaults here, instead of rolling our own default system. f56017b fix missed PURGEONLY changes This should be squashed into the settings work that caused it. c5eb7f7 fix mounts, mountmap hardcoding removal, use normpath to fix paths, add some debug print statements, remove clear & purge code from support.py, Split the long commit summary. Can we use logging instead of print? The except (Failure loading " + x + " plugin) should be squashed into the commit that caused it and be restricted to only catch ImportErrors. The mountmap fix for 'proc' → '/proc' should be squashed into the commit that caused it. - mypath=self.settings["chroot_path"] + #mypath=self.settings["chroot_path"] Just remove this entirely, don't comment it out. b2ea321 remove a named parameter in generate_hash() call due to a parameter name change Move “due to …” into the commit message body. I think keyword arguments are better, because changes to keywords usually occur alongside changes to the argument semantics. A keyword mismatch is an obvious fix, while changes due to a semantic shift can be more subtle. a847803 Use normpath from support Why are we not using os.path.normpath? Also, remove instead of commenting out. 6108400 chmod +x all sh scripts so they can run from the git checkout Looks good, 242c17d rename all target .py file and classes without the "_target" so they are the same as the .sh files and work with teh simplified module loading. Move “so they …” into the commit message body and s/teh/the/. I was going to suggest this if you hadn't gotten around to it ;). ecc653f fix targets.sh path Squash into the commit that caused it. d563be8 Comment out a small code block causing TypeError, which also was short circuiting another large code block. FIXME!!!! This whole class seems overly complicated with TOO MANY nested try:excepts: Long commit summary. I agree that a few of these methods should be split into helper functions for easier reading. e05cc34 Break out a few more repeated (path1 + path2)'s to doing it once and using the temp variable. comment out some debug print's Long commit summary. os.path.join! Also, convert debug prints to use the logging module ;). e0738c0 rename a make.conf key to make_conf due to bash variable name restrictions Squash into causing commit. db2af17 fix my broken subprocess call to use Popen due to environment passing and return code needs Squash into causing commit. ae61e4a reduce 2 operations into one simpler one Just switch to ConfigParser ;). b08a757 extend ParserBase to do variable substitution. ConfigParser does that to ;). 265c8ed add a "shdir" setting to make moving the bash code around easier and have less hardcoded paths in the bash scripts Long commit summary. Your comment claims catalyst runtime executables for both sharedir and shdir. dc90e40 migrate all target shell scripts to use the new shdir setting Squash into 265c8ed. 2fa8968 minor cleanup Squash into earlier (proposed) global whitespace cleanup and import cleanups. d7007a9 fix a bug in relative path use. Add some unique identifiers to echo statements to make identifying the source of a message easier. Split into two commits (relative path, unique identifiers). We should also try and figure out less arbitrary identifiers. 8b9a1c0 add embedded variable substitiution to default settings ConfigParser does this. 1259fa6 make shdir a complete path to ease it's use. Add port_config path setting to remove more hard coded paths in code. Squash shdir stuff into the earlier shdir addition. Squash port_config into wherever you use it… 923e8a2 remove trailing slash for consistency in variables and remove extra slashes in paths os.path.join() 55a58ee Add a forced debug print statement in cmd() for better debug output Python logging. 24d69bd Commit my testpath file with instructions to run the git checkout code directly without being installed. s/caatalyst/catalyst/ It would also probably be a good idea to add an example test.conf with relative paths. b7036f3 update gitignore As I said earlier, the test.* files your ignoring don't seem to exist in my checkout. 0fb2609 add archdir to settings Looks good. Cheers, Trevor [1]: http://www.python.org/dev/peps/pep-0257/#multi-line-docstrings [2]: http://docs.python.org/dev/library/importlib.html#importlib.import_module -- 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