From: Ed Bartosh <ed.bartosh@linux.intel.com>
To: "Damian, Alexandru" <alexandru.damian@intel.com>
Cc: "toaster@yoctoproject.org" <toaster@yoctoproject.org>
Subject: Re: [review-request] ed/toaster/misc
Date: Tue, 28 Jul 2015 15:35:50 +0300 [thread overview]
Message-ID: <20150728123550.GA13075@linux.intel.com> (raw)
In-Reply-To: <CAJ2CSBtbmX8An22ydiAW5L7MD+TMNq5neESTjUeqmUUF1X-+pQ@mail.gmail.com>
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 <
> > 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
next prev parent reply other threads:[~2015-07-28 12:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-10 14:48 [review-request] ed/toaster/misc Ed Bartosh
2015-07-13 16:47 ` Michael Wood
2015-07-14 11:18 ` Damian, Alexandru
2015-07-14 13:37 ` Ed Bartosh
2015-07-19 19:35 ` Ed Bartosh
2015-07-14 13:06 ` Ed Bartosh
2015-07-14 13:37 ` Damian, Alexandru
2015-07-14 13:52 ` Ed Bartosh
2015-07-24 16:22 ` Ed Bartosh
2015-07-27 12:26 ` Damian, Alexandru
2015-07-28 12:35 ` Ed Bartosh [this message]
2015-07-28 14:00 ` Damian, Alexandru
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150728123550.GA13075@linux.intel.com \
--to=ed.bartosh@linux.intel.com \
--cc=alexandru.damian@intel.com \
--cc=toaster@yoctoproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.