Re: [gentoo-catalyst] More proposed Catalyst changes

[Brian Dolbec <dolsen@gentoo.org>]

On Thu, 2013-01-31 at 14:46 -0500, W. Trevor King wrote:
> 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 hadn't looked at util until now.

hmm, I wonder if it would be better to move CatalystError there and
rename it to error.py  There is only a couple error traceback functions
in it.  I was thinking it might be good to define a few more specific
error classes than just the general CatalystError().  But I am not yet
familiar enough with the code to know for certain.

> 
> I'd also use catalyst.targets.__all__ instead of coding a list of
> targets in the apparently unrelated
> catalyst.defaults.valid_build_targets.

yeah, that's part of the old plugin system that was there.  I've
simplified it some, but it needs more work.  That variable is pretty
much useless now if I remember correctly. I was contemplating whether it
would justify using the plugin system I did for emaint.  It would be
easily imported from portage.  But I think it might be more than is
needed for this.

> 
> a4ef493 re-version to "git-rewrite branch"
> 
> Why?
> 

Why not.  It is not intended to be pushed into master.  It also helps to
confirm that you are running the correct code.  I have fixed it so the
code can be run from the git checkout.  That way you can have a
"Production" version installed on the same system.  Now that I am
testing/debugging, I am comparing catalyst operation and results to
current -9999 code.  I am not familiar with using catalyst, so that is
helping me figure out what is wrong.


> 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).

keeping a separate defaults file can be helpful in importing some info
into different modules while keeping imports to a minimum.  Sometimes it
helps prevents circular import problems.  At this point I opted for a
separate file, to be determined later if a merge is warranted.

As for list(confdefaults), py3 compatibility.  dict.keys() isn't usable
and 2to3 converts it to list(dict)... something about needing to specify
the return type.  So is a preemptive change.   One less thing to change
later.

> 
> 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,
>     …
> 		}
> 

Yeah, I removed those.  They were capitalized versions of the values in
options.  So, I optimized them into options becoming a set which
eliminates, the duplication and potential problems by changing the value
of one and not the other.  Believe me keeping 2 different lists in sync
can be much more difficult than it seems.  It also makes things much
more difficult to debug.  (the independent booleans like you suggest can
be considered a list, the other is the options list, set,
string...whatever form it is in)  

They are set in default's "options" and updated from a config files's
"options".  So keeping them in "options" makes more sense to me, I just
convert them into a set to eliminate any duplicates.  Member inclusion
equates to True, while not being a member False.

if "ccache" in settings["options"]:

is just as easy to read as

if settings["ccache"]:  or if settings["ccache"] == True: ...

also using member inclusion is faster and prefered compared to other
methods like has_key().  In this case, it is just simpler to use
"options" rather than to individualize them.



> 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 *`!).  

Yeah, those import * were hiding a LOT of sins, turning catalyst into a
huge global variable using app for almost everything. 

> 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).
> 

I added that to try and limit traceback printing to areas that could
benefit from it for debugging. Reducing noise in others.  It still needs
fine tuning.

> It looks like an unrelated change to
> catalyst.defaults.required_build_targets snuck into this commit.

will look into it


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

Yeah, there is more to do in this.  There are other areas that need
changing too.

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

Yes, I agree.


> Can we use logging instead of print?
> 

YES!!!!,  please :D

that's been on my wish list too. 


> -               mypath=self.settings["chroot_path"]
> +               #mypath=self.settings["chroot_path"]
> 
> Just remove this entirely, don't comment it out.

I sometimes do that if I'm unsure what I'm going to do, so makes it
easier to revert and go a different direction/track some changes during
debugging.  Sometimes, I forget to remove it after I've settled on the
change and done the testing.  It will be cleaned up after debugging,
when I make final clean commits to a new branch.


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

yeah, I was a bit frustrated at that point, debugging code, so chose the
easy way.  /me fixes.

> 
> a847803 Use normpath from support
> 
> Why are we not using os.path.normpath?
> 

I haven't figured that one out either.  os.path.normpath is used in
places.



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

yeah, it does need splitting.  It was easy at the time, this is
something for the powers that be (releng team) to have some output
over... It may also be something that gets cleaned once changes &
debugging are done.



> 923e8a2 remove trailing slash for consistency in variables and remove
> extra slashes in paths
> 
> os.path.join()

Yes, for sure.  But after I debug my current changes.  But also most of
those are for the bash side consistency which can not use os.path.join()
and were adding the slashes again at times.

I am not a bash programmer, so if someone good at the bash stuff wants
to work on those... go for it.  I'll try to keep my damage to a minimum.

Also if releng want to recode them in python, that's ok with me ;)


> 24d69bd Commit my testpath file with instructions to run the git
> checkout code directly without being installed.
> 
> s/caatalyst/catalyst/
> 
my bad typing, also is partially caused by a tremor in my hand :(

> It would also probably be a good idea to add an example test.conf with
> relative paths.
> 
ok


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

Thank you for the review, it has been informative.  And good to keep me
from any blunders.