Re: [gentoo-catalyst] More proposed Catalyst changes

[W. Trevor King]

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