[Toaster] [review-request] ed/toaster/misc

Damian, Alexandru alexandru.damian at intel.com
Tue Jul 28 07:00:36 PDT 2015 

On Tue, Jul 28, 2015 at 1:35 PM, Ed Bartosh <ed.bartosh at linux.intel.com>
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 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.
>

​It is going to work if you have a single build enviro​nment, 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 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.
>
> ​Yep, you are right.​




> >
> > 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.
>
>
​Yep, 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 ​that 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.​


>
> 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 at 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, review.
> > >
> > > > > Cheers,
> > > > > Alex
> > > > >
> > > > > On Tue, Jul 14, 2015 at 2:06 PM, Ed Bartosh <
> > > ed.bartosh at 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 at yoctoproject.org
> > > > > > https://lists.yoctoproject.org/listinfo/toaster
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Alex Damian
> > > > > Yocto Project
> > > > > SSG / OTC
> > > >
> > > > --
> > > > --
> > > > Regards,
> > > > Ed
> > > > --
> > > > _______________________________________________
> > > > toaster mailing list
> > > > toaster at yoctoproject.org
> > > > https://lists.yoctoproject.org/listinfo/toaster
> > >
> > > --
> > > --
> > > Regards,
> > > Ed
> > >
> >
> >
> >
> > --
> > Alex Damian
> > Yocto Project
> > SSG / OTC
>
> --
> --
> Regards,
> Ed
>



-- 
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150728/9ed96ee4/attachment-0001.html>

More information about the toaster mailing list