[bitbake-devel] [PATCH 0/5] Add a standard module for accessing the layerindex

[Mark Hatle]

On 7/17/18 3:37 PM, Paul Eggleton wrote:
> 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.

That was actually the intent, to treat it as a list.  It must have fallen away
in a refactor, I'll work to get this fixed.

> 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.

Behind the scenes the issue is that I expect there to be different plugins that
allow data manipulation in similar ways, but with different processing.  I was
afraid of overriding the URI part, thus added the type field instead.

If we can come up with a proper format for this, then I think we could drop the
typing..  but what I've come up with so far is that we need:

Cooker - data from the local cooker instance

restapi-web - knowledge of how to load and parse layerindex-web data

restapi-file - a json formatted file with all of the data.. (Note: in this case,
it can be one or more files, but everything has to be loaded together)

(the web and file stuff are co-mingled right now, maybe that is a mistake...
they were put together since for 'offline' work, the web should be able to use a
cached file...)

Looking at toaster, I could see the need for a 'database' way to store and load
it... but I'm not sure that is a good longer term idea.

but I'm open to suggestion there to remove the type stuff..

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

This is a mistake in the implementation, the exceptions should have been:
LayerIndexError exceptions.  (Defined in common.py)

> 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.

Ok, I will update that.

> 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).

I'm not sure I understand.

You mean something like:

class Recipe(LayerIndexItem_LayerBranch):
    ...

    self.pn = value

so you can do recipe.pn vs recipe.pn() or get_pn()?

The reason it's setup the way it is, is to preserve the data in the same format
as the layerindex itself.  This allows someone to just dump the recipe.data
directly, and everything stays in format.   Does python have a way to return
processed data without it looking like a function call, i.e. with the ()?

Is this what __getattr()__ is for?

> 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.

I was trying to not call everything 'layerindex' every time I used it.. :|  Thus
lindex..

self.lindex is the list of layerindex objects BTW.  The layerindexer can handle
more then one collection of objects... (multiple layer indexes specifically).

so the for lindex in self.lindex...  is for each index in the list of indexes,
do...  I'll see what I can do about renaming this stuff.

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

Errant paste.. no usage in this.  I'll remove it.

> Cheers,
> Paul
>