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.
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 <[email protected]> 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 <[email protected] >> <mailto:[email protected]>> 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" >> <[email protected] <mailto:[email protected]>> >> wrote: >> >> Hi, >> >> >> I have some comments below, >> >> >> Alex >> >> On Tue, Jul 7, 2015 at 3:54 PM, Barros Pena, Belen >> <[email protected] >> <mailto:[email protected]>> wrote: >> >> >> >> On 07/07/2015 15:10, "Damian, Alexandru" >> <[email protected] >> <mailto:[email protected]>> >> >> 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 >> [email protected] <mailto:[email protected]> >> 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
-- _______________________________________________ toaster mailing list [email protected] https://lists.yoctoproject.org/listinfo/toaster
