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

[Damian, Alexandru]

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)

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.


On patch:

toaster: Add template for shell script

I appreciate the ingenuity of using the Django template rendering framework
to create the startup script :)
The script can be better formatted though, so it's more readable.


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.

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).

Please instantiate another process in the bitbake context that runs the
toasterui, and does so asynchronously.


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.


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.



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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150727/e7337624/attachment-0001.html>