From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id B2A6DE0099A; Tue, 28 Jul 2015 07:01:02 -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=-2.6 required=5.0 tests=BAYES_00,HTML_MESSAGE, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-HAM-Report: * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] * 0.0 HTML_MESSAGE BODY: HTML included in message * -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low * trust * [209.85.213.182 listed in list.dnswl.org] Received: from mail-ig0-f182.google.com (mail-ig0-f182.google.com [209.85.213.182]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id 82B38E00985 for ; Tue, 28 Jul 2015 07:00:59 -0700 (PDT) Received: by igbpg9 with SMTP id pg9so122504969igb.0 for ; Tue, 28 Jul 2015 07:00:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=LUGD+v0qoeDY+YI1mfT7rovJ5VQK646z+5qwcg3piZE=; b=GqKhSQUKD5BkCABGE3OgUk1draN97sJ25LtJYlWQIovG1qXuQWy9sRSuwqiezK6XjC npxY9hEnW/yQaHsAXjBQh05qHG9/zwZsR6Vgpg692VTxL1Vz//WEAlmZXfNhvsLbVqia qam4LiUrdcmLnXCkHbBcGMIzzQZPoh26uL21zTzb6wRJwY4vavqT+rnITCyg8w/AbnIL Khj4YNjGdr53U43ckW3Y5ORB3PMeHIYymkVza+zLcrp+Yct7BNtsgW9tA4rOMCTLsbC2 Besi3uDgkoyz9cet5YFPsVqc6XpcFaJ2STpnUn0SBz38Lyb0WZyk82HA7UuAF5PNDtgK g2nA== X-Gm-Message-State: ALoCoQmtIPdoX5OH/mb5vdFK8H11XjuLvjQusrXxAG84aVW6nhMh/OPCKhAoWTMxN3gs3nh2ZUSs X-Received: by 10.107.37.134 with SMTP id l128mr58983342iol.92.1438092056259; Tue, 28 Jul 2015 07:00:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.79.65.207 with HTTP; Tue, 28 Jul 2015 07:00:36 -0700 (PDT) In-Reply-To: <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> <20150728123550.GA13075@linux.intel.com> From: "Damian, Alexandru" Date: Tue, 28 Jul 2015 15:00:36 +0100 Message-ID: To: Ed Bartosh Cc: "toaster@yoctoproject.org" Subject: Re: [review-request] ed/toaster/misc X-BeenThere: toaster@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list 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 14:01:02 -0000 Content-Type: multipart/alternative; boundary=001a1140ea862b1166051befe6ce --001a1140ea862b1166051befe6ce Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, Jul 28, 2015 at 1:35 PM, Ed Bartosh wrote: > 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 reference= s > > local paths for "HEAD" checkouts - see > > > > + from os.path import dirname as DN > > + local_checkout_path =3D > 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 d= o > 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 controlle= r > > 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. > =E2=80=8BIt is going to work if you have a single build enviro=E2=80=8Bnmen= t, or if there is a single build executing at a time. So the proper way to test this is to set up two build environments (via the admin/ interface), and try to execute two builds simultaneously). > > > > > 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 t= he > > 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. > > =E2=80=8BYep, you are right.=E2=80=8B > > > > 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. > > =E2=80=8BYep, and I agree to your assesment - we should be reusing as much = code as possible; the original localhost controller is a quick-and-dirty affair, I would pretty much prefer having a script =E2=80=8Bthat starts everythings, = and toss that around local and remote machines :). Furthermore, this same script could be used (with modifications), possibly, to set up a shell-friendly build environment so that a user can use command line tools on something that Toaster exported; or could be pushed as build script to a Jenkins instance - and voila, we have build-environment export capabilities from Toaster. > BTW, ed/toaster/misc contains different code now. Sorry for confusion. > I got what's going on, and I've seen your new branch. But, yes, maybe it's better to keep one branch per work piece for ease of review.=E2=80=8B > > 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 th= e > > > 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 =3D None): > > > > > > > if cwd is None: > > > > > > > cwd =3D self.be.sourcedir > > > > > > > > > > > > > > #logger.debug("lbc_shellcmmd: (%s) %s" % (cwd, > command)) > > > > > > > p =3D subprocess.Popen(command, cwd =3D cwd, shell=3D= True, > > > > > > > stdout=3Dsubprocess.PIPE, stderr=3Dsubprocess.PIPE) > > > > > > > (out,err) =3D p.communicate() > > > > > > > p.wait() > > > > > > > if p.returncode: > > > > > > > if len(err) =3D=3D 0: > > > > > > > err =3D "command: %s \n%s" % (command, out) > > > > > > > else: > > > > > > > err =3D "command: %s \n%s" % (command, err) > > > > > > > #logger.warn("localhostbecontroller: shellcmd err= or > > > %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 > --=20 Alex Damian Yocto Project SSG / OTC --001a1140ea862b1166051befe6ce Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Tue, Jul 28, 2015 at 1:35 PM, Ed Bartosh <ed.bartosh@linux= .intel.com> wrote:
Hi Alex,

Thank you for review.

On Mon, Jul 27, 2015 at 01:26:46PM +0100, Damian, A= lexandru wrote:
> Hello,
>
> On patch:
>
> toaster: Move getGitCloneDirectory to parent class
>
> the implementation of the getGitCloneDirectory is specific to localhos= t
> controller - it can't be moved to the parent class because it refe= rences
> local paths for "HEAD" checkouts - see
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 from os.path import dirname as DN
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 local_checkout_path =3D DN(DN(DN(DN(DN(os= .path.abspath(__file__))))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 #logger.debug("localhostbecontroller= : using HEAD checkout in %s" %
> local_checkout_path)
Yes, it does, but only when branch equals 'HEAD', which is n= ot the case
for remote builds.

>
> building on "HEAD" doesn't make sense on SSH remote buil= ds, 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 goo= d
> idea - this is because the code in the toasterui is a Bitbake-specific= UI
> implementation, and should not be contaminated with code that doesn= 9;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 fram= ework
> 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.<= br> >
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 controll= er
> runs a different process for the toasterui, and does not run the toast= erui
> 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.

=E2=80=8BIt is going to w= ork if you have a single build enviro=E2=80=8Bnment, or if there is a singl= e build executing at a time. So the proper way to test this is to set up tw= o build environments (via the admin/ interface), and try to execute two bui= lds simultaneously).

=C2=A0

>
> 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 p= ointing that out to me.

> Please instantiate another process in the bitbake context that runs th= e
> 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 re= mote
> 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<= br> setups users would want to work with locally accessible repos.

=E2=80=8BYep, you are right.=E2=80= =8B


=C2=A0
>
> 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.


=E2=80=8BYep, and I agree to your assesment - we should= be reusing as much code as possible; the original localhost controller is = a quick-and-dirty affair, I would pretty much prefer having a script =E2=80= =8Bthat starts everythings, and toss that around local and remote machines = :).

Furthermore, this same script could be used (with modifications)= , possibly, to set up a shell-friendly build environment so that a user can= use command line tools on something that Toaster exported; or could be pus= hed as build script to a Jenkins instance - and voila, we have build-enviro= nment export capabilities from Toaster.

=C2=A0
BTW, ed/toaster/misc contains different code now. Sorry for confusion.
<= /blockquote>

I got what's going on, and I've seen your new branch= . But, yes, maybe it's better to keep one branch per work piece for eas= e of review.=E2=80=8B
=C2=A0

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 <ed.bartosh@linux.intel.com>
> 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, revi= ew.
> >
> > > > 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
> > > > > >
> > > > > >=C2=A0 =C2=A0 =C2=A0def _shellcmd(self, comman= d, cwd =3D None):
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if cwd is No= ne:
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0cwd =3D self.be.sourcedir
> > > > > >
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0#logger.debu= g("lbc_shellcmmd: (%s) %s" % (cwd, command))
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p =3D subpro= cess.Popen(command, cwd =3D cwd, shell=3DTrue,
> > > > > > stdout=3Dsubprocess.PIPE, stderr=3Dsubprocess= .PIPE)
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(out,err) = =3D p.communicate()
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p.wait()
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if p.returnc= ode:
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0if len(err) =3D=3D 0:
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0err =3D "command: %s \n%s" % (command, out)
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0else:
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0err =3D "command: %s \n%s" % (command, err)
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0#logger.warn("localhostbecontroller: shellcmd error
> > %s" %
> > > > > err)
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0raise ShellCmdException(err)
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else:
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0#logger.debug("localhostbecontroller: shellcmd
> > success")
> > > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0return 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 para= meter.
> > > > >
> > > > > 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 o= k with this.
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Ed
> > > > > --
> > > > > _______________________________________________ > > > > > toaster mailing list
> > > > > toaster@yoctoproject.org
> > > > > https://lis= ts.yoctoproject.org/listinfo/toaster
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Alex Damian
> > > > Yocto Project
> > > > SSG / OTC
> > >
> > > --
> > > --
> > > Regards,
> > > Ed
> > > --
> > > _______________________________________________
> > > toaster mailing list
> > > toaster@y= octoproject.org
> > > https://lists.yoctoproject.org/listinfo/toaster
> >
> > --
> > --
> > Regards,
> > Ed
> >
>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC

--
--
Regards,
Ed



--
Alex Damian
Yocto Project
SSG / OTC=C2=A0
--001a1140ea862b1166051befe6ce--