Re: [PATCH 1/2] data_smart.py: track file inclusion and variable modifications

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Thu, 2012-08-16 at 11:39 -0500, Peter Seebach wrote:
> On Thu, 16 Aug 2012 15:32:44 +0100
> Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> > On Wed, 2012-08-15 at 20:14 -0500, Peter Seebach wrote:
> 
> > > 1.  We create a history dict in the data_smart object, the
> > > members of which are lists of tuples.  It's three, count them,
> > > THREE unrelated data types in a single object!
>  
> > Do we really need comments like "count them" in the commit message?
> 
> Probably not. That commit message dates to the end of a very long
> day. It could be altered without much harm.

Fair enough, I think we do need the final commit message to avoid things
like that though :)

> 
> > > The special file name 'Ignore' indicates that an event need
> > > not be logged.
>  
> > This isn't particularly pythonic but I'm not going to lose sleep over
> > it.
> 
> I am very open to the idea of a better idiom, I just couldn't think of
> a good way to make some suitable value visible. (Actually, I suspect
> the right answer is some kind of named-arguments thing, but I don't
> know off the top of my head whether that can be adapted here.)

I think I'm leaning in favour of an internal _setVar() call for these...

> > > +# These are used in dataSmart, here as protection against
> > > KeyErrors. +def enableTracking():
> > > +    pass
> > > +
> > > +def disableTracking():
> > > +    pass
> > > +
> 
> > Where would these get called from that would trigger a keyerror?
> 
> I honestly don't know. I believe that it happened to me once, but I
> can't imagine why. This is from the first pass of the code, and could
> well be not particularly useful or correct; I did not have a good
> mental map of the relationship between data and data_smart. Come to
> think of it, I probably still don't.
>  
> > > -def setVar(var, value, d):
> > > +def setVar(var, value, d, filename = None, lineno = None):
> > >      """Set a variable to a given value"""
> > > -    d.setVar(var, value)
> > > +    filename, lineno = d.infer_file_and_line(filename, lineno)
> > > +    d.setVar(var, value, filename, lineno)
> > >  
> 
> > Changing any of these in data.py looks like a pointless waste of time.
> > The parser should be calling the object (dataSmart) variants and that
> > is now the preferred mechanism. If some old user does call here, the
> > parameters will be unset and the dataSmart setVar will call
> > infer_file_and_line for us anyway.
> 
> Yes, but in that case it would just report the line inside
> data.py/setVar, which is not as useful. Hmm. Well, that is an easy
> thing to check on!
> 
> Answer: In a bitbake -e (no target), there are 14 instances of hits
> on these functions. All of which come from:
> 
>     # DONE WITH PARSING... time to evaluate
>     if ext != ".bbclass":
>         data.setVar('FILE', abs_fn, d)
> 
> in BBHandler.py.
> 
> So I'd be totally happy with dropping all the data.py changes and
> instead fixing BBHandler.


Right, lets fix BBHandler.

> > > +    def includeLog(self, filename):
> > > +        """includeLog(included_file) shows that the file was
> > > included
> > > +        by the currently-processed file or context."""
> > > +        if self._tracking_enabled:
> > > +            event = (filename, [])
> > > +            position = (len(self.include_stack[-1][1]), event[1])
> > > +            self.include_stack[-1][1].append(event)
> > > +            self.include_stack.append(position)
> > > +
> > > +    def includeLogDone(self, filename):
> > > +        if self._tracking_enabled:
> > > +            if len(self.include_stack) > 1:
> > > +                self.include_stack.pop()
> > > +            else:
> > > +                bb.warn("Uh-oh:  includeLogDone(%s) tried to empty
> > > the stack." % filename)
> 
> > There has to be a better way to write code to do this. This looks
> > horrible.
> 
> I agree, but I wasn't able to come up with one.

I'll try and see if I can come up with something when I get a free
moment. I spent a short while yesterday thinking about this but I need
more time on it.

> > > +    def setVar(self, var, value, filename = None, lineno = None,
> > > op = 'set', details = None):
> 
> > As a function prototype, this scares me and shouts that there is
> > something going wrong here. The conversion of lineno to details and
> > back under a variety of different circumstances doesn't seem
> > particularly elegant or easy to understand.
> 
> Yes. At a bare minimum, it should probably be done with, say, a dict or
> something as a single additional/optional argument.
> 
> > Also, I want to hightlight that these changes effectively set our data
> > API in stone, making it effectively impossible to ever add sensible
> > external facing API changes. We should really change all these calls
> > to be named parameters so it doesn't totally freeze the API. I hate
> > having to do that but I hate the alternatives more.
> 
> Hmm. What if it were
>    def setVar(self, var, value, logging = None)
> 
> And then logging could be, say, { filename = ..., lineno = ..., op
> = ..., deteails = ... } where each component is optional with sane
> defaults.
> 
> I think that solves a LOT of the problems. It's a single optional
> parameter, with named components, and it avoids the addition of four
> separate arguments to do a single thing.

If you already know this please ignore me but what I meant by named
parameter was that you can make a call to a function like this:

d.setVar(x, y, filename=a, lineno=b)

which means when you can later extend the function without all users
needing to specify filename and lineno. This would avoid some of the API
lockin problems I worry about since we could make logging a named
parameter which is then effectively internal to data_smart, yet still
add external API without needing to specify it.

For the sake of readability, I might want to truncate it to "log" if
we're specifying it in many places. I do like the idea of collapsing the
info into one parameter rather than many.

> > So I do like the intent of this series but I don't like the impact on
> > the code base and think it needs some work to improve readability at
> > the very least.
> 
> Okay. I will do another pass.
> 
> BTW, if anyone can think of a clearer way to express that includelog
> history, I'd love to hear about it; I spent a while trying to think of
> something and came up blank.

I will see if I can find a few minutes to give that some thought...

Cheers,

Richard