1 |
On Mon, 23 Jan 2006 14:08:00 -0800 |
2 |
Brian Harring <ferringb@g.o> wrote: |
3 |
|
4 |
> On Mon, Jan 23, 2006 at 07:44:44PM +0100, Marius Mauch wrote: |
5 |
> > On Mon, 23 Jan 2006 03:47:11 -0800 |
6 |
> > Can't follow your thinking here. As said, the code won't corrupt any |
7 |
> > data, at worst it will tell the user that it couldn't extract some |
8 |
> > keys (and even that only if there would be such a stupid case which |
9 |
> > itself has a chance of like 10^-10 or so, or can you name a single |
10 |
> > case?). |
11 |
> |
12 |
> Yeah, any extension of your code to pick up EAPI is going to false |
13 |
> positive on somewhere around a quarter of my vdb nodes- I use a |
14 |
> debug function from my bashrc for prefix testing (holdover from eapi |
15 |
> testing days). Why? Because the eapi var didn't exist (thus no env |
16 |
> assignment), the only possible match is in the middle of a here op- |
17 |
> |
18 |
> debug_dump() { |
19 |
> ... |
20 |
> echo <<<HERE >&2 |
21 |
> EAPI="${EAPI-unset}" |
22 |
> PF="$PF" |
23 |
> CATEGORY="$CATEGORY" |
24 |
> HERE |
25 |
> ... |
26 |
> } |
27 |
> |
28 |
> Setting EAPI to unset strikes me as corruption here, because now |
29 |
> portage will *never* do anything with that vdb node- the eapi differs |
30 |
> from what it supports (it's been set to effectively a random value), |
31 |
> thus I have to fix the vdb entry myself if I ever want to get rid of |
32 |
> it. |
33 |
> |
34 |
> Guessing the response on that one is "well, you're using the bashrc", |
35 |
> and yes, I am- the vdb code should be *robust*, not choking on a |
36 |
> users debug code. |
37 |
|
38 |
Not even trying to comment on this one. |
39 |
|
40 |
> > So what package contains filter-env then? |
41 |
> |
42 |
> Portage- been in cvs/svn for over a year now :) |
43 |
|
44 |
Not in trunk or any release (except the alpha snapshot). And I'm not |
45 |
going to blindly copy unknown code from completely different codebases |
46 |
and later be blamed for doing it improperly. |
47 |
|
48 |
> > > Aside from that, if the code is in debate (as this is), I really |
49 |
> > > don't think it should get slid into svn 2 weeks later effectively |
50 |
> > > unchanged- didn't write that original email just for the hell of |
51 |
> > > it :) |
52 |
> > |
53 |
> > As said, I disagree with your assessment of the situation. If you |
54 |
> > can name a single case where the code "breaks" or filter-env hits |
55 |
> > the tree I might reconsider, but not before. |
56 |
> |
57 |
> Well, if you disagreed with the original response, continue the |
58 |
> conversation prior to commiting- otherwise we see a commit, then a |
59 |
> rebuttal a few hours later. Not really how things should go for a |
60 |
> contested piece of code (at least when the only two to weigh in our |
61 |
> flat out opposed on it)- especially if the code's effect is |
62 |
> nontrivial and it hasn't had any actual peer review (only comment was |
63 |
> on your algo). |
64 |
|
65 |
Interesting, you now count for two people? Or who is this second person |
66 |
you're talking of? I still think you're making more out of this than it |
67 |
really is. Also there really isn't a point in discussing a difference in |
68 |
opinion IMO, such attempts lead nowhere. I considered your comment, but |
69 |
couldn't come up with a reason why a theoretical issue should hold this |
70 |
up. |
71 |
|
72 |
> Issues with the code. |
73 |
|
74 |
Now we're getting somewhere. |
75 |
|
76 |
> 1) Scans for metadata that didn't exist at the time of the ebuild's |
77 |
> installation, only possible match is unintended. Not good. |
78 |
|
79 |
Not really sure what that's supposed to mean, assuming you mean that |
80 |
nodes will lack SRC_URI/HOMEPAGE/... from env.bz2 is about as likely as |
81 |
here doc statements containing them (and indicates a broken package |
82 |
anyway). |
83 |
|
84 |
> 2) your code is breaking upon *all* newlines, regardless if it's |
85 |
> in the middle of a variable assignment. Yes, bash does use $'' for |
86 |
> assignment, but you're relying (hoping really) that the variable has |
87 |
> been filtered by python portage and the ebuild defined var is |
88 |
> stomped. This is a bad idea- description comes to mind. Your code |
89 |
> seems to be aware of this possibility also, since it's doing rather |
90 |
> loose quote filtering. |
91 |
|
92 |
Have to think about this. |
93 |
|
94 |
> 3) Stop using regex all over the place. split/join is a helluva lot |
95 |
> faster, and collapses that nasty regex that attempts to remove |
96 |
> whitespace down to two lines (a one time translate, and the |
97 |
> split/join to kill whitespace). |
98 |
|
99 |
Ok (cosmetics). |
100 |
|
101 |
> 4) Code specifically tries to find, and remove variable chars- |
102 |
> this is *really* the wrong thing to do. If you're trying to work |
103 |
> around catching a var reference, either your parsing screwed up, or |
104 |
> you're mangling a DESCRIPTION value that you shouldn't be hosing. |
105 |
|
106 |
define "variable chars", otherwise doesn't make sense. |
107 |
|
108 |
> 5) hardcoded vdb. |
109 |
> 6) Non root aware VDB. |
110 |
|
111 |
Ok (trivial) |
112 |
|
113 |
> 7) Only is aware of environment.bz2- older portage versions differed |
114 |
> in this afaik (I *do* know PORT_ENV_FILE overloading for ebd I had to |
115 |
> address this case already, so it's out there). |
116 |
|
117 |
Well, it's present as long as I can remember, but an sanity check |
118 |
wouldn't hurt there. |
119 |
|
120 |
> 8) perms on the new files? |
121 |
|
122 |
Ok. |
123 |
|
124 |
> 9) general code comments; 'if s != ""', use 'if s:'. Catch the |
125 |
> exceptions for check mode, don't let vdbkeyhandler nuke world file |
126 |
> checking due to a potential exception. Right now, your code will TB |
127 |
> if ran by non-root for a check run rather then stating "got to be |
128 |
> root". |
129 |
|
130 |
a) hate theses boolean optimizations, decrease code readability IMO. |
131 |
b) Ok. |
132 |
|
133 |
> In general, in looking through the parsing here there is a *lot* of |
134 |
> chunks that look questionable- the scan for a var assignment for |
135 |
> example, should *not* be stripping quotes on it's own. The only |
136 |
> benefit of that stripping is allowing for false positives (hardly a |
137 |
> benefit). |
138 |
|
139 |
Hmm, sure there was a reason for that, but can't find that anymore. |
140 |
|
141 |
> Aside from code commentary, the real brass tacks here is that this |
142 |
> code if used is going to be expanded for more then the trivial vars |
143 |
> it handles now- in other words, this problematic parser will wind up |
144 |
> being used months down the line for important vars with intrinsic |
145 |
> issues still unaddressed. |
146 |
|
147 |
Any new auxdbkeys should be stored in vdb nodes from the beginning, so |
148 |
that only leaves EAPI. Or do you plan to convert existing envvars to |
149 |
auxdbkeys? |
150 |
|
151 |
> And yes, what I'm pointing at is a corner case- that doesn't mean |
152 |
> we cut corners on the solution *especially* when a proper tool |
153 |
> exists for this already. No tool? I'd be a bit quieter, but the |
154 |
> issues *are* resolved already, you just need to ditch the adhoc regex |
155 |
> attempt (or write your own state aware parser). It's a 20 minutes |
156 |
> mod to fix my initial complaints on this- it might seem pointless, |
157 |
> but the potential is there as is a fix, so address it. |
158 |
|
159 |
Simply port the tool to trunk or put it into the tree (which is |
160 |
probably preferrable long term) and we can be done with this. |
161 |
I'm just hesistant to more or less take maintainership of that code. |
162 |
|
163 |
Btw, wasn't it you who said that you'd rather want to fix issues in |
164 |
committed code than sending patches back and forth till there is a |
165 |
perfect solution? |
166 |
|
167 |
Marius |
168 |
|
169 |
-- |
170 |
Public Key at http://www.genone.de/info/gpg-key.pub |
171 |
|
172 |
In the beginning, there was nothing. And God said, 'Let there be |
173 |
Light.' And there was still nothing, but you could see a bit better. |