From: "Damian, Alexandru" <alexandru.damian@intel.com>
To: Ed Bartosh <ed.bartosh@linux.intel.com>
Cc: "toaster@yoctoproject.org" <toaster@yoctoproject.org>
Subject: Re: [review-request] ed/toaster/misc
Date: Mon, 27 Jul 2015 13:26:46 +0100 [thread overview]
Message-ID: <CAJ2CSBtbmX8An22ydiAW5L7MD+TMNq5neESTjUeqmUUF1X-+pQ@mail.gmail.com> (raw)
In-Reply-To: <20150724162238.GA11956@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 5567 bytes --]
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@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
[-- Attachment #2: Type: text/html, Size: 11242 bytes --]
next prev parent reply other threads:[~2015-07-27 12:27 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 [this message]
2015-07-28 12:35 ` Ed Bartosh
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=CAJ2CSBtbmX8An22ydiAW5L7MD+TMNq5neESTjUeqmUUF1X-+pQ@mail.gmail.com \
--to=alexandru.damian@intel.com \
--cc=ed.bartosh@linux.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.