[Toaster] [review-request] adamian/20150707_bugs
Michael Wood
michael.g.wood at intel.com
Tue Jul 21 05:39:15 PDT 2015
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 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.
>
More information about the toaster
mailing list