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