From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id 44986E00A3B; Tue, 28 Jul 2015 05:35:59 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on yocto-www.yoctoproject.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 X-Spam-HAM-Report: * -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high * trust * [192.55.52.88 listed in list.dnswl.org] * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id C9C81E00902 for ; Tue, 28 Jul 2015 05:35:56 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 28 Jul 2015 05:35:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,562,1432623600"; d="scan'208";a="772752442" Received: from linux.intel.com ([10.23.219.25]) by orsmga002.jf.intel.com with ESMTP; 28 Jul 2015 05:35:55 -0700 Received: from linux.intel.com (vmed.fi.intel.com [10.237.72.65]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by linux.intel.com (Postfix) with ESMTP id 23D816A4005; Tue, 28 Jul 2015 05:35:09 -0700 (PDT) Date: Tue, 28 Jul 2015 15:35:50 +0300 From: Ed Bartosh To: "Damian, Alexandru" Message-ID: <20150728123550.GA13075@linux.intel.com> References: <20150710144844.GA14439@linux.intel.com> <55A3EBB9.5060109@intel.com> <20150714130636.GA25220@linux.intel.com> <20150714135220.GC25220@linux.intel.com> <20150724162238.GA11956@linux.intel.com> MIME-Version: 1.0 In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "toaster@yoctoproject.org" Subject: Re: [review-request] ed/toaster/misc X-BeenThere: toaster@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list Reply-To: ed.bartosh@linux.intel.com List-Id: Web based interface for BitBake List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Jul 2015 12:35:59 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Alex, Thank you for review. On Mon, Jul 27, 2015 at 01:26:46PM +0100, Damian, Alexandru wrote: > Hello, > > On patch: > > toaster: Move getGitCloneDirectory to parent class > > the implementation of the getGitCloneDirectory is specific to localhost > controller - it can't be moved to the parent class because it references > local paths for "HEAD" checkouts - see > > + from os.path import dirname as DN > + local_checkout_path = DN(DN(DN(DN(DN(os.path.abspath(__file__)))))) > + #logger.debug("localhostbecontroller: using HEAD checkout in %s" % > local_checkout_path) Yes, it does, but only when branch equals 'HEAD', which is not the case for remote builds. > > building on "HEAD" doesn't make sense on SSH remote builds, so I would > leave this part of the code in an overriding method in the > LocalhostBEController. > > > On patch: > > toaster: Move 3 classes to toasterui > > I do not think that adding the classes to the bb.ui.toasterui is a good > idea - this is because the code in the toasterui is a Bitbake-specific UI > implementation, and should not be contaminated with code that doesn't do UI > event processing duties. > > I think a way better place would be in a new module under bb.server as they > conceptually implement a mock Bitbake server. Thanks, will move them there. > > > On patch: > > toaster: Add template for shell script > > I appreciate the ingenuity of using the Django template rendering framework > to create the startup script :) Thank you. I expected that you'll like it :) > The script can be better formatted though, so it's more readable. > It's a work in progress. I'll format it better when/if I'm done with this. > > On patch: > > toaster: Reimplement SSH BE Controller > > triggerBuild() is a long-running method if it going to execute > toasterui.main before returning. The system is not designed to support this > case - the caller of the triggerBuild() expects a quick return so that the > scheduling process can continue. In this sense, the localhost controller > runs a different process for the toasterui, and does not run the toasterui > in the same context. It worked for me just fine for some reason. I was able to see remote build going in the toaster ui. > > This also has a security implication - the code in the bldcontrol > application is run in the context of the web server user, but bitbake - > including toasterui - is expected to be run in the context of bitbake user. > This will lead to all sorts of interesting problems regarding file > creation, if the users are not identical (E.g. when toaster runs under a > web server). > That's valid point. I didn't think about it. Thank you for pointing that out to me. > Please instantiate another process in the bitbake context that runs the > toasterui, and does so asynchronously. yep, point taken. > > I am not sure what happens when the git repo starts with "file://" in the > remote use case; I would deem this to be an invalid use case in the remote > use case. Well, if the repo doesn't exist on the remote system build will fail. I don't think we need to do anything about it. Who knows, may be in some setups users would want to work with locally accessible repos. > > On patch: > > toaster: Move code from SSH controler to parent > > This would break Toaster in subtle ways due to reasons specified above, now > applying to the localhost use case as well. > It didn't break it for me.. BTW, did you read my explanations about this patchset here: https://lists.yoctoproject.org/pipermail/toaster/2015-July/002354.html This patchset is not ready to be merged, I submitted it as RFC. BTW, ed/toaster/misc contains different code now. Sorry for confusion. I pushed it to poky-contrib/ed/toaster/bec to avoid further misunderstanding. Thank you for review! It was very useful. Regards, Ed > > > Cheers, > Alex > > > On Fri, Jul 24, 2015 at 5:22 PM, Ed Bartosh > wrote: > > > On Tue, Jul 14, 2015 at 04:52:20PM +0300, Ed Bartosh wrote: > > > On Tue, Jul 14, 2015 at 02:37:27PM +0100, Damian, Alexandru wrote: > > > > Yep, the original code was about "cwd". Also, I think now that the > > logging > > > > messages really help, what if we turn them on with the lowest priority > > > > possible ? > > > Uncommented them. Please review. > > > > > > > Added implementation of [YOCTO #7571] to the branch. Please, review. > > > > > > Cheers, > > > > Alex > > > > > > > > On Tue, Jul 14, 2015 at 2:06 PM, Ed Bartosh < > > ed.bartosh@linux.intel.com> > > > > wrote: > > > > > > > > > Hi Michael, > > > > > > > > > > Thanks for the review. > > > > > > > > > > ... > > > > > > > > > > > While you're in localhostbecontroller > > > > > > > > > > > > We could also get rid of > > > > > > > > > > > > def _shellcmd(self, command, cwd = None): > > > > > > if cwd is None: > > > > > > cwd = self.be.sourcedir > > > > > > > > > > > > #logger.debug("lbc_shellcmmd: (%s) %s" % (cwd, command)) > > > > > > p = subprocess.Popen(command, cwd = cwd, shell=True, > > > > > > stdout=subprocess.PIPE, stderr=subprocess.PIPE) > > > > > > (out,err) = p.communicate() > > > > > > p.wait() > > > > > > if p.returncode: > > > > > > if len(err) == 0: > > > > > > err = "command: %s \n%s" % (command, out) > > > > > > else: > > > > > > err = "command: %s \n%s" % (command, err) > > > > > > #logger.warn("localhostbecontroller: shellcmd error > > %s" % > > > > > err) > > > > > > raise ShellCmdException(err) > > > > > > else: > > > > > > #logger.debug("localhostbecontroller: shellcmd > > success") > > > > > > return out > > > > > > > > > > > > > > > > > > I believe with: > > > > > > > > > > > > subprocess.check_output > > > > > > > > > > > Yep, I've noticed this too. I think the reason for not using > > > > > check_output was that it doesn't have cwd parameter. > > > > > > > > > > I'm planning to generalize build controllers, so I'll make much more > > > > > changes to this code anyway. Hopefully it will be for good :) > > > > > Just discussed this with Alex and he seems to be ok with this. > > > > > > > > > > -- > > > > > Regards, > > > > > Ed > > > > > -- > > > > > _______________________________________________ > > > > > toaster mailing list > > > > > toaster@yoctoproject.org > > > > > https://lists.yoctoproject.org/listinfo/toaster > > > > > > > > > > > > > > > > > > > > > -- > > > > Alex Damian > > > > Yocto Project > > > > SSG / OTC > > > > > > -- > > > -- > > > Regards, > > > Ed > > > -- > > > _______________________________________________ > > > toaster mailing list > > > toaster@yoctoproject.org > > > https://lists.yoctoproject.org/listinfo/toaster > > > > -- > > -- > > Regards, > > Ed > > > > > > -- > Alex Damian > Yocto Project > SSG / OTC -- -- Regards, Ed