[Toaster] [review-request] adamian/20150707_bugs
Damian, Alexandru
alexandru.damian at intel.com
Tue Jul 21 05:44:50 PDT 2015
On Tue, Jul 21, 2015 at 1:39 PM, Michael Wood <michael.g.wood at intel.com>
wrote:
> On 21/07/15 12:26, Damian, Alexandru wrote:
>
>> Hi,
>>
>> I am reluctant to use a check_output() call because I would like the
>> stderroroutput in the exception message as to debug easily what went wrong.
>>
>>
> I'm not sure I follow, the CalledProcessError exception generated by a
> non-zero exist status on check_output has all the data you'd need in it.
>
> The CalledProcessError has the return code, but not the stderr output.
Manually raising the exception allows me to get the stderr.
>
> The check_output() documentation specifies to use Popen with
>> communicate() in order to properly get the stderr output/
>>
>> Note
>>
>> Do not use stderr=PIPE with this function as that can deadlock based on
>> the child process error volume. UsePopen <
>> https://docs.python.org/2/library/subprocess.html#subprocess.Popen> with
>> the communicate() method when you need a stderr pipe.
>>
>>
>>
>> If everything else is fine, can you please submit this patchset upstream ?
>>
>> Thank you,
>> Alex
>>
>> On Fri, Jul 17, 2015 at 11:27 AM, Michael Wood <michael.g.wood at intel.com
>> <mailto:michael.g.wood at intel.com>> wrote:
>>
>>
>>
>> From 4c34c2261685ca45a0013ed331a9663bd5e15898 Mon Sep 17 00:00:00 2001
>>
>> diff --git
>> a/bitbake/lib/toaster/bldcontrol/management/commands/checksettings.py
>> b/bitbake/lib/toaster/bldcontrol/management/commands/checksettings.py
>> index 9b591ad..590a7b0 100644
>> ---
>> a/bitbake/lib/toaster/bldcontrol/management/commands/checksettings.py
>> +++
>> b/bitbake/lib/toaster/bldcontrol/management/commands/checksettings.py
>> @@ -166,7 +166,12 @@ class Command(NoArgsCommand):
>> for dirname in
>> self._recursive_list_directories(be.sourcedir,2):
>> if os.path.exists(os.path.join(dirname,
>> ".templateconf")):
>> import subprocess
>> - conffilepath, error =
>> subprocess.Popen('bash -c ". '+os.path.join(dirname,
>> ".templateconf")+'; echo \"\$TEMPLATECONF\""', shell=True,
>> stdout=subprocess.PIPE).communicate()
>> + proc = subprocess.Popen('bash -c ".
>> '+os.path.join(dirname, ".templateconf")+'; echo
>> \"\$TEMPLATECONF\""', shell=True, stdout=subprocess.PIPE)
>> + conffilepath, stderroroutput =
>> proc.communicate()
>> + proc.wait()
>> + if proc.returncode != 0:
>> + raise Exception("Failed to source
>> TEMPLATECONF: %s" % stderroroutput)
>> +
>>
>>
>> Can't we just use subprocess.check_output and a try/except for all
>> of this?
>>
>> Apart from that everything else looks fine.
>>
>> Michael
>>
>>
>> On 15/07/15 13:31, Damian, Alexandru wrote:
>>
>> Hi,
>>
>> The patch that Michael suggested to fix is now refactored, and
>> the patchset rebased.
>>
>> I pushed the changes on the same branch (force update).
>>
>> Can you please review ?
>>
>> Cheers,
>> Alex
>>
>> On Mon, Jul 13, 2015 at 5:30 PM, Michael Wood
>> <michael.g.wood at intel.com <mailto:michael.g.wood at intel.com>
>> <mailto:michael.g.wood at intel.com
>> <mailto:michael.g.wood at intel.com>>> wrote:
>>
>>
>> Could you include the summary of each of the commits for
>> review so
>> that we can be sure which patches are the new ones
>>
>> * 3b0073f bitbake: toaster: fix updates on failed build
>> requests
>> * 1bdd687 bitbake: toaster: replace raising Exceptions in
>> loadconf
>> * 99a626a bitbake: toaster: do not stop data import on bad
>> data
>>
>> Look fine to me
>>
>> * 4fc46e7 bitbake: toastergui: fixing pylint warnings
>>
>> This one needs splitting up, pylint fixes, logging fixes ,
>> string
>> to int casting, etc
>>
>> Thanks,
>>
>> Michael
>>
>>
>>
>> On 08/07/15 15:52, Barros Pena, Belen wrote:
>>
>>
>> On 07/07/2015 17:27, "Damian, Alexandru"
>> <alexandru.damian at intel.com
>> <mailto:alexandru.damian at intel.com>
>> <mailto:alexandru.damian at intel.com
>> <mailto:alexandru.damian at intel.com>>>
>> wrote:
>>
>> Hi,
>>
>>
>> I have some comments below,
>>
>>
>> Alex
>>
>> On Tue, Jul 7, 2015 at 3:54 PM, Barros Pena, Belen
>> <belen.barros.pena at intel.com
>> <mailto:belen.barros.pena at intel.com>
>> <mailto:belen.barros.pena at intel.com
>> <mailto:belen.barros.pena at intel.com>>> wrote:
>>
>>
>>
>> On 07/07/2015 15:10, "Damian, Alexandru"
>> <alexandru.damian at intel.com
>> <mailto:alexandru.damian at intel.com>
>> <mailto:alexandru.damian at intel.com
>>
>> <mailto:alexandru.damian at intel.com>>>
>>
>> wrote:
>>
>> Hello,
>>
>>
>> I'm pushing two patches for review
>>
>>
>> - one is fixing 7955
>>
>> Thanks for the quick fix, Alex! I've tested this on
>> master, and I've
>> noticed a couple of things:
>>
>> 1. The invalid data from the layer index causes
>> warnings
>> when importing
>> the information. This might be because the debug
>> mode is
>> enabled, though,
>> but I thought I'd bringing it up just in case.
>>
>> This happens because data doesn't match what Toaster
>> expect - Toaster
>> has a bit stricter requirements than Layer Index.
>> I would
>> expect that
>> the warning messages are helpful to the user, and they
>> should not be
>> obscured. If a particular user wishes to not see
>> some of
>> the messages,
>> they can set up the debug level to something higher in
>> settings.py. Or we
>> can ship with a higher debug level by default, but
>> I don't
>> think we
>> should silently ignore bad data.
>>
>> Sure: I can see your point. I was only bringing it up,
>> but I
>> am not sure
>> what's best, to be honest: showing them or not. As you
>> said,
>> falling
>> silent doesn't sound right; on the other hand, the
>> messages
>> look a bit
>> alarming.
>>
>>
>> 2. I can see at least one recipe in the 'all recipes'
>> table without a
>> name. This particular one is provided by meta-ivi.
>> You can
>> add the layer
>> and you get a build button, which you can click,
>> although
>> when you do so
>> the build does not seem to start. I think we need
>> to hide
>> any recipes that
>> do not have a name from the list. They are
>> invalid, and
>> should not be
>> exposed to users.
>>
>> We can add such a check, of course, on imported
>> data. I
>> would say this
>> is the object of a different bug report, though.
>>
>> Done!
>> https://bugzilla.yoctoproject.org/show_bug.cgi?id=7969
>>
>> Also, I think we are going to need to back port
>> the fix to
>> Fido.
>>
>> Is back porting ok? Should we track this somehow?
>>
>> Thanks!
>>
>> Belén
>>
>> Cheers
>>
>> Belén
>>
>> - one is fixing various issues highlighted by
>> pylint
>>
>>
>>
>> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20
>> 1
>> 50707_bugs
>>
>>
>>
>> Can you please review ?
>>
>>
>> Cheers,
>> Alex
>>
>>
>>
>>
>> --
>> Alex Damian
>> Yocto Project
>>
>> SSG / OTC
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> -- Alex Damian
>> Yocto Project
>>
>> SSG / OTC
>>
>>
>>
>>
>>
>> -- _______________________________________________
>> toaster mailing list
>> toaster at yoctoproject.org <mailto:toaster at yoctoproject.org>
>> <mailto:toaster at yoctoproject.org
>>
>> <mailto:toaster at yoctoproject.org>>
>> https://lists.yoctoproject.org/listinfo/toaster
>>
>>
>>
>>
>> -- Alex Damian
>> Yocto Project
>> SSG / OTC
>>
>>
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ
>> VAT No: 860 2173 47
>>
>> This e-mail and any attachments may contain confidential
>> material for
>> the sole use of the intended recipient(s). Any review or
>> distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>>
>>
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ
>> VAT No: 860 2173 47
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>>
>>
>>
>>
>> --
>> Alex Damian
>> Yocto Project
>> SSG / OTC
>>
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ
>> VAT No: 860 2173 47
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>>
>>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
--
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150721/a68d1659/attachment-0001.html>
More information about the toaster
mailing list