[review-request] ed/toaster/misc

[review-request] ed/toaster/misc

[Ed Bartosh]

Hi reviewers,

Please review fix for YOCTO: #7965: Wait for toaster gui to come
and couple of other changes:
1. tiny refactoring change:  get rid of _createdirpath function
2. and small fix: Fix usage of wrong variables

--
Regards,
Ed

Re: [review-request] ed/toaster/misc

[Michael Wood]

On 10/07/15 15:48, Ed Bartosh wrote:
> Hi reviewers,
>
> Please review fix for YOCTO: #7965: Wait for toaster gui to come
> and couple of other changes:
> 1. tiny refactoring change:  get rid of _createdirpath function
> 2. and small fix: Fix usage of wrong variables
>
> --
> Regards,
> Ed

* 7cee08d toaster: get rid of _createdirpath function
* eae47f7 toaster: Fix usage of wrong variables
* 321d6ae toaster: Wait for toaster gui to come

Looks fine to me, though not familiar enough with build stuff to fully 
OK this branch.

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

Thanks,

Michael

Re: [review-request] ed/toaster/misc

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]

Hi,

I have some comments below:

*toaster: get rid of _createdirpath function*

    os.mkdirs is missing the mode specification; I am not sure it matters,
though.


*toaster: Wait for toaster gui to come*

  I am not sure which logger should to be used here; the reason for
changing the logger in the first place (which was originally defined as
"BitBake" was that the "BitBake" logger settings are modified in
bitbake/lib/bb/__init__.py, and it is targeted at the inner Bitbake core;
some updates there changed the original output, with output disappearing
from the console being an obvious effect.
Messing with "BitBake" logger settings with  this may have side effects
throught the bitbake code, so I refrained from touching that

  I considered using the "toaster" logger that is defined in
bitbake/lib/toaster/toastermain/settings.py and outputs strictly to
console; I felt likewise reluctant to use this logger because changing it
to output to a file would be affect the way the django application is
logging.

  ToasterLogger is not defined anywhere, and this is why I used it here. I
am not sure I understand the root cause of why the code breaks when using
"ToasterLogger" instead of "BitBake"; can you please assess the root cause
and get a solution to have messages properly logged to the toaster_ui.log
file using a logger that's not BitBake ?


Otherwise, all fine.

Thank you,
Alex



On Mon, Jul 13, 2015 at 5:47 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

> On 10/07/15 15:48, Ed Bartosh wrote:
>
>> Hi reviewers,
>>
>> Please review fix for YOCTO: #7965: Wait for toaster gui to come
>> and couple of other changes:
>> 1. tiny refactoring change:  get rid of _createdirpath function
>> 2. and small fix: Fix usage of wrong variables
>>
>> --
>> Regards,
>> Ed
>>
>
> * 7cee08d toaster: get rid of _createdirpath function
> * eae47f7 toaster: Fix usage of wrong variables
> * 321d6ae toaster: Wait for toaster gui to come
>
> Looks fine to me, though not familiar enough with build stuff to fully OK
> this branch.
>
> 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
>
> Thanks,
>
> Michael
>
> --
> _______________________________________________
> toaster mailing list
> toaster@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/toaster
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 5157 bytes --]

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

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

Re: [review-request] ed/toaster/misc

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

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 ?

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

[-- Attachment #2: Type: text/html, Size: 3076 bytes --]

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

Hi Alex,

Thank you for review. My answers are below.

On Tue, Jul 14, 2015 at 12:18:02PM +0100, Damian, Alexandru wrote:
> Hi,
> 
> I have some comments below:
> 
> *toaster: get rid of _createdirpath function*
> 
>     os.mkdirs is missing the mode specification; I am not sure it matters,
> though.
I think it shouldn't matter, but it doesn't look very clear as I was
lazy and put both changes in one commit. Thanks to point that out to me.
I've added mode to the call and updated the branch.

> 
> 
> *toaster: Wait for toaster gui to come*
> 
>   I am not sure which logger should to be used here; the reason for
> changing the logger in the first place (which was originally defined as
> "BitBake" was that the "BitBake" logger settings are modified in
> bitbake/lib/bb/__init__.py, and it is targeted at the inner Bitbake core;
> some updates there changed the original output, with output disappearing
> from the console being an obvious effect.
> Messing with "BitBake" logger settings with  this may have side effects
> throught the bitbake code, so I refrained from touching that
> 
>   I considered using the "toaster" logger that is defined in
> bitbake/lib/toaster/toastermain/settings.py and outputs strictly to
> console; I felt likewise reluctant to use this logger because changing it
> to output to a file would be affect the way the django application is
> logging.
> 
>   ToasterLogger is not defined anywhere, and this is why I used it here.
And this apparently didn't work out well and caused "ToasterUI waiting for
events" message to disappear from toaster_ui.log and be controller
failing after 10 unsuccessful attempts to find this message in the log.

> I am not sure I understand the root cause of why the code breaks when using
> "ToasterLogger" instead of "BitBake"; can you please assess the root cause
> and get a solution to have messages properly logged to the toaster_ui.log
> file using a logger that's not BitBake ?
OK, I'll look at it. Meanwhile I'd suggest to accept this change because
it's much better than previous one. It doesn't fix the root case, but it
reverts the change that caused the bug.

Regards,
Ed

> On Mon, Jul 13, 2015 at 5:47 PM, Michael Wood <michael.g.wood@intel.com>
> wrote:
> 
> > On 10/07/15 15:48, Ed Bartosh wrote:
> >
> >> Hi reviewers,
> >>
> >> Please review fix for YOCTO: #7965: Wait for toaster gui to come
> >> and couple of other changes:
> >> 1. tiny refactoring change:  get rid of _createdirpath function
> >> 2. and small fix: Fix usage of wrong variables
> >>
> >> --
> >> Regards,
> >> Ed
> >>
> >
> > * 7cee08d toaster: get rid of _createdirpath function
> > * eae47f7 toaster: Fix usage of wrong variables
> > * 321d6ae toaster: Wait for toaster gui to come
> >
> > Looks fine to me, though not familiar enough with build stuff to fully OK
> > this branch.
> >
> > 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
> >
> > Thanks,
> >
> > Michael
> >
> > --
> > _______________________________________________
> > toaster mailing list
> > toaster@yoctoproject.org
> > https://lists.yoctoproject.org/listinfo/toaster
> >
> 
> 
> 
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC

> -- 
> _______________________________________________
> toaster mailing list
> toaster@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/toaster


-- 
--
Regards,
Ed

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

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.

> 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

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

Hi Alex,

> > *toaster: Wait for toaster gui to come*
> > 
> >   I am not sure which logger should to be used here; the reason for
> > changing the logger in the first place (which was originally defined as
> > "BitBake" was that the "BitBake" logger settings are modified in
> > bitbake/lib/bb/__init__.py, and it is targeted at the inner Bitbake core;
> > some updates there changed the original output, with output disappearing
> > from the console being an obvious effect.
> > Messing with "BitBake" logger settings with  this may have side effects
> > throught the bitbake code, so I refrained from touching that
> > 
> >   I considered using the "toaster" logger that is defined in
> > bitbake/lib/toaster/toastermain/settings.py and outputs strictly to
> > console; I felt likewise reluctant to use this logger because changing it
> > to output to a file would be affect the way the django application is
> > logging.
> > 
> >   ToasterLogger is not defined anywhere, and this is why I used it here.
> And this apparently didn't work out well and caused "ToasterUI waiting for
> events" message to disappear from toaster_ui.log and be controller
> failing after 10 unsuccessful attempts to find this message in the log.
> 
> > I am not sure I understand the root cause of why the code breaks when using
> > "ToasterLogger" instead of "BitBake"; can you please assess the root cause
> > and get a solution to have messages properly logged to the toaster_ui.log
> > file using a logger that's not BitBake ?
> OK, I'll look at it. Meanwhile I'd suggest to accept this change because
> it's much better than previous one. It doesn't fix the root case, but it
> reverts the change that caused the bug.
>
The root cause of the issue was that logging level was not set for the
ToasterLogger. I've updated the patch:
https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=ed/toaster/misc&id=6d1ddedaef28c0f17c2b821c3ceb077641fab772

Please, review.

--
Regards,
Ed

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

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

Re: [review-request] ed/toaster/misc

[Damian, Alexandru]

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

Re: [review-request] ed/toaster/misc

[Ed Bartosh]

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

Re: [review-request] ed/toaster/misc

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 8862 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 13384 bytes --]