From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id 10482E01492 for ; Fri, 19 Jul 2013 06:00:10 -0700 (PDT) Received: from azsmga002.ch.intel.com ([10.2.17.35]) by azsmga102.ch.intel.com with ESMTP; 19 Jul 2013 06:00:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.89,701,1367996400"; d="scan'208,217";a="270559746" Received: from calin-desktop.rb.intel.com (HELO [10.237.105.152]) ([10.237.105.152]) by AZSMGA002.ch.intel.com with ESMTP; 19 Jul 2013 06:00:05 -0700 Message-ID: <51E93917.20104@intel.com> Date: Fri, 19 Jul 2013 16:03:19 +0300 From: Calin Dragomir User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: "Damian, Alexandru" References: In-Reply-To: Cc: "webhob@yoctoproject.org" Subject: Re: [Webhob] in-depth refactoring of database code X-BeenThere: webhob@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jul 2013 13:00:13 -0000 Content-Type: multipart/alternative; boundary="------------050301070900060208070205" --------------050301070900060208070205 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 --------------050301070900060208070205 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
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 

--------------050301070900060208070205--