Re: [Webhob] in-depth refactoring of database code

[Calin Dragomir <calinx.l.dragomir@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5137 bytes --]

Hello Alex,

Thank you for these updates, the code looks really solid !

Here are a couple of suggestions I have for these change-sets:

For the commit: *bitbake: runqueue, build, dsi: event data change**
***
> +            task.save()
> +
> +        if isinstance(event, bb.build.TaskBase):
> +            task.recipe.name = event._package
> +            task.recipe.save()

The first save() can go away, as there is another task.save() call a few 
lines down.

-----

Would it be better to change this:
     self._message = "recipe %s: task %s: %s" % (d.getVar("PF", True), 
t, self.getDisplayName())
into this:
     self._message = "recipe %s: task %s: %s" % (self._package, t, 
self.getDisplayName())  ?

------

     identifier = task_information['recipe'].file_path + 
task_information['task_name']
This may be a very long key. Maybe we can do something like this:
     identifier = task_information['recipe'].file_path.split('/')[-1] + 
task_information['task_name']
This will store only the recipe name+version together with the task_name

If this point is taken, this line:
     identifier = event.taskfile + event.taskname
Should become:
identifier = event.taskfile.split('/')[-1] + event.taskname

! Beware of other lines that may need to change together with this.

=====================================================

For commit:***bitbake: dsi: refactor the BuildInfoHelper code*

-        return machine_info
+        pass

      def create_machine_object(self, machine_information):

Do we need this 'pass' here ?

---

+            task_object.outcome=task_information['outcome']
+
+ task_object.task_executed=task_information['task_executed']

Can we delete the space between these lines ? It makes the first line 
stand out from the rest and there's no reason for that.

---

+            import traceback

It's best to have all imports at the beginning of the file.

-----

Get task object and get recipe object are not consistent with eachother.
get_recipe should consider the created attribute before trying to store 
information:

+        try:
+            recipe_object.name=recipe_information['name']
+            recipe_object.version=recipe_information['version']
+            recipe_object.summary=recipe_information['summary']
+ recipe_object.description=recipe_information['description']
+            recipe_object.section=recipe_information['section']
+            recipe_object.license=recipe_information['license']
+ recipe_object.licensing_info=recipe_information['licensing_info']
+            recipe_object.homepage=recipe_information['homepage']
+ recipe_object.bugtracker=recipe_information['bugtracker']
+            recipe_object.author=recipe_information['author']
+        except:
+            pass
+        finally:
+            recipe_object.save()

And also, if we put this in a try except block, maybe it is best to 
print the traceback then let it go.
We might consider to use a try/except block to the get_task method.

---------

+        for bl in sorted(self.internal_state['build_layers'], 
reverse=True, key=_slkey):
+            if (path.startswith(bl.layer.local_path)):
+                return bl

Should we add a comment here with what's the motivation behind this ?

--------------

+        identifier = event.taskfile + event.taskname

This also needs to change if we consider my advice on using smaller 
identifier values, from above.

--------------

+        if isinstance(event, bb.runqueue.runQueueTaskCompleted):
+            task_information['outcome'] = 3     # TODO: needs to use 
constants

The Django model updates from this patch states the default for this is 
value 4.
Either this or that need to change for consistency reasons.

--------------

+            e = event
+            e.taskname = pn

Do we need to do this?  I think event instead of e makes the code more 
clear.

------------

+            task_obj = self.orm_wrapper.update_task_object(task_info)

Caution: This method doesn't return anything, so it may be better just 
to call it, instead of assigning it to an attribute:
         self.orm_wrapper.update_task_object(task_info)

-------------


That's about it :)
I hope this helps!

Thank you,
Calin


On 19.07.2013 01:35, Damian, Alexandru wrote:
> Hi guys,
>
> I've performed a deep refactoring of the database interface code.
> This was triggered by the opportunity to use dependency information 
> which now we dump from Bitbake to pre-load recipe and task information 
> prior to actual running.
>
> This is fairly deep, as I moved a lot of code around. Because of this, 
> I would like a round of review before merging in the webhob-master. As 
> such, the code has been pushed to:
>
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=webhob-poky/master-next
> contrib/webhob-poky/master-next
>
> Can you please check this out and let me know if you find something 
> spotty ?
>
> Cheers,
> Alex
>
>
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC


[-- Attachment #2: Type: text/html, Size: 9594 bytes --]