Re: [review-request] adamian/20150707_bugs

[Michael Wood <michael.g.wood@intel.com>]

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 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@intel.com <mailto:michael.g.wood@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@intel.com <mailto:michael.g.wood@intel.com>
>         <mailto:michael.g.wood@intel.com
>         <mailto:michael.g.wood@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@intel.com
>         <mailto:alexandru.damian@intel.com>
>         <mailto:alexandru.damian@intel.com
>         <mailto:alexandru.damian@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@intel.com
>         <mailto:belen.barros.pena@intel.com>
>                     <mailto:belen.barros.pena@intel.com
>         <mailto:belen.barros.pena@intel.com>>> wrote:
>
>
>
>                     On 07/07/2015 15:10, "Damian, Alexandru"
>                     <alexandru.damian@intel.com
>         <mailto:alexandru.damian@intel.com>
>                     <mailto:alexandru.damian@intel.com
>         <mailto:alexandru.damian@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@yoctoproject.org <mailto:toaster@yoctoproject.org>
>         <mailto:toaster@yoctoproject.org
>         <mailto:toaster@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.
>