Re: [PATCH 0/5] Add a standard module for accessing the layerindex

[Paul Eggleton <paul.eggleton@linux.intel.com>]

Hi Mark

On Thursday, 12 July 2018 11:07:06 PM CEST Mark Hatle wrote:
> On 7/12/18 3:34 PM, Mark Hatle wrote:
> > In order to simply existing components, and add support to create some
> > new functionaly -- we need a common apporach for access the layerindex.
> > 
> > The class supports loading multilib layerindexes, but right now that
> > functionality is not being used by either bitbake-layers or the toaster.
> > 
> > There are a few 'TODO' items that remain in the code.  These are related
> > to either un-implemented, but planned functionality or to display stuff
> > in bitbake-layers.  I'm hoping that part of this review can discuss the
> > TODO items.

This looks good, thanks - great to start to have a client API that we can use 
in a bunch of different places. Some comments:

1) LAYERSERIES_CORENAMES is treated by bitbake as a list. At the moment it's 
just one item, but we should treat it the same here - for now we can probably 
take the easiest way out and just split it and take the first item.

2) I feel like the ;type= stuff is an unnecessary complication. Can we not 
just treat http URLs as being restapi and local:// (or something similarly 
simple) as being the local configuration? The plugins could simply have a 
function that returns True if it can handle the URL, as we do in bb.fetch2, 
and we take the first one that returns True or error out if none do.

3) "except Exception" is too broad, can we catch a specific exception class or 
classes instead?

4) From an API perspective, it seems to me that we should be taking lists 
everywhere rather than space-delimited strings e.g. in get_dependencies() for 
the names parameter, particularly given the other parameters are actual lists.

5) I'd like to see the data classes (e.g. Recipe) have actual python 
properties on top of the getter functions, and the rest of the code should 
then use the properties. recipe.pn is a bit neater than recipe.get_pn() 
particularly as the former mirrors usage within the layer index code itself as 
well as tinfoil's TinfoilRecipeInfo (although TinfoilRecipeInfo doesn't 
actually use properties - it probably should - but the usage syntax is the key 
bit).

6) There are a number of different instances of variables "lindex" and they 
are different types. There's even "for lindex in self.lindex". This is a bit 
confusing.

7) What's loadRecipes = 1 for above load_layerindex() ?

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre