Patchwork [bitbake-devel] knotty: Fold knotty2 into knotty and make it the default

login
register
mail settings
Submitter Richard Purdie
Date Aug. 15, 2012, 4:50 p.m.
Message ID <1345049422.14667.11.camel@ted>
Download mbox | patch
Permalink /patch/34655/
State New
Headers show

Comments

Richard Purdie - Aug. 15, 2012, 4:50 p.m.
There is no good reason knotty2 shouldn't be the default now. If you need
the old behaviour, just pipe the output through cat as non-interactive
terminals get the old output.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
Jason Wessel - Aug. 15, 2012, 4:56 p.m.
On 08/15/2012 11:50 AM, Richard Purdie wrote:
> There is no good reason knotty2 shouldn't be the default now. If you need
> the old behaviour, just pipe the output through cat as non-interactive
> terminals get the old output.


I didn't look at this in much detail to see if it is was a direct fold or if you made changes along the way.

I have a whole series of patches outstanding against knotty2 for fixing its behavior for the very annoying line wrapping problems and the like.  Certainly I can rebase the work done to this end, but these problems need to be addressed, preferably before you make this the default.

Jason.
Richard Purdie - Aug. 15, 2012, 5:03 p.m.
On Wed, 2012-08-15 at 11:56 -0500, Jason Wessel wrote:
> On 08/15/2012 11:50 AM, Richard Purdie wrote:
> > There is no good reason knotty2 shouldn't be the default now. If you need
> > the old behaviour, just pipe the output through cat as non-interactive
> > terminals get the old output.
> 
> 
> I didn't look at this in much detail to see if it is was a direct fold or if you made changes along the way.
> 
> I have a whole series of patches outstanding against knotty2 for
> fixing its behavior for the very annoying line wrapping problems and
> the like.  Certainly I can rebase the work done to this end, but these
> problems need to be addressed, preferably before you make this the
> default.

Have a look at the preceding patches ;-)

I've fixed the line wrapping and window resizing issues first. It was
based on some of your changes but I fixed various things (like having
progressbar chain the WINCH signal handlers instead of hacking around
it).

This doesn't add the interactive logging as I'm still not sure that
makes sense in master. If we do that I want to think more carefully
about the UI to it.

The interactive terminal stuff is also pending as I was pretty horrified
when I realised how you implemented it. What we need to do is issue an
event and have bitbake respond to that event where the event contains a
"command" parameter.

So I'm working through the patches but I think with the changes I just
posted, knotty2 should be ready for wider usage.

Cheers,

Richard
Chris Larson - Aug. 15, 2012, 7:20 p.m.
On Wed, Aug 15, 2012 at 9:50 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> There is no good reason knotty2 shouldn't be the default now. If you need
> the old behaviour, just pipe the output through cat as non-interactive
> terminals get the old output.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

I'm 100% in favor of this series, for what it's worth. I've been using
knotty2 as my default BITBAKE_UI for months, just ignoring the minor
bugs. The reduction in unnecessary output makes it *much* easier to
spot *real* problems.

That said, I think we should look into prefixing some of our log
messages — some of them don't make it clear what task/recipe emitted
them, making it difficult to isolate their cause. But of course, that
was an issue with knotty as well, but it was slightly easier to narrow
down the cause based on where it stood between the task
started/completed messages :). Would you be open to potentially
injecting a log message filter which adds task context informastion,
in the code which forks the tasks? Then the formatter would have to do
a hasattr() to check for that and add it to the format, but it'd mean
we *always* know what task log messages are coming from.
Richard Purdie - Aug. 15, 2012, 9:52 p.m.
On Wed, 2012-08-15 at 12:20 -0700, Chris Larson wrote:
> On Wed, Aug 15, 2012 at 9:50 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > There is no good reason knotty2 shouldn't be the default now. If you need
> > the old behaviour, just pipe the output through cat as non-interactive
> > terminals get the old output.
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> I'm 100% in favor of this series, for what it's worth. I've been using
> knotty2 as my default BITBAKE_UI for months, just ignoring the minor
> bugs. The reduction in unnecessary output makes it *much* easier to
> spot *real* problems.

Agreed, that is one of the intents.

> That said, I think we should look into prefixing some of our log
> messages — some of them don't make it clear what task/recipe emitted
> them, making it difficult to isolate their cause. But of course, that
> was an issue with knotty as well, but it was slightly easier to narrow
> down the cause based on where it stood between the task
> started/completed messages :). Would you be open to potentially
> injecting a log message filter which adds task context informastion,
> in the code which forks the tasks? Then the formatter would have to do
> a hasattr() to check for that and add it to the format, but it'd mean
> we *always* know what task log messages are coming from.

Obviously such a change needs to be carefully considered but yes, I
think knowing where a message has come from is important and I'm in
favour of doing something. I'm slightly surprised there isn't something
in the message already.

Thinking about this more, I think we did add in the pid or at least
looked at doing so, with the assumption that the UI code could have
information about what that pid represents and be able to supplement the
data. I think the graphical UIs might use that to put the messages into
different "buckets" in the graphical task display.

The immediate reaction is "lets just add all the data to the event"
which I can certainly understand. My big worry is the pushing up the
size of each event to something that damages performance, particularly
if we so start using remote UIs. This is why the uihelper code exists,
effectively to maintain a UI side cache of the data rather than
pickle/unpickling the same thing many times.

As a side note, I do want to get handlers only receiving the events they
care about with server side filtering too. That is starting to show up
on profiles of some workloads.

Also, we currently use the word "package" in the old knotty output, I
want to change that to "recipe" as its confusing and we should be
consistent about our terminology. I guess I should propose a patch :)

Cheers,

Richard

Patch

diff --git a/bitbake/lib/bb/ui/knotty.py b/bitbake/lib/bb/ui/knotty.py
index 34b5969..304ba29 100644
--- a/bitbake/lib/bb/ui/knotty.py
+++ b/bitbake/lib/bb/ui/knotty.py
@@ -27,6 +27,9 @@  import logging
 import progressbar
 import signal
 import bb.msg
+import fcntl
+import struct
+import copy
 from bb.ui import uihelper
 
 logger = logging.getLogger("BitBake")
@@ -84,39 +87,124 @@  def pluralise(singular, plural, qty):
     else:
         return plural % qty
 
+
+class InteractConsoleLogFilter(logging.Filter):
+    def __init__(self, tf, format):
+        self.tf = tf
+        self.format = format
+
+    def filter(self, record):
+        if record.levelno == self.format.NOTE and (record.msg.startswith("Running") or record.msg.startswith("package ")):
+            return False
+        self.tf.clearFooter()
+        return True
+
 class TerminalFilter(object):
+    columns = 80
+
+    def sigwinch_handle(self, signum, frame):
+        self.columns = self.getTerminalColumns()
+        if self._sigwinch_default:
+            self._sigwinch_default(signum, frame)
+
+    def getTerminalColumns(self):
+        def ioctl_GWINSZ(fd):
+            try:
+                cr = struct.unpack('hh', fcntl.ioctl(fd, self.termios.TIOCGWINSZ, '1234'))
+            except:
+                return None
+            return cr
+        cr = ioctl_GWINSZ(sys.stdout.fileno())
+        if not cr:
+            try:
+                fd = os.open(os.ctermid(), os.O_RDONLY)
+                cr = ioctl_GWINSZ(fd)
+                os.close(fd)
+            except:
+                pass
+        if not cr:
+            try:
+                cr = (env['LINES'], env['COLUMNS'])
+            except:
+                cr = (25, 80)
+        return cr[1]
+
     def __init__(self, main, helper, console, format):
         self.main = main
         self.helper = helper
+        self.cuu = None
+        self.stdinbackup = None
+        self.interactive = sys.stdout.isatty()
+        self.footer_present = False
+        self.lastpids = []
+
+        if not self.interactive:
+            return
+
+        import curses
+        import termios
+        self.curses = curses
+        self.termios = termios
+        try:
+            fd = sys.stdin.fileno()
+            self.stdinbackup = termios.tcgetattr(fd)
+            new = copy.deepcopy(self.stdinbackup)
+            new[3] = new[3] & ~termios.ECHO
+            termios.tcsetattr(fd, termios.TCSADRAIN, new)
+            curses.setupterm()
+            self.ed = curses.tigetstr("ed")
+            if self.ed:
+                self.cuu = curses.tigetstr("cuu")
+            try:
+                self._sigwinch_default = signal.getsignal(signal.SIGWINCH)
+                signal.signal(signal.SIGWINCH, self.sigwinch_handle)
+            except:
+                pass
+            self.columns = self.getTerminalColumns()
+        except:
+            self.cuu = None
+        console.addFilter(InteractConsoleLogFilter(self, format))
 
     def clearFooter(self):
-        return
+        if self.footer_present:
+            lines = self.footer_present
+            sys.stdout.write(self.curses.tparm(self.cuu, lines))
+            sys.stdout.write(self.curses.tparm(self.ed))
+        self.footer_present = False
 
     def updateFooter(self):
-        if not main.shutdown or not self.helper.needUpdate:
+        if not self.cuu:
             return
-
         activetasks = self.helper.running_tasks
+        failedtasks = self.helper.failed_tasks
         runningpids = self.helper.running_pids
-
-        if len(runningpids) == 0:
+        if self.footer_present and (self.lastpids == runningpids):
+            return
+        if self.footer_present:
+            self.clearFooter()
+        if not activetasks:
             return
-
-        self.helper.getTasks()
-
         tasks = []
         for t in runningpids:
             tasks.append("%s (pid %s)" % (activetasks[t]["title"], t))
 
-        if main.shutdown:
-            print("Waiting for %s running tasks to finish:" % len(activetasks))
+        if self.main.shutdown:
+            content = "Waiting for %s running tasks to finish:" % len(activetasks)
         else:
-            print("Currently %s running tasks (%s of %s):" % (len(activetasks), self.helper.tasknumber_current, self.helper.tasknumber_total))
+            content = "Currently %s running tasks (%s of %s):" % (len(activetasks), self.helper.tasknumber_current, self.helper.tasknumber_total)
+        print content
+        lines = 1 + int(len(content) / (self.columns + 1))
         for tasknum, task in enumerate(tasks):
-            print("%s: %s" % (tasknum, task))
+            content = "%s: %s" % (tasknum, task)
+            print content
+            lines = lines + 1 + int(len(content) / (self.columns + 1))
+        self.footer_present = lines
+        self.lastpids = runningpids[:]
 
     def finish(self):
-        return
+        if self.stdinbackup:
+            fd = sys.stdin.fileno()
+            self.termios.tcsetattr(fd, self.termios.TCSADRAIN, self.stdinbackup)
 
 def main(server, eventHandler, tf = TerminalFilter):
 
diff --git a/bitbake/lib/bb/ui/knotty2.py b/bitbake/lib/bb/ui/knotty2.py
deleted file mode 100644
index 57ad67f..0000000
--- a/bitbake/lib/bb/ui/knotty2.py
+++ b/dev/null
@@ -1,149 +0,0 @@ 
-#
-# BitBake (No)TTY UI Implementation (v2)
-#
-# Handling output to TTYs or files (no TTY)
-#
-# Copyright (C) 2012 Richard Purdie
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License version 2 as
-# published by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program; if not, write to the Free Software Foundation, Inc.,
-# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
-
-from bb.ui import knotty
-import logging
-import sys
-import os
-import fcntl
-import struct
-import copy
-logger = logging.getLogger("BitBake")
-
-class InteractConsoleLogFilter(logging.Filter):
-    def __init__(self, tf, format):
-        self.tf = tf
-        self.format = format
-
-    def filter(self, record):
-        if record.levelno == self.format.NOTE and (record.msg.startswith("Running") or record.msg.startswith("package ")):
-            return False
-        self.tf.clearFooter()
-        return True
-
-class TerminalFilter2(object):
-    columns = 80
-
-    def sigwinch_handle(self, signum, frame):
-        self.columns = self.getTerminalColumns()
-        if self._sigwinch_default:
-            self._sigwinch_default(signum, frame)
-
-    def getTerminalColumns(self):
-        def ioctl_GWINSZ(fd):
-            try:
-                cr = struct.unpack('hh', fcntl.ioctl(fd, self.termios.TIOCGWINSZ, '1234'))
-            except:
-                return None
-            return cr
-        cr = ioctl_GWINSZ(sys.stdout.fileno())
-        if not cr:
-            try:
-                fd = os.open(os.ctermid(), os.O_RDONLY)
-                cr = ioctl_GWINSZ(fd)
-                os.close(fd)
-            except:
-                pass
-        if not cr:
-            try:
-                cr = (env['LINES'], env['COLUMNS'])
-            except:
-                cr = (25, 80)
-        return cr[1]
-
-    def __init__(self, main, helper, console, format):
-        self.main = main
-        self.helper = helper
-        self.cuu = None
-        self.stdinbackup = None
-        self.interactive = sys.stdout.isatty()
-        self.footer_present = False
-        self.lastpids = []
-
-        if not self.interactive:
-            return
-
-        import curses
-        import termios
-        self.curses = curses
-        self.termios = termios
-        try:
-            fd = sys.stdin.fileno()
-            self.stdinbackup = termios.tcgetattr(fd)
-            new = copy.deepcopy(self.stdinbackup)
-            new[3] = new[3] & ~termios.ECHO
-            termios.tcsetattr(fd, termios.TCSADRAIN, new)
-            curses.setupterm()
-            self.ed = curses.tigetstr("ed")
-            if self.ed:
-                self.cuu = curses.tigetstr("cuu")
-            try:
-                self._sigwinch_default = signal.getsignal(signal.SIGWINCH)
-                signal.signal(signal.SIGWINCH, self.sigwinch_handle)
-            except:
-                pass
-            self.columns = self.getTerminalColumns()
-        except:
-            self.cuu = None
-        console.addFilter(InteractConsoleLogFilter(self, format))
-
-    def clearFooter(self):
-        if self.footer_present:
-            lines = self.footer_present
-            sys.stdout.write(self.curses.tparm(self.cuu, lines))
-            sys.stdout.write(self.curses.tparm(self.ed))
-        self.footer_present = False
-
-    def updateFooter(self):
-        if not self.cuu:
-            return
-        activetasks = self.helper.running_tasks
-        failedtasks = self.helper.failed_tasks
-        runningpids = self.helper.running_pids
-        if self.footer_present and (self.lastpids == runningpids):
-            return
-        if self.footer_present:
-            self.clearFooter()
-        if not activetasks:
-            return
-        tasks = []
-        for t in runningpids:
-            tasks.append("%s (pid %s)" % (activetasks[t]["title"], t))
-
-        if self.main.shutdown:
-            content = "Waiting for %s running tasks to finish:" % len(activetasks)
-        else:
-            content = "Currently %s running tasks (%s of %s):" % (len(activetasks), self.helper.tasknumber_current, self.helper.tasknumber_total)
-        print content
-        lines = 1 + int(len(content) / (self.columns + 1))
-        for tasknum, task in enumerate(tasks):
-            content = "%s: %s" % (tasknum, task)
-            print content
-            lines = lines + 1 + int(len(content) / (self.columns + 1))
-        self.footer_present = lines
-        self.lastpids = runningpids[:]
-
-    def finish(self):
-        if self.stdinbackup:
-            fd = sys.stdin.fileno()
-            self.termios.tcsetattr(fd, self.termios.TCSADRAIN, self.stdinbackup)
-
-def main(server, eventHandler):
-    return bb.ui.knotty.main(server, eventHandler, TerminalFilter2)