1 |
On Thu, 2013-01-31 at 14:46 -0500, W. Trevor King wrote: |
2 |
> On Thu, Jan 31, 2013 at 01:39:22PM -0500, W. Trevor King wrote: |
3 |
> > I'll see if I can get through 968d818, … sometime later today. |
4 |
> |
5 |
> I had a second wind ;). Here are some comments on the remaining |
6 |
> commits: |
7 |
> |
8 |
> 968d818 Initial creation of a defaults file. Split out hash and |
9 |
> contents to their own classes, files |
10 |
> |
11 |
> After this, it's not clear to me what the difference is between |
12 |
> catalyst.support and catalyst.util. Perhaps they should be merged. |
13 |
|
14 |
I hadn't looked at util until now. |
15 |
|
16 |
hmm, I wonder if it would be better to move CatalystError there and |
17 |
rename it to error.py There is only a couple error traceback functions |
18 |
in it. I was thinking it might be good to define a few more specific |
19 |
error classes than just the general CatalystError(). But I am not yet |
20 |
familiar enough with the code to know for certain. |
21 |
|
22 |
> |
23 |
> I'd also use catalyst.targets.__all__ instead of coding a list of |
24 |
> targets in the apparently unrelated |
25 |
> catalyst.defaults.valid_build_targets. |
26 |
|
27 |
yeah, that's part of the old plugin system that was there. I've |
28 |
simplified it some, but it needs more work. That variable is pretty |
29 |
much useless now if I remember correctly. I was contemplating whether it |
30 |
would justify using the plugin system I did for emaint. It would be |
31 |
easily imported from portage. But I think it might be more than is |
32 |
needed for this. |
33 |
|
34 |
> |
35 |
> a4ef493 re-version to "git-rewrite branch" |
36 |
> |
37 |
> Why? |
38 |
> |
39 |
|
40 |
Why not. It is not intended to be pushed into master. It also helps to |
41 |
confirm that you are running the correct code. I have fixed it so the |
42 |
code can be run from the git checkout. That way you can have a |
43 |
"Production" version installed on the same system. Now that I am |
44 |
testing/debugging, I am comparing catalyst operation and results to |
45 |
current -9999 code. I am not familiar with using catalyst, so that is |
46 |
helping me figure out what is wrong. |
47 |
|
48 |
|
49 |
> 9d752a7 move confdefaults out of main.py |
50 |
> |
51 |
> Looks good, except, I'm not sure why you changed from |
52 |
> `confdefaults.keys()` to `list(confdefaults)` in parse_config() (which |
53 |
> should probably be living in catalyst.config anyway). |
54 |
|
55 |
keeping a separate defaults file can be helpful in importing some info |
56 |
into different modules while keeping imports to a minimum. Sometimes it |
57 |
helps prevents circular import problems. At this point I opted for a |
58 |
separate file, to be determined later if a merge is warranted. |
59 |
|
60 |
As for list(confdefaults), py3 compatibility. dict.keys() isn't usable |
61 |
and 2to3 converts it to list(dict)... something about needing to specify |
62 |
the return type. So is a preemptive change. One less thing to change |
63 |
later. |
64 |
|
65 |
> |
66 |
> c303dae some options cleanup, unifying their use, reducing redundancy. |
67 |
> |
68 |
> While I like the general thrust of this, I'd be happier with explicit |
69 |
> boolean options instead of a set of boolean options. For example: |
70 |
> |
71 |
> confdefaults = { |
72 |
> 'autoresume': False, |
73 |
> 'ccache': False, |
74 |
> … |
75 |
> } |
76 |
> |
77 |
|
78 |
Yeah, I removed those. They were capitalized versions of the values in |
79 |
options. So, I optimized them into options becoming a set which |
80 |
eliminates, the duplication and potential problems by changing the value |
81 |
of one and not the other. Believe me keeping 2 different lists in sync |
82 |
can be much more difficult than it seems. It also makes things much |
83 |
more difficult to debug. (the independent booleans like you suggest can |
84 |
be considered a list, the other is the options list, set, |
85 |
string...whatever form it is in) |
86 |
|
87 |
They are set in default's "options" and updated from a config files's |
88 |
"options". So keeping them in "options" makes more sense to me, I just |
89 |
convert them into a set to eliminate any duplicates. Member inclusion |
90 |
equates to True, while not being a member False. |
91 |
|
92 |
if "ccache" in settings["options"]: |
93 |
|
94 |
is just as easy to read as |
95 |
|
96 |
if settings["ccache"]: or if settings["ccache"] == True: ... |
97 |
|
98 |
also using member inclusion is faster and prefered compared to other |
99 |
methods like has_key(). In this case, it is just simpler to use |
100 |
"options" rather than to individualize them. |
101 |
|
102 |
|
103 |
|
104 |
> 6aeb5e3 Move LockInUse from support.py to lock.py, fix bad execption |
105 |
> raising, pyflakes cleanup |
106 |
> |
107 |
> With respect to the exception raising, see my comments on 63a25eb in |
108 |
> the previous email. Ahh, it looks like you finish that off with |
109 |
> 04068a1. |
110 |
> |
111 |
> 04068a1 massive pyflakes import cleanup and broken CatalystError calls |
112 |
> |
113 |
> A lot of this is great stuff (Hooray no `import *`!). |
114 |
|
115 |
Yeah, those import * were hiding a LOT of sins, turning catalyst into a |
116 |
huge global variable using app for almost everything. |
117 |
|
118 |
> However, I'd |
119 |
> split the `print_traceback=True` changes out into their own commit (or |
120 |
> just change the default value for CatalystError while you're testing). |
121 |
> |
122 |
|
123 |
I added that to try and limit traceback printing to areas that could |
124 |
benefit from it for debugging. Reducing noise in others. It still needs |
125 |
fine tuning. |
126 |
|
127 |
> It looks like an unrelated change to |
128 |
> catalyst.defaults.required_build_targets snuck into this commit. |
129 |
|
130 |
will look into it |
131 |
|
132 |
|
133 |
> 28888a7 remove redundant /bin/bash additions in cmd() calls |
134 |
> |
135 |
> Looks good. |
136 |
> |
137 |
> 97802b6 clean out the old spawn functions, use subprocess.call() |
138 |
> instead |
139 |
> |
140 |
> Hooray. Since you're doing this, I'd drop the earlier commits that |
141 |
> referenced this change (e.g. d23d96b and 1b2597e). |
142 |
> |
143 |
> 624eb0d move base stage and target files to thier own sub-pkg |
144 |
> |
145 |
> s/thier/their/ |
146 |
> |
147 |
> The diff looks good. |
148 |
> |
149 |
> 5c689a8 update module loading for the new python structure, rename |
150 |
> snapshot_target to snapshot |
151 |
> |
152 |
> I think I like this. It's good to use __import__. You're missing a |
153 |
> blank line after your import_module docstring summary [1]. |
154 |
> |
155 |
> I think that the import_module API should match Python 3's |
156 |
> importlib.import_module [2]. |
157 |
|
158 |
Yeah, there is more to do in this. There are other areas that need |
159 |
changing too. |
160 |
|
161 |
> |
162 |
> 669451d fix options being reset by a config file |
163 |
> |
164 |
> See my comments on c303dae. We should really be using ConfigParser's |
165 |
> defaults here, instead of rolling our own default system. |
166 |
> |
167 |
|
168 |
Yes, I agree. |
169 |
|
170 |
|
171 |
> Can we use logging instead of print? |
172 |
> |
173 |
|
174 |
YES!!!!, please :D |
175 |
|
176 |
that's been on my wish list too. |
177 |
|
178 |
|
179 |
> - mypath=self.settings["chroot_path"] |
180 |
> + #mypath=self.settings["chroot_path"] |
181 |
> |
182 |
> Just remove this entirely, don't comment it out. |
183 |
|
184 |
I sometimes do that if I'm unsure what I'm going to do, so makes it |
185 |
easier to revert and go a different direction/track some changes during |
186 |
debugging. Sometimes, I forget to remove it after I've settled on the |
187 |
change and done the testing. It will be cleaned up after debugging, |
188 |
when I make final clean commits to a new branch. |
189 |
|
190 |
|
191 |
> I think keyword arguments are better, because changes to keywords |
192 |
> usually occur alongside changes to the argument semantics. A keyword |
193 |
> mismatch is an obvious fix, while changes due to a semantic shift can |
194 |
> be more subtle. |
195 |
|
196 |
yeah, I was a bit frustrated at that point, debugging code, so chose the |
197 |
easy way. /me fixes. |
198 |
|
199 |
> |
200 |
> a847803 Use normpath from support |
201 |
> |
202 |
> Why are we not using os.path.normpath? |
203 |
> |
204 |
|
205 |
I haven't figured that one out either. os.path.normpath is used in |
206 |
places. |
207 |
|
208 |
|
209 |
|
210 |
> d7007a9 fix a bug in relative path use. Add some unique identifiers |
211 |
> to echo statements to make identifying the source of a message easier. |
212 |
> |
213 |
> Split into two commits (relative path, unique identifiers). We should |
214 |
> also try and figure out less arbitrary identifiers. |
215 |
> |
216 |
|
217 |
yeah, it does need splitting. It was easy at the time, this is |
218 |
something for the powers that be (releng team) to have some output |
219 |
over... It may also be something that gets cleaned once changes & |
220 |
debugging are done. |
221 |
|
222 |
|
223 |
|
224 |
> 923e8a2 remove trailing slash for consistency in variables and remove |
225 |
> extra slashes in paths |
226 |
> |
227 |
> os.path.join() |
228 |
|
229 |
Yes, for sure. But after I debug my current changes. But also most of |
230 |
those are for the bash side consistency which can not use os.path.join() |
231 |
and were adding the slashes again at times. |
232 |
|
233 |
I am not a bash programmer, so if someone good at the bash stuff wants |
234 |
to work on those... go for it. I'll try to keep my damage to a minimum. |
235 |
|
236 |
Also if releng want to recode them in python, that's ok with me ;) |
237 |
|
238 |
|
239 |
> 24d69bd Commit my testpath file with instructions to run the git |
240 |
> checkout code directly without being installed. |
241 |
> |
242 |
> s/caatalyst/catalyst/ |
243 |
> |
244 |
my bad typing, also is partially caused by a tremor in my hand :( |
245 |
|
246 |
> It would also probably be a good idea to add an example test.conf with |
247 |
> relative paths. |
248 |
> |
249 |
ok |
250 |
|
251 |
|
252 |
> Cheers, |
253 |
> Trevor |
254 |
> |
255 |
> [1]: http://www.python.org/dev/peps/pep-0257/#multi-line-docstrings |
256 |
> [2]: |
257 |
> http://docs.python.org/dev/library/importlib.html#importlib.import_module |
258 |
> |
259 |
|
260 |
Thank you for the review, it has been informative. And good to keep me |
261 |
from any blunders. |