From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id D7776138804 for ; Sat, 2 Feb 2013 20:41:38 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 15CCDE0330; Sat, 2 Feb 2013 20:41:37 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 7A5CEE0330 for ; Sat, 2 Feb 2013 20:41:36 +0000 (UTC) Received: from [192.168.1.210] (unknown [24.86.176.233]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 62B0833DBDD for ; Sat, 2 Feb 2013 20:41:35 +0000 (UTC) Message-ID: <1359837692.3997.132.camel@big_daddy.dol-sen.ca> Subject: Re: [gentoo-catalyst] More proposed Catalyst changes From: Brian Dolbec To: gentoo-catalyst@lists.gentoo.org Date: Sat, 02 Feb 2013 12:41:32 -0800 In-Reply-To: <20130131194653.GA4540@odin.tremily.us> References: <1357633976.4289.61.camel@big_daddy.dol-sen.ca> <1357980958.4289.82.camel@big_daddy.dol-sen.ca> <20130131183922.GA3946@odin.tremily.us> <20130131194653.GA4540@odin.tremily.us> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.6.3 Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Archives-Salt: 0e42e2b7-ba10-43c6-b74d-0a06c0c6860e X-Archives-Hash: 3930ae52d5d75b2948a5f921d968f224 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.