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 <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, review.
> >
> > > > Cheers,
> > > > Alex
> > > >
> > > > On Tue, Jul 14, 2015 at 2:06 PM, Ed Bartosh <
> > > > > toaster@yoctoproject.org> > 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
> > > > > 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