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, 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@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@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



--
Alex Damian
Yocto Project
SSG / OTC