On Mon, 23 Jan 2006 14:08:00 -0800 Brian Harring <[EMAIL PROTECTED]> wrote:
> On Mon, Jan 23, 2006 at 07:44:44PM +0100, Marius Mauch wrote:
> > On Mon, 23 Jan 2006 03:47:11 -0800
> > Can't follow your thinking here. As said, the code won't corrupt any
> > data, at worst it will tell the user that it couldn't extract some
> > keys (and even that only if there would be such a stupid case which
> > itself has a chance of like 10^-10 or so, or can you name a single
> > case?).
>
> Yeah, any extension of your code to pick up EAPI is going to false
> positive on somewhere around a quarter of my vdb nodes- I use a
> debug function from my bashrc for prefix testing (holdover from eapi
> testing days). Why? Because the eapi var didn't exist (thus no env
> assignment), the only possible match is in the middle of a here op-
>
> debug_dump() {
> ...
> echo <<<HERE >&2
> EAPI="${EAPI-unset}"
> PF="$PF"
> CATEGORY="$CATEGORY"
> HERE
> ...
> }
>
> Setting EAPI to unset strikes me as corruption here, because now
> portage will *never* do anything with that vdb node- the eapi differs
> from what it supports (it's been set to effectively a random value),
> thus I have to fix the vdb entry myself if I ever want to get rid of
> it.
>
> Guessing the response on that one is "well, you're using the bashrc",
> and yes, I am- the vdb code should be *robust*, not choking on a
> users debug code.
Not even trying to comment on this one.
> > So what package contains filter-env then?
>
> Portage- been in cvs/svn for over a year now :)
Not in trunk or any release (except the alpha snapshot). And I'm not
going to blindly copy unknown code from completely different codebases
and later be blamed for doing it improperly.
> > > Aside from that, if the code is in debate (as this is), I really
> > > don't think it should get slid into svn 2 weeks later effectively
> > > unchanged- didn't write that original email just for the hell of
> > > it :)
> >
> > As said, I disagree with your assessment of the situation. If you
> > can name a single case where the code "breaks" or filter-env hits
> > the tree I might reconsider, but not before.
>
> Well, if you disagreed with the original response, continue the
> conversation prior to commiting- otherwise we see a commit, then a
> rebuttal a few hours later. Not really how things should go for a
> contested piece of code (at least when the only two to weigh in our
> flat out opposed on it)- especially if the code's effect is
> nontrivial and it hasn't had any actual peer review (only comment was
> on your algo).
Interesting, you now count for two people? Or who is this second person
you're talking of? I still think you're making more out of this than it
really is. Also there really isn't a point in discussing a difference in
opinion IMO, such attempts lead nowhere. I considered your comment, but
couldn't come up with a reason why a theoretical issue should hold this
up.
> Issues with the code.
Now we're getting somewhere.
> 1) Scans for metadata that didn't exist at the time of the ebuild's
> installation, only possible match is unintended. Not good.
Not really sure what that's supposed to mean, assuming you mean that
nodes will lack SRC_URI/HOMEPAGE/... from env.bz2 is about as likely as
here doc statements containing them (and indicates a broken package
anyway).
> 2) your code is breaking upon *all* newlines, regardless if it's
> in the middle of a variable assignment. Yes, bash does use $'' for
> assignment, but you're relying (hoping really) that the variable has
> been filtered by python portage and the ebuild defined var is
> stomped. This is a bad idea- description comes to mind. Your code
> seems to be aware of this possibility also, since it's doing rather
> loose quote filtering.
Have to think about this.
> 3) Stop using regex all over the place. split/join is a helluva lot
> faster, and collapses that nasty regex that attempts to remove
> whitespace down to two lines (a one time translate, and the
> split/join to kill whitespace).
Ok (cosmetics).
> 4) Code specifically tries to find, and remove variable chars-
> this is *really* the wrong thing to do. If you're trying to work
> around catching a var reference, either your parsing screwed up, or
> you're mangling a DESCRIPTION value that you shouldn't be hosing.
define "variable chars", otherwise doesn't make sense.
> 5) hardcoded vdb.
> 6) Non root aware VDB.
Ok (trivial)
> 7) Only is aware of environment.bz2- older portage versions differed
> in this afaik (I *do* know PORT_ENV_FILE overloading for ebd I had to
> address this case already, so it's out there).
Well, it's present as long as I can remember, but an sanity check
wouldn't hurt there.
> 8) perms on the new files?
Ok.
> 9) general code comments; 'if s != ""', use 'if s:'. Catch the
> exceptions for check mode, don't let vdbkeyhandler nuke world file
> checking due to a potential exception. Right now, your code will TB
> if ran by non-root for a check run rather then stating "got to be
> root".
a) hate theses boolean optimizations, decrease code readability IMO.
b) Ok.
> In general, in looking through the parsing here there is a *lot* of
> chunks that look questionable- the scan for a var assignment for
> example, should *not* be stripping quotes on it's own. The only
> benefit of that stripping is allowing for false positives (hardly a
> benefit).
Hmm, sure there was a reason for that, but can't find that anymore.
> Aside from code commentary, the real brass tacks here is that this
> code if used is going to be expanded for more then the trivial vars
> it handles now- in other words, this problematic parser will wind up
> being used months down the line for important vars with intrinsic
> issues still unaddressed.
Any new auxdbkeys should be stored in vdb nodes from the beginning, so
that only leaves EAPI. Or do you plan to convert existing envvars to
auxdbkeys?
> And yes, what I'm pointing at is a corner case- that doesn't mean
> we cut corners on the solution *especially* when a proper tool
> exists for this already. No tool? I'd be a bit quieter, but the
> issues *are* resolved already, you just need to ditch the adhoc regex
> attempt (or write your own state aware parser). It's a 20 minutes
> mod to fix my initial complaints on this- it might seem pointless,
> but the potential is there as is a fix, so address it.
Simply port the tool to trunk or put it into the tree (which is
probably preferrable long term) and we can be done with this.
I'm just hesistant to more or less take maintainership of that code.
Btw, wasn't it you who said that you'd rather want to fix issues in
committed code than sending patches back and forth till there is a
perfect solution?
Marius
--
Public Key at http://www.genone.de/info/gpg-key.pub
In the beginning, there was nothing. And God said, 'Let there be
Light.' And there was still nothing, but you could see a bit better.
signature.asc
Description: PGP signature
