* Re: [Webhob] in-depth refactoring of database code
2013-07-18 22:35 [Webhob] in-depth refactoring of database code Damian, Alexandru
@ 2013-07-19 13:03 ` Calin Dragomir
0 siblings, 0 replies; 2+ messages in thread
From: Calin Dragomir @ 2013-07-19 13:03 UTC (permalink / raw)
To: Damian, Alexandru; +Cc: webhob@yoctoproject.org
[-- 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 --]
^ permalink raw reply [flat|nested] 2+ messages in thread