From 3c6c0f81bf3a2edfe4240bf0a3eb0e0c75140ac5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:06 -0600 Subject: [PATCH 01/21] patman: Add a way of recording terminal output for testing When running unit tests we don't want output to go to the terminal. Provide a way of collecting it so that it can be examined by test code later. Signed-off-by: Simon Glass --- tools/patman/terminal.py | 72 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/tools/patman/terminal.py b/tools/patman/terminal.py index 963f2f891b..e78a7c14f5 100644 --- a/tools/patman/terminal.py +++ b/tools/patman/terminal.py @@ -14,6 +14,78 @@ import sys # Selection of when we want our output to be colored COLOR_IF_TERMINAL, COLOR_ALWAYS, COLOR_NEVER = range(3) +# Initially, we are set up to print to the terminal +print_test_mode = False +print_test_list = [] + +class PrintLine: + """A line of text output + + Members: + text: Text line that was printed + newline: True to output a newline after the text + colour: Text colour to use + """ + def __init__(self, text, newline, colour): + self.text = text + self.newline = newline + self.colour = colour + + def __str__(self): + return 'newline=%s, colour=%s, text=%s' % (self.newline, self.colour, + self.text) + +def Print(text='', newline=True, colour=None): + """Handle a line of output to the terminal. + + In test mode this is recorded in a list. Otherwise it is output to the + terminal. + + Args: + text: Text to print + newline: True to add a new line at the end of the text + colour: Colour to use for the text + """ + if print_test_mode: + print_test_list.append(PrintLine(text, newline, colour)) + else: + if colour: + col = Color() + text = col.Color(colour, text) + print text, + if newline: + print + +def SetPrintTestMode(): + """Go into test mode, where all printing is recorded""" + global print_test_mode + + print_test_mode = True + +def GetPrintTestLines(): + """Get a list of all lines output through Print() + + Returns: + A list of PrintLine objects + """ + global print_test_list + + ret = print_test_list + print_test_list = [] + return ret + +def EchoPrintTestLines(): + """Print out the text lines collected""" + for line in print_test_list: + if line.colour: + col = Color() + print col.Color(line.colour, line.text), + else: + print line.text, + if line.newline: + print + + class Color(object): """Conditionally wraps text in ANSI color escape sequences.""" BLACK, RED, GREEN, YELLOW, BLUE, MAGENTA, CYAN, WHITE = range(8) From 4653a8826f33df981a815f91fef7972ec1b37833 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:07 -0600 Subject: [PATCH 02/21] buildman: Send builder output through a function for testing To allow us to verify the builder's console output, send it through a function which can collect it when running in test mode. Signed-off-by: Simon Glass --- tools/buildman/builder.py | 58 ++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 324239a841..1b6517b488 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -20,6 +20,7 @@ import builderthread import command import gitutil import terminal +from terminal import Print import toolchain @@ -299,8 +300,8 @@ class Builder: length: Length of new line, in characters """ if length < self.last_line_len: - print ' ' * (self.last_line_len - length), - print '\r', + Print(' ' * (self.last_line_len - length), newline=False) + Print('\r', newline=False) self.last_line_len = length sys.stdout.flush() @@ -351,7 +352,7 @@ class Builder: if result.already_done: self.already_done += 1 if self._verbose: - print '\r', + Print('\r', newline=False) self.ClearLine(0) boards_selected = {target : result.brd} self.ResetResultSummary(boards_selected) @@ -379,7 +380,7 @@ class Builder: self.commit_count) name += target - print line + name, + Print(line + name, newline=False) length = 14 + len(name) self.ClearLine(length) @@ -495,7 +496,7 @@ class Builder: try: size, type, name = line[:-1].split() except: - print "Invalid line in file '%s': '%s'" % (fname, line[:-1]) + Print("Invalid line in file '%s': '%s'" % (fname, line[:-1])) continue if type in 'tTdDbB': # function names begin with '.' on 64-bit powerpc @@ -723,16 +724,16 @@ class Builder: return args = [self.ColourNum(x) for x in args] indent = ' ' * 15 - print ('%s%s: add: %s/%s, grow: %s/%s bytes: %s/%s (%s)' % - tuple([indent, self.col.Color(self.col.YELLOW, fname)] + args)) - print '%s %-38s %7s %7s %+7s' % (indent, 'function', 'old', 'new', - 'delta') + Print('%s%s: add: %s/%s, grow: %s/%s bytes: %s/%s (%s)' % + tuple([indent, self.col.Color(self.col.YELLOW, fname)] + args)) + Print('%s %-38s %7s %7s %+7s' % (indent, 'function', 'old', 'new', + 'delta')) for diff, name in delta: if diff: color = self.col.RED if diff > 0 else self.col.GREEN msg = '%s %-38s %7s %7s %+7d' % (indent, name, old.get(name, '-'), new.get(name,'-'), diff) - print self.col.Color(color, msg) + Print(msg, colour=color) def PrintSizeDetail(self, target_list, show_bloat): @@ -757,11 +758,12 @@ class Builder: color = self.col.RED if diff > 0 else self.col.GREEN msg = ' %s %+d' % (name, diff) if not printed_target: - print '%10s %-15s:' % ('', result['_target']), + Print('%10s %-15s:' % ('', result['_target']), + newline=False) printed_target = True - print self.col.Color(color, msg), + Print(msg, colour=color, newline=False) if printed_target: - print + Print() if show_bloat: target = result['_target'] outcome = result['_outcome'] @@ -866,13 +868,13 @@ class Builder: color = self.col.RED if avg_diff > 0 else self.col.GREEN msg = ' %s %+1.1f' % (name, avg_diff) if not printed_arch: - print '%10s: (for %d/%d boards)' % (arch, count, - arch_count[arch]), + Print('%10s: (for %d/%d boards)' % (arch, count, + arch_count[arch]), newline=False) printed_arch = True - print self.col.Color(color, msg), + Print(msg, colour=color, newline=False) if printed_arch: - print + Print() if show_detail: self.PrintSizeDetail(target_list, show_bloat) @@ -977,19 +979,19 @@ class Builder: self.AddOutcome(board_selected, arch_list, unknown, '?', self.col.MAGENTA) for arch, target_list in arch_list.iteritems(): - print '%10s: %s' % (arch, target_list) + Print('%10s: %s' % (arch, target_list)) self._error_lines += 1 if better_err: - print self.col.Color(self.col.GREEN, '\n'.join(better_err)) + Print('\n'.join(better_err), colour=self.col.GREEN) self._error_lines += 1 if worse_err: - print self.col.Color(self.col.RED, '\n'.join(worse_err)) + Print('\n'.join(worse_err), colour=self.col.RED) self._error_lines += 1 if better_warn: - print self.col.Color(self.col.YELLOW, '\n'.join(better_warn)) + Print('\n'.join(better_warn), colour=self.col.CYAN) self._error_lines += 1 if worse_warn: - print self.col.Color(self.col.MAGENTA, '\n'.join(worse_warn)) + Print('\n'.join(worse_warn), colour=self.col.MAGENTA) self._error_lines += 1 if show_sizes: @@ -1009,8 +1011,8 @@ class Builder: if not board in board_dict: not_built.append(board) if not_built: - print "Boards not built (%d): %s" % (len(not_built), - ', '.join(not_built)) + Print("Boards not built (%d): %s" % (len(not_built), + ', '.join(not_built))) def ProduceResultSummary(self, commit_upto, commits, board_selected): (board_dict, err_lines, err_line_boards, warn_lines, @@ -1020,7 +1022,7 @@ class Builder: if commits: msg = '%02d: %s' % (commit_upto + 1, commits[commit_upto].subject) - print self.col.Color(self.col.BLUE, msg) + Print(msg, colour=self.col.BLUE) self.PrintResultSummary(board_selected, board_dict, err_lines if self._show_errors else [], err_line_boards, warn_lines if self._show_errors else [], warn_line_boards, @@ -1044,7 +1046,7 @@ class Builder: for commit_upto in range(0, self.commit_count, self._step): self.ProduceResultSummary(commit_upto, commits, board_selected) if not self._error_lines: - print self.col.Color(self.col.GREEN, '(no errors to report)') + Print('(no errors to report)', colour=self.col.GREEN) def SetupBuild(self, board_selected, commits): @@ -1089,7 +1091,7 @@ class Builder: if os.path.exists(git_dir): gitutil.Fetch(git_dir, thread_dir) else: - print 'Cloning repo for thread %d' % thread_num + Print('Cloning repo for thread %d' % thread_num) gitutil.Clone(src_dir, thread_dir) def _PrepareWorkingSpace(self, max_threads, setup_git): @@ -1160,6 +1162,6 @@ class Builder: # Wait until we have processed all output self.out_queue.join() - print + Print() self.ClearLine(0) return (self.fail, self.warned) From 6208fcef9457f3c9b72c482376cfdba4c60cb728 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:08 -0600 Subject: [PATCH 03/21] buildman: Enhance basic test to check summary output Adjust the basic test so that it checks all console output. This will help to ensure that the builder is behaving correctly with printing summary information. Signed-off-by: Simon Glass --- tools/buildman/test.py | 101 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 96 insertions(+), 5 deletions(-) diff --git a/tools/buildman/test.py b/tools/buildman/test.py index a51c9429e9..f0c4d0e409 100644 --- a/tools/buildman/test.py +++ b/tools/buildman/test.py @@ -21,20 +21,21 @@ import builder import control import command import commit +import terminal import toolchain errors = [ '''main.c: In function 'main_loop': main.c:260:6: warning: unused variable 'joe' [-Wunused-variable] ''', - '''main.c: In function 'main_loop': + '''main.c: In function 'main_loop2': main.c:295:2: error: 'fred' undeclared (first use in this function) main.c:295:2: note: each undeclared identifier is reported only once for each function it appears in make[1]: *** [main.o] Error 1 make: *** [common/libcommon.o] Error 2 Make failed ''', - '''main.c: In function 'main_loop': + '''main.c: In function 'main_loop3': main.c:280:6: warning: unused variable 'mary' [-Wunused-variable] ''', '''powerpc-linux-ld: warning: dot moved backwards before `.bss' @@ -103,6 +104,10 @@ class TestBuild(unittest.TestCase): self.toolchains.Add('powerpc-linux-gcc', test=False) self.toolchains.Add('gcc', test=False) + # Avoid sending any output + terminal.SetPrintTestMode() + self._col = terminal.Color() + def Make(self, commit, brd, stage, *args, **kwargs): result = command.CommandResult() boardnum = int(brd.target[-1]) @@ -121,13 +126,26 @@ class TestBuild(unittest.TestCase): if not os.path.isdir(target_dir): os.mkdir(target_dir) - #time.sleep(.2 + boardnum * .2) result.combined = result.stdout + result.stderr return result - def testBasic(self): - """Test basic builder operation""" + def assertSummary(self, text, arch, plus, boards, ok=False): + col = self._col + expected_colour = col.GREEN if ok else col.RED + expect = '%10s: ' % arch + # TODO(sjg@chromium.org): If plus is '', we shouldn't need this + expect += col.Color(expected_colour, plus) + expect += ' ' + for board in boards: + expect += col.Color(expected_colour, ' %s' % board) + self.assertEqual(text, expect) + + def testOutput(self): + """Test basic builder operation and output + + This does a line-by-line verification of the summary output. + """ output_dir = tempfile.mkdtemp() if not os.path.isdir(output_dir): os.mkdir(output_dir) @@ -138,8 +156,81 @@ class TestBuild(unittest.TestCase): build.BuildBoards(self.commits, board_selected, keep_outputs=False, verbose=False) + lines = terminal.GetPrintTestLines() + count = 0 + for line in lines: + if line.text.strip(): + count += 1 + + # We should get one starting message, then an update for every commit + # built. + self.assertEqual(count, len(commits) * len(boards) + 1) build.SetDisplayOptions(show_errors=True); build.ShowSummary(self.commits, board_selected) + lines = terminal.GetPrintTestLines() + self.assertEqual(lines[0].text, '01: %s' % commits[0][1]) + self.assertEqual(lines[1].text, '02: %s' % commits[1][1]) + + # We expect all archs to fail + col = terminal.Color() + self.assertSummary(lines[2].text, 'sandbox', '+', ['board4']) + self.assertSummary(lines[3].text, 'arm', '+', ['board1']) + self.assertSummary(lines[4].text, 'powerpc', '+', ['board2', 'board3']) + + # Now we should have the compiler warning + self.assertEqual(lines[5].text, 'w+%s' % + errors[0].rstrip().replace('\n', '\nw+')) + self.assertEqual(lines[5].colour, col.MAGENTA) + + self.assertEqual(lines[6].text, '03: %s' % commits[2][1]) + self.assertSummary(lines[7].text, 'sandbox', '+', ['board4']) + self.assertSummary(lines[8].text, 'arm', '', ['board1'], ok=True) + self.assertSummary(lines[9].text, 'powerpc', '+', ['board2', 'board3']) + + # Compiler error + self.assertEqual(lines[10].text, '+%s' % + errors[1].rstrip().replace('\n', '\n+')) + + self.assertEqual(lines[11].text, '04: %s' % commits[3][1]) + self.assertSummary(lines[12].text, 'sandbox', '', ['board4'], ok=True) + self.assertSummary(lines[13].text, 'powerpc', '', ['board2', 'board3'], + ok=True) + + # Compile error fixed + self.assertEqual(lines[14].text, '-%s' % + errors[1].rstrip().replace('\n', '\n-')) + self.assertEqual(lines[14].colour, col.GREEN) + + self.assertEqual(lines[15].text, 'w+%s' % + errors[2].rstrip().replace('\n', '\nw+')) + self.assertEqual(lines[15].colour, col.MAGENTA) + + self.assertEqual(lines[16].text, '05: %s' % commits[4][1]) + self.assertSummary(lines[17].text, 'sandbox', '+', ['board4']) + self.assertSummary(lines[18].text, 'powerpc', '', ['board3'], ok=True) + + # The second line of errors[3] is a duplicate, so buildman will drop it + expect = errors[3].rstrip().split('\n') + expect = [expect[0]] + expect[2:] + self.assertEqual(lines[19].text, '+%s' % + '\n'.join(expect).replace('\n', '\n+')) + + self.assertEqual(lines[20].text, 'w-%s' % + errors[2].rstrip().replace('\n', '\nw-')) + + self.assertEqual(lines[21].text, '06: %s' % commits[5][1]) + self.assertSummary(lines[22].text, 'sandbox', '', ['board4'], ok=True) + + # The second line of errors[3] is a duplicate, so buildman will drop it + expect = errors[3].rstrip().split('\n') + expect = [expect[0]] + expect[2:] + self.assertEqual(lines[23].text, '-%s' % + '\n'.join(expect).replace('\n', '\n-')) + + self.assertEqual(lines[24].text, 'w-%s' % + errors[0].rstrip().replace('\n', '\nw-')) + + self.assertEqual(len(lines), 25) def _testGit(self): """Test basic builder operation by building a branch""" From ddaf5c8f3030050fcd356a1e49e3ee8f8f52c6d4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:09 -0600 Subject: [PATCH 04/21] patman: RunPipe() should not pipe stdout/stderr unless asked RunPipe() currently pipes the output of stdout and stderr to a pty, but this is not the intended behaviour. Fix it. Signed-off-by: Simon Glass --- tools/patman/command.py | 2 ++ tools/patman/gitutil.py | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/patman/command.py b/tools/patman/command.py index 449d3d0e03..7212fdfd40 100644 --- a/tools/patman/command.py +++ b/tools/patman/command.py @@ -48,6 +48,8 @@ def RunPipe(pipe_list, infile=None, outfile=None, last_pipe = None pipeline = list(pipe_list) user_pipestr = '|'.join([' '.join(pipe) for pipe in pipe_list]) + kwargs['stdout'] = None + kwargs['stderr'] = None while pipeline: cmd = pipeline.pop(0) if last_pipe is not None: diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py index 80edc7c2c6..b68df5d72e 100644 --- a/tools/patman/gitutil.py +++ b/tools/patman/gitutil.py @@ -152,7 +152,8 @@ def Checkout(commit_hash, git_dir=None, work_tree=None, force=False): if force: pipe.append('-f') pipe.append(commit_hash) - result = command.RunPipe([pipe], capture=True, raise_on_error=False) + result = command.RunPipe([pipe], capture=True, raise_on_error=False, + capture_stderr=True) if result.return_code != 0: raise OSError, 'git checkout (%s): %s' % (pipe, result.stderr) @@ -163,7 +164,8 @@ def Clone(git_dir, output_dir): commit_hash: Commit hash to check out """ pipe = ['git', 'clone', git_dir, '.'] - result = command.RunPipe([pipe], capture=True, cwd=output_dir) + result = command.RunPipe([pipe], capture=True, cwd=output_dir, + capture_stderr=True) if result.return_code != 0: raise OSError, 'git clone: %s' % result.stderr @@ -179,7 +181,7 @@ def Fetch(git_dir=None, work_tree=None): if work_tree: pipe.extend(['--work-tree', work_tree]) pipe.append('fetch') - result = command.RunPipe([pipe], capture=True) + result = command.RunPipe([pipe], capture=True, capture_stderr=True) if result.return_code != 0: raise OSError, 'git fetch: %s' % result.stderr From d3d5c1233156215c3c793e77dce72f2fdfe745f1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:10 -0600 Subject: [PATCH 05/21] buildman: Move the command line code into its own file We want to be able to issue parser commands from within buildman for test purposes. Move the parser code into its own file so we don't end up needing the buildman and test modules to reference each other. Signed-off-by: Simon Glass --- tools/buildman/buildman.py | 73 +------------------------------- tools/buildman/cmdline.py | 85 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 71 deletions(-) create mode 100644 tools/buildman/cmdline.py diff --git a/tools/buildman/buildman.py b/tools/buildman/buildman.py index 1258b760ca..c4de857d99 100755 --- a/tools/buildman/buildman.py +++ b/tools/buildman/buildman.py @@ -8,7 +8,6 @@ """See README for more information""" import multiprocessing -from optparse import OptionParser import os import re import sys @@ -22,7 +21,7 @@ sys.path.append(os.path.join(our_path, '../patman')) import board import builder import checkpatch -import command +import cmdline import control import doctest import gitutil @@ -59,75 +58,7 @@ def RunTests(): print err -parser = OptionParser() -parser.add_option('-b', '--branch', type='string', - help='Branch name to build') -parser.add_option('-B', '--bloat', dest='show_bloat', - action='store_true', default=False, - help='Show changes in function code size for each board') -parser.add_option('-c', '--count', dest='count', type='int', - default=-1, help='Run build on the top n commits') -parser.add_option('-C', '--force-reconfig', dest='force_reconfig', - action='store_true', default=False, - help='Reconfigure for every commit (disable incremental build)') -parser.add_option('-d', '--detail', dest='show_detail', - action='store_true', default=False, - help='Show detailed information for each board in summary') -parser.add_option('-e', '--show_errors', action='store_true', - default=False, help='Show errors and warnings') -parser.add_option('-f', '--force-build', dest='force_build', - action='store_true', default=False, - help='Force build of boards even if already built') -parser.add_option('-F', '--force-build-failures', dest='force_build_failures', - action='store_true', default=False, - help='Force build of previously-failed build') -parser.add_option('-g', '--git', type='string', - help='Git repo containing branch to build', default='.') -parser.add_option('-G', '--config-file', type='string', - help='Path to buildman config file', default='') -parser.add_option('-H', '--full-help', action='store_true', dest='full_help', - default=False, help='Display the README file') -parser.add_option('-i', '--in-tree', dest='in_tree', - action='store_true', default=False, - help='Build in the source tree instead of a separate directory') -parser.add_option('-j', '--jobs', dest='jobs', type='int', - default=None, help='Number of jobs to run at once (passed to make)') -parser.add_option('-k', '--keep-outputs', action='store_true', - default=False, help='Keep all build output files (e.g. binaries)') -parser.add_option('-l', '--list-error-boards', action='store_true', - default=False, help='Show a list of boards next to each error/warning') -parser.add_option('--list-tool-chains', action='store_true', default=False, - help='List available tool chains') -parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', - default=False, help="Do a try run (describe actions, but no nothing)") -parser.add_option('-o', '--output-dir', type='string', - dest='output_dir', default='..', - help='Directory where all builds happen and buildman has its workspace (default is ../)') -parser.add_option('-Q', '--quick', action='store_true', - default=False, help='Do a rough build, with limited warning resolution') -parser.add_option('-s', '--summary', action='store_true', - default=False, help='Show a build summary') -parser.add_option('-S', '--show-sizes', action='store_true', - default=False, help='Show image size variation in summary') -parser.add_option('--step', type='int', - default=1, help='Only build every n commits (0=just first and last)') -parser.add_option('-t', '--test', action='store_true', dest='test', - default=False, help='run tests') -parser.add_option('-T', '--threads', type='int', - default=None, help='Number of builder threads to use') -parser.add_option('-u', '--show_unknown', action='store_true', - default=False, help='Show boards with unknown build result') -parser.add_option('-v', '--verbose', action='store_true', - default=False, help='Show build results while the build progresses') -parser.add_option('-x', '--exclude', dest='exclude', - type='string', action='append', - help='Specify a list of boards to exclude, separated by comma') - -parser.usage += """ - -Build U-Boot for all commits in a branch. Use -n to do a dry run""" - -(options, args) = parser.parse_args() +options, args = cmdline.ParseArgs() # Run our meagre tests if options.test: diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py new file mode 100644 index 0000000000..fad9a1c3f3 --- /dev/null +++ b/tools/buildman/cmdline.py @@ -0,0 +1,85 @@ +# +# Copyright (c) 2014 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +# + +from optparse import OptionParser + +def ParseArgs(): + """Parse command line arguments from sys.argv[] + + Returns: + tuple containing: + options: command line options + args: command lin arguments + """ + parser = OptionParser() + parser.add_option('-b', '--branch', type='string', + help='Branch name to build') + parser.add_option('-B', '--bloat', dest='show_bloat', + action='store_true', default=False, + help='Show changes in function code size for each board') + parser.add_option('-c', '--count', dest='count', type='int', + default=-1, help='Run build on the top n commits') + parser.add_option('-C', '--force-reconfig', dest='force_reconfig', + action='store_true', default=False, + help='Reconfigure for every commit (disable incremental build)') + parser.add_option('-d', '--detail', dest='show_detail', + action='store_true', default=False, + help='Show detailed information for each board in summary') + parser.add_option('-e', '--show_errors', action='store_true', + default=False, help='Show errors and warnings') + parser.add_option('-f', '--force-build', dest='force_build', + action='store_true', default=False, + help='Force build of boards even if already built') + parser.add_option('-F', '--force-build-failures', dest='force_build_failures', + action='store_true', default=False, + help='Force build of previously-failed build') + parser.add_option('-g', '--git', type='string', + help='Git repo containing branch to build', default='.') + parser.add_option('-G', '--config-file', type='string', + help='Path to buildman config file', default='') + parser.add_option('-H', '--full-help', action='store_true', dest='full_help', + default=False, help='Display the README file') + parser.add_option('-i', '--in-tree', dest='in_tree', + action='store_true', default=False, + help='Build in the source tree instead of a separate directory') + parser.add_option('-j', '--jobs', dest='jobs', type='int', + default=None, help='Number of jobs to run at once (passed to make)') + parser.add_option('-k', '--keep-outputs', action='store_true', + default=False, help='Keep all build output files (e.g. binaries)') + parser.add_option('-l', '--list-error-boards', action='store_true', + default=False, help='Show a list of boards next to each error/warning') + parser.add_option('--list-tool-chains', action='store_true', default=False, + help='List available tool chains') + parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', + default=False, help="Do a try run (describe actions, but no nothing)") + parser.add_option('-o', '--output-dir', type='string', + dest='output_dir', default='..', + help='Directory where all builds happen and buildman has its workspace (default is ../)') + parser.add_option('-Q', '--quick', action='store_true', + default=False, help='Do a rough build, with limited warning resolution') + parser.add_option('-s', '--summary', action='store_true', + default=False, help='Show a build summary') + parser.add_option('-S', '--show-sizes', action='store_true', + default=False, help='Show image size variation in summary') + parser.add_option('--step', type='int', + default=1, help='Only build every n commits (0=just first and last)') + parser.add_option('-t', '--test', action='store_true', dest='test', + default=False, help='run tests') + parser.add_option('-T', '--threads', type='int', + default=None, help='Number of builder threads to use') + parser.add_option('-u', '--show_unknown', action='store_true', + default=False, help='Show boards with unknown build result') + parser.add_option('-v', '--verbose', action='store_true', + default=False, help='Show build results while the build progresses') + parser.add_option('-x', '--exclude', dest='exclude', + type='string', action='append', + help='Specify a list of boards to exclude, separated by comma') + + parser.usage += """ + + Build U-Boot for all commits in a branch. Use -n to do a dry run""" + + return parser.parse_args() From 48ba5856eb47dca0abc4d24e7c4e3ce1fd2628f1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:11 -0600 Subject: [PATCH 06/21] buildman: Move full help code into the control module There is no good reason to keep this code separate. Move it into control.py so it is easier to test. Signed-off-by: Simon Glass --- tools/buildman/buildman.py | 6 ------ tools/buildman/control.py | 8 ++++++++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/buildman/buildman.py b/tools/buildman/buildman.py index c4de857d99..70c2142901 100755 --- a/tools/buildman/buildman.py +++ b/tools/buildman/buildman.py @@ -63,12 +63,6 @@ options, args = cmdline.ParseArgs() # Run our meagre tests if options.test: RunTests() -elif options.full_help: - pager = os.getenv('PAGER') - if not pager: - pager = 'more' - fname = os.path.join(os.path.dirname(sys.argv[0]), 'README') - command.Run(pager, fname) # Build selected commits for selected boards else: diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 06c9229fba..408d9b126b 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -84,6 +84,14 @@ def DoBuildman(options, args): options: Command line options object args: Command line arguments (list of strings) """ + if options.full_help: + pager = os.getenv('PAGER') + if not pager: + pager = 'more' + fname = os.path.join(os.path.dirname(sys.argv[0]), 'README') + command.Run(pager, fname) + return 0 + gitutil.Setup() bsettings.Setup(options.config_file) From 82012dd284257ce785b1e3996a9a2519e8a4b303 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:12 -0600 Subject: [PATCH 07/21] patman: Provide a way to intercept commands for testing Add a test point for the command module. This allows tests to emulate the execution of commands. This provides more control (since we can make the fake 'commands' do whatever we like), makes it faster to write tests since we don't need to set up as much environment, and speeds up test execution. Signed-off-by: Simon Glass --- tools/patman/command.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/patman/command.py b/tools/patman/command.py index 7212fdfd40..d586f11158 100644 --- a/tools/patman/command.py +++ b/tools/patman/command.py @@ -20,9 +20,25 @@ class CommandResult: def __init__(self): self.stdout = None self.stderr = None + self.combined = None self.return_code = None self.exception = None + def __init__(self, stdout='', stderr='', combined='', return_code=0, + exception=None): + self.stdout = stdout + self.stderr = stderr + self.combined = combined + self.return_code = return_code + self.exception = exception + + +# This permits interception of RunPipe for test purposes. If it is set to +# a function, then that function is called with the pipe list being +# executed. Otherwise, it is assumed to be a CommandResult object, and is +# returned as the result for every RunPipe() call. +# When this value is None, commands are executed as normal. +test_result = None def RunPipe(pipe_list, infile=None, outfile=None, capture=False, capture_stderr=False, oneline=False, @@ -44,6 +60,10 @@ def RunPipe(pipe_list, infile=None, outfile=None, Returns: CommandResult object """ + if test_result: + if hasattr(test_result, '__call__'): + return test_result(pipe_list=pipe_list) + return test_result result = CommandResult() last_pipe = None pipeline = list(pipe_list) From d4144e45b4245c60f50d456293cad2f53373efcd Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:13 -0600 Subject: [PATCH 08/21] buildman: Add a functional test Buildman currently lacks testing in many areas, including its use of git, make and many command-line flags. Add a functional test which covers some of these areas. So far it does a fake 'build' of all boards for the current source tree. This version reads the real ~/.buildman and boards.cfg files. Future work will improve this. Signed-off-by: Simon Glass --- tools/buildman/buildman.py | 17 +--- tools/buildman/control.py | 21 ++++- tools/buildman/func_test.py | 182 ++++++++++++++++++++++++++++++++++++ tools/buildman/toolchain.py | 4 +- 4 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 tools/buildman/func_test.py diff --git a/tools/buildman/buildman.py b/tools/buildman/buildman.py index 70c2142901..6771c862d3 100755 --- a/tools/buildman/buildman.py +++ b/tools/buildman/buildman.py @@ -30,27 +30,20 @@ import terminal import toolchain def RunTests(): + import func_test import test import doctest result = unittest.TestResult() - for module in ['toolchain']: + for module in ['toolchain', 'gitutil']: suite = doctest.DocTestSuite(module) suite.run(result) - # TODO: Surely we can just 'print' result? - print result - for test, err in result.errors: - print err - for test, err in result.failures: - print err - sys.argv = [sys.argv[0]] - suite = unittest.TestLoader().loadTestsFromTestCase(test.TestBuild) - result = unittest.TestResult() - suite.run(result) + for module in (test.TestBuild, func_test.TestFunctional): + suite = unittest.TestLoader().loadTestsFromTestCase(module) + suite.run(result) - # TODO: Surely we can just 'print' result? print result for test, err in result.errors: print err diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 408d9b126b..213e235a11 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -13,6 +13,7 @@ from builder import Builder import gitutil import patchstream import terminal +from terminal import Print import toolchain import command import subprocess @@ -77,12 +78,18 @@ def ShowActions(series, why_selected, boards_selected, builder, options): print ('Total boards to build for each commit: %d\n' % why_selected['all']) -def DoBuildman(options, args): +def DoBuildman(options, args, toolchains=None, make_func=None): """The main control code for buildman Args: options: Command line options object args: Command line arguments (list of strings) + toolchains: Toolchains to use - this should be a Toolchains() + object. If None, then it will be created and scanned + make_func: Make function to use for the builder. This is called + to execute 'make'. If this is None, the normal function + will be used, which calls the 'make' tool with suitable + arguments. This setting is useful for tests. """ if options.full_help: pager = os.getenv('PAGER') @@ -97,8 +104,10 @@ def DoBuildman(options, args): bsettings.Setup(options.config_file) options.git_dir = os.path.join(options.git, '.git') - toolchains = toolchain.Toolchains() - toolchains.Scan(options.list_tool_chains) + if not toolchains: + toolchains = toolchain.Toolchains() + toolchains.GetSettings() + toolchains.Scan(options.list_tool_chains) if options.list_tool_chains: toolchains.List() print @@ -202,6 +211,8 @@ def DoBuildman(options, args): options.threads, options.jobs, gnu_make=gnu_make, checkout=True, show_unknown=options.show_unknown, step=options.step) builder.force_config_on_failure = not options.quick + if make_func: + builder.do_make = make_func # For a dry run, just show our actions as a sanity check if options.dry_run: @@ -220,8 +231,8 @@ def DoBuildman(options, args): else: commits = None - print GetActionSummary(options.summary, commits, board_selected, - options) + Print(GetActionSummary(options.summary, commits, board_selected, + options)) builder.SetDisplayOptions(options.show_errors, options.show_sizes, options.show_detail, options.show_bloat, diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py new file mode 100644 index 0000000000..8711f9c131 --- /dev/null +++ b/tools/buildman/func_test.py @@ -0,0 +1,182 @@ +# +# Copyright (c) 2014 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +# + +import os +import shutil +import sys +import tempfile +import unittest + +import cmdline +import command +import control +import gitutil +import terminal +import toolchain + +class TestFunctional(unittest.TestCase): + """Functional test for buildman. + + This aims to test from just below the invocation of buildman (parsing + of arguments) to 'make' and 'git' invocation. It is not a true + emd-to-end test, as it mocks git, make and the tool chain. But this + makes it easier to detect when the builder is doing the wrong thing, + since in many cases this test code will fail. For example, only a + very limited subset of 'git' arguments is supported - anything + unexpected will fail. + """ + def setUp(self): + self._base_dir = tempfile.mkdtemp() + self._git_dir = os.path.join(self._base_dir, 'src') + self._buildman_pathname = sys.argv[0] + self._buildman_dir = os.path.dirname(sys.argv[0]) + command.test_result = self._HandleCommand + self._toolchains = toolchain.Toolchains() + self._toolchains.Add('gcc', test=False) + + def tearDown(self): + shutil.rmtree(self._base_dir) + + def _RunBuildman(self, *args): + return command.RunPipe([[self._buildman_pathname] + list(args)], + capture=True, capture_stderr=True) + + def _RunControl(self, *args): + sys.argv = [sys.argv[0]] + list(args) + options, args = cmdline.ParseArgs() + return control.DoBuildman(options, args, toolchains=self._toolchains, + make_func=self._HandleMake) + + def testFullHelp(self): + command.test_result = None + result = self._RunBuildman('-H') + help_file = os.path.join(self._buildman_dir, 'README') + self.assertEqual(len(result.stdout), os.path.getsize(help_file)) + self.assertEqual(0, len(result.stderr)) + self.assertEqual(0, result.return_code) + + def testHelp(self): + command.test_result = None + result = self._RunBuildman('-h') + help_file = os.path.join(self._buildman_dir, 'README') + self.assertTrue(len(result.stdout) > 1000) + self.assertEqual(0, len(result.stderr)) + self.assertEqual(0, result.return_code) + + def testGitSetup(self): + """Test gitutils.Setup(), from outside the module itself""" + command.test_result = command.CommandResult(return_code=1) + gitutil.Setup() + self.assertEqual(gitutil.use_no_decorate, False) + + command.test_result = command.CommandResult(return_code=0) + gitutil.Setup() + self.assertEqual(gitutil.use_no_decorate, True) + + def _HandleCommandGitLog(self, args): + if '-n0' in args: + return command.CommandResult(return_code=0) + + # Not handled, so abort + print 'git log', args + sys.exit(1) + + def _HandleCommandGit(self, in_args): + """Handle execution of a git command + + This uses a hacked-up parser. + + Args: + in_args: Arguments after 'git' from the command line + """ + git_args = [] # Top-level arguments to git itself + sub_cmd = None # Git sub-command selected + args = [] # Arguments to the git sub-command + for arg in in_args: + if sub_cmd: + args.append(arg) + elif arg[0] == '-': + git_args.append(arg) + else: + sub_cmd = arg + if sub_cmd == 'config': + return command.CommandResult(return_code=0) + elif sub_cmd == 'log': + return self._HandleCommandGitLog(args) + + # Not handled, so abort + print 'git', git_args, sub_cmd, args + sys.exit(1) + + def _HandleCommandNm(self, args): + return command.CommandResult(return_code=0) + + def _HandleCommandObjdump(self, args): + return command.CommandResult(return_code=0) + + def _HandleCommandSize(self, args): + return command.CommandResult(return_code=0) + + def _HandleCommand(self, **kwargs): + """Handle a command execution. + + The command is in kwargs['pipe-list'], as a list of pipes, each a + list of commands. The command should be emulated as required for + testing purposes. + + Returns: + A CommandResult object + """ + pipe_list = kwargs['pipe_list'] + if len(pipe_list) != 1: + print 'invalid pipe', kwargs + sys.exit(1) + cmd = pipe_list[0][0] + args = pipe_list[0][1:] + if cmd == 'git': + return self._HandleCommandGit(args) + elif cmd == './scripts/show-gnu-make': + return command.CommandResult(return_code=0, stdout='make') + elif cmd == 'nm': + return self._HandleCommandNm(args) + elif cmd == 'objdump': + return self._HandleCommandObjdump(args) + elif cmd == 'size': + return self._HandleCommandSize(args) + + # Not handled, so abort + print 'unknown command', kwargs + sys.exit(1) + return command.CommandResult(return_code=0) + + def _HandleMake(self, commit, brd, stage, cwd, *args, **kwargs): + """Handle execution of 'make' + + Args: + commit: Commit object that is being built + brd: Board object that is being built + stage: Stage that we are at (mrproper, config, build) + cwd: Directory where make should be run + args: Arguments to pass to make + kwargs: Arguments to pass to command.RunPipe() + """ + if stage == 'mrproper': + return command.CommandResult(return_code=0) + elif stage == 'config': + return command.CommandResult(return_code=0, + combined='Test configuration complete') + elif stage == 'build': + return command.CommandResult(return_code=0) + + # Not handled, so abort + print 'make', stage + sys.exit(1) + + def testCurrentSource(self): + """Very simple test to invoke buildman on the current source""" + self._RunControl() + lines = terminal.GetPrintTestLines() + self.assertTrue(lines[0].text.startswith('Building current source')) diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py index 0e9129437f..27dc31889b 100644 --- a/tools/buildman/toolchain.py +++ b/tools/buildman/toolchain.py @@ -99,6 +99,9 @@ class Toolchains: def __init__(self): self.toolchains = {} self.paths = [] + self._make_flags = dict(bsettings.GetItems('make-flags')) + + def GetSettings(self): toolchains = bsettings.GetItems('toolchain') if not toolchains: print ("Warning: No tool chains - please add a [toolchain] section" @@ -110,7 +113,6 @@ class Toolchains: self.paths += glob.glob(value) else: self.paths.append(value) - self._make_flags = dict(bsettings.GetItems('make-flags')) def Add(self, fname, test=True, verbose=False): """Add a toolchain to our list From fd03d63f347e28e4e4394245210f048c82c9b085 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:14 -0600 Subject: [PATCH 09/21] buildman: Set up bsettings outside the control module Move the bsettings code back to the main buildman.py file, so we can do something different when testing. Signed-off-by: Simon Glass --- tools/buildman/buildman.py | 2 ++ tools/buildman/control.py | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/buildman/buildman.py b/tools/buildman/buildman.py index 6771c862d3..d0afeda6c0 100755 --- a/tools/buildman/buildman.py +++ b/tools/buildman/buildman.py @@ -19,6 +19,7 @@ sys.path.append(os.path.join(our_path, '../patman')) # Our modules import board +import bsettings import builder import checkpatch import cmdline @@ -59,5 +60,6 @@ if options.test: # Build selected commits for selected boards else: + bsettings.Setup(options.config_file) ret_code = control.DoBuildman(options, args) sys.exit(ret_code) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 213e235a11..c8eeb6ac45 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -101,7 +101,6 @@ def DoBuildman(options, args, toolchains=None, make_func=None): gitutil.Setup() - bsettings.Setup(options.config_file) options.git_dir = os.path.join(options.git, '.git') if not toolchains: From 8b985eebd0f7582614399fdf6c108a81ab446ae7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:15 -0600 Subject: [PATCH 10/21] buildman: Avoid looking at config file or toolchains in tests These files may not exist in the environment, or may not be suitable for testing. Provide our own config file and our own toolchains when running tests. Signed-off-by: Simon Glass --- tools/buildman/bsettings.py | 15 ++++++++++----- tools/buildman/func_test.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py index 916479866c..fdd875b073 100644 --- a/tools/buildman/bsettings.py +++ b/tools/buildman/bsettings.py @@ -5,6 +5,7 @@ import ConfigParser import os +import StringIO def Setup(fname=''): @@ -17,11 +18,15 @@ def Setup(fname=''): global config_fname settings = ConfigParser.SafeConfigParser() - config_fname = fname - if config_fname == '': - config_fname = '%s/.buildman' % os.getenv('HOME') - if config_fname: - settings.read(config_fname) + if fname is not None: + config_fname = fname + if config_fname == '': + config_fname = '%s/.buildman' % os.getenv('HOME') + if config_fname: + settings.read(config_fname) + +def AddFile(data): + settings.readfp(StringIO.StringIO(data)) def GetItems(section): """Get the items from a section of the config. diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index 8711f9c131..b92cde3607 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -10,6 +10,7 @@ import sys import tempfile import unittest +import bsettings import cmdline import command import control @@ -17,6 +18,22 @@ import gitutil import terminal import toolchain +settings_data = ''' +# Buildman settings file + +[toolchain] + +[toolchain-alias] + +[make-flags] +src=/home/sjg/c/src +chroot=/home/sjg/c/chroot +vboot=USE_STDINT=1 VBOOT_DEBUG=1 MAKEFLAGS_VBOOT=DEBUG=1 CFLAGS_EXTRA_VBOOT=-DUNROLL_LOOPS VBOOT_SOURCE=${src}/platform/vboot_reference +chromeos_coreboot=VBOOT=${chroot}/build/link/usr ${vboot} +chromeos_daisy=VBOOT=${chroot}/build/daisy/usr ${vboot} +chromeos_peach=VBOOT=${chroot}/build/peach_pit/usr ${vboot} +''' + class TestFunctional(unittest.TestCase): """Functional test for buildman. @@ -36,6 +53,8 @@ class TestFunctional(unittest.TestCase): command.test_result = self._HandleCommand self._toolchains = toolchain.Toolchains() self._toolchains.Add('gcc', test=False) + bsettings.Setup(None) + bsettings.AddFile(settings_data) def tearDown(self): shutil.rmtree(self._base_dir) From 823e60b62a98061a662536093d46f8f26f6d318f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:16 -0600 Subject: [PATCH 11/21] buildman: Allow tests to have their own boards Rather than reading boards.cfg, which may take time to generate and is not necessarily suitable for running tests, create our own list of boards. Signed-off-by: Simon Glass --- tools/buildman/control.py | 19 +++++++++++-------- tools/buildman/func_test.py | 20 +++++++++++++++++++- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index c8eeb6ac45..fb15ae89c8 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -78,7 +78,7 @@ def ShowActions(series, why_selected, boards_selected, builder, options): print ('Total boards to build for each commit: %d\n' % why_selected['all']) -def DoBuildman(options, args, toolchains=None, make_func=None): +def DoBuildman(options, args, toolchains=None, make_func=None, boards=None): """The main control code for buildman Args: @@ -90,6 +90,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None): to execute 'make'. If this is None, the normal function will be used, which calls the 'make' tool with suitable arguments. This setting is useful for tests. + board: Boards() object to use, containing a list of available + boards. If this is None it will be created and scanned. """ if options.full_help: pager = os.getenv('PAGER') @@ -135,14 +137,15 @@ def DoBuildman(options, args, toolchains=None, make_func=None): sys.exit(col.Color(col.RED, str)) # Work out what subset of the boards we are building - board_file = os.path.join(options.git, 'boards.cfg') - status = subprocess.call([os.path.join(options.git, - 'tools/genboardscfg.py')]) - if status != 0: - sys.exit("Failed to generate boards.cfg") + if not boards: + board_file = os.path.join(options.git, 'boards.cfg') + status = subprocess.call([os.path.join(options.git, + 'tools/genboardscfg.py')]) + if status != 0: + sys.exit("Failed to generate boards.cfg") - boards = board.Boards() - boards.ReadBoards(os.path.join(options.git, 'boards.cfg')) + boards = board.Boards() + boards.ReadBoards(os.path.join(options.git, 'boards.cfg')) exclude = [] if options.exclude: diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index b92cde3607..237a80b430 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -10,6 +10,7 @@ import sys import tempfile import unittest +import board import bsettings import cmdline import command @@ -34,6 +35,14 @@ chromeos_daisy=VBOOT=${chroot}/build/daisy/usr ${vboot} chromeos_peach=VBOOT=${chroot}/build/peach_pit/usr ${vboot} ''' +boards = [ + ['Active', 'arm', 'armv7', '', 'Tester', 'ARM Board 1', 'board0', ''], + ['Active', 'arm', 'armv7', '', 'Tester', 'ARM Board 2', 'board1', ''], + ['Active', 'powerpc', 'powerpc', '', 'Tester', 'PowerPC board 1', 'board2', ''], + ['Active', 'powerpc', 'mpc5xx', '', 'Tester', 'PowerPC board 2', 'board3', ''], + ['Active', 'sandbox', 'sandbox', '', 'Tester', 'Sandbox board', 'board4', ''], +] + class TestFunctional(unittest.TestCase): """Functional test for buildman. @@ -55,6 +64,9 @@ class TestFunctional(unittest.TestCase): self._toolchains.Add('gcc', test=False) bsettings.Setup(None) bsettings.AddFile(settings_data) + self._boards = board.Boards() + for brd in boards: + self._boards.AddBoard(board.Board(*brd)) def tearDown(self): shutil.rmtree(self._base_dir) @@ -67,7 +79,7 @@ class TestFunctional(unittest.TestCase): sys.argv = [sys.argv[0]] + list(args) options, args = cmdline.ParseArgs() return control.DoBuildman(options, args, toolchains=self._toolchains, - make_func=self._HandleMake) + make_func=self._HandleMake, boards=self._boards) def testFullHelp(self): command.test_result = None @@ -194,6 +206,12 @@ class TestFunctional(unittest.TestCase): print 'make', stage sys.exit(1) + def testNoBoards(self): + """Test that buildman aborts when there are no boards""" + self._boards = board.Boards() + with self.assertRaises(SystemExit): + self._RunControl() + def testCurrentSource(self): """Very simple test to invoke buildman on the current source""" self._RunControl() From fb3954f9ea444100be70f175bbedb685e397c540 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:17 -0600 Subject: [PATCH 12/21] buildman: Correct counting of build failures on retry When a build is to be performed, buildman checks to see if it has already been done. In most cases it will not bother trying again. However, it was not reading the return code from the 'done' file, so if the result was a failure, it would not be counted. This depresses the 'failure' count stats that buildman prints in this case. Fix this bug by always reading the return code. Signed-off-by: Simon Glass --- tools/buildman/builderthread.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 0246375bfa..261919f127 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -138,16 +138,17 @@ class BuilderThread(threading.Thread): result.already_done = os.path.exists(done_file) will_build = (force_build or force_build_failures or not result.already_done) - if result.already_done and will_build: + if result.already_done: # Get the return code from that build and use it with open(done_file, 'r') as fd: result.return_code = int(fd.readline()) - err_file = self.builder.GetErrFile(commit_upto, brd.target) - if os.path.exists(err_file) and os.stat(err_file).st_size: - result.stderr = 'bad' - elif not force_build: - # The build passed, so no need to build it again - will_build = False + if will_build: + err_file = self.builder.GetErrFile(commit_upto, brd.target) + if os.path.exists(err_file) and os.stat(err_file).st_size: + result.stderr = 'bad' + elif not force_build: + # The build passed, so no need to build it again + will_build = False if will_build: # We are going to have to build it. First, get a toolchain From 883a321a4bee2d2b2f16e445060c17494e76bc48 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:18 -0600 Subject: [PATCH 13/21] buildman: Provide an internal option to clean the outpur dir For testing it is useful to clean the output directory before running a test. This avoids a test interfering with the results of a subsequent test by leaving data around. Add this feature as an optional parameter to the control logic. Signed-off-by: Simon Glass --- tools/buildman/control.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index fb15ae89c8..ee9637591c 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -5,6 +5,7 @@ import multiprocessing import os +import shutil import sys import board @@ -78,7 +79,8 @@ def ShowActions(series, why_selected, boards_selected, builder, options): print ('Total boards to build for each commit: %d\n' % why_selected['all']) -def DoBuildman(options, args, toolchains=None, make_func=None, boards=None): +def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, + clean_dir=False): """The main control code for buildman Args: @@ -93,6 +95,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None): board: Boards() object to use, containing a list of available boards. If this is None it will be created and scanned. """ + global builder + if options.full_help: pager = os.getenv('PAGER') if not pager: @@ -209,6 +213,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None): else: dirname = 'current' output_dir = os.path.join(options.output_dir, dirname) + if clean_dir and os.path.exists(output_dir): + shutil.rmtree(output_dir) builder = Builder(toolchains, output_dir, options.git_dir, options.threads, options.jobs, gnu_make=gnu_make, checkout=True, show_unknown=options.show_unknown, step=options.step) @@ -230,6 +236,9 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None): if series: commits = series.commits + # Number the commits for test purposes + for commit in range(len(commits)): + commits[commit].sequence = commit else: commits = None From 891b7a076146db7984388e5b8388101eb4f44347 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:19 -0600 Subject: [PATCH 14/21] patman: Start with a clean series when needed For reasons that are not well-understood, GetMetaDataForList() can end up adding to an existing series even when it appears that it should be starting a new one. Change from using a default constructor parameter to an explicit one, to work around this problem. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index b0b81534bf..b3e66c32a9 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -355,7 +355,7 @@ class PatchStream: def GetMetaDataForList(commit_range, git_dir=None, count=None, - series = Series()): + series = None): """Reads out patch series metadata from the commits This does a 'git log' on the relevant commits and pulls out the tags we @@ -370,6 +370,8 @@ def GetMetaDataForList(commit_range, git_dir=None, count=None, Returns: A Series object containing information about the commits. """ + if not series: + series = Series() params = gitutil.LogCmd(commit_range,reverse=True, count=count, git_dir=git_dir) stdout = command.RunPipe([params], capture=True).stdout From dfb7e932350f3afee230d733e32335fe3c9b96b1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:20 -0600 Subject: [PATCH 15/21] buildman: Add additional functional tests This adds coverage of core features of the builder, including the command-line options which affect building. Signed-off-by: Simon Glass --- tools/buildman/func_test.py | 324 ++++++++++++++++++++++++++++++++++-- 1 file changed, 306 insertions(+), 18 deletions(-) diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index 237a80b430..2cb5cf05fc 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -43,6 +43,125 @@ boards = [ ['Active', 'sandbox', 'sandbox', '', 'Tester', 'Sandbox board', 'board4', ''], ] +commit_shortlog = """4aca821 patman: Avoid changing the order of tags +39403bb patman: Use --no-pager' to stop git from forking a pager +db6e6f2 patman: Remove the -a option +f2ccf03 patman: Correct unit tests to run correctly +1d097f9 patman: Fix indentation in terminal.py +d073747 patman: Support the 'reverse' option for 'git log +""" + +commit_log = ["""commit 7f6b8315d18f683c5181d0c3694818c1b2a20dcd +Author: Masahiro Yamada +Date: Fri Aug 22 19:12:41 2014 +0900 + + buildman: refactor help message + + "buildman [options]" is displayed by default. + + Append the rest of help messages to parser.usage + instead of replacing it. + + Besides, "-b " is not mandatory since commit fea5858e. + Drop it from the usage. + + Signed-off-by: Masahiro Yamada +""", +"""commit d0737479be6baf4db5e2cdbee123e96bc5ed0ba8 +Author: Simon Glass +Date: Thu Aug 14 16:48:25 2014 -0600 + + patman: Support the 'reverse' option for 'git log' + + This option is currently not supported, but needs to be, for buildman to + operate as expected. + + Series-changes: 7 + - Add new patch to fix the 'reverse' bug + + + Change-Id: I79078f792e8b390b8a1272a8023537821d45feda + Reported-by: York Sun + Signed-off-by: Simon Glass + +""", +"""commit 1d097f9ab487c5019152fd47bda126839f3bf9fc +Author: Simon Glass +Date: Sat Aug 9 11:44:32 2014 -0600 + + patman: Fix indentation in terminal.py + + This code came from a different project with 2-character indentation. Fix + it for U-Boot. + + Series-changes: 6 + - Add new patch to fix indentation in teminal.py + + Change-Id: I5a74d2ebbb3cc12a665f5c725064009ac96e8a34 + Signed-off-by: Simon Glass + +""", +"""commit f2ccf03869d1e152c836515a3ceb83cdfe04a105 +Author: Simon Glass +Date: Sat Aug 9 11:08:24 2014 -0600 + + patman: Correct unit tests to run correctly + + It seems that doctest behaves differently now, and some of the unit tests + do not run. Adjust the tests to work correctly. + + ./tools/patman/patman --test + + + Series-changes: 6 + - Add new patch to fix patman unit tests + + Change-Id: I3d2ca588f4933e1f9d6b1665a00e4ae58269ff3b + +""", +"""commit db6e6f2f9331c5a37647d6668768d4a40b8b0d1c +Author: Simon Glass +Date: Sat Aug 9 12:06:02 2014 -0600 + + patman: Remove the -a option + + It seems that this is no longer needed, since checkpatch.pl will catch + whitespace problems in patches. Also the option is not widely used, so + it seems safe to just remove it. + + Series-changes: 6 + - Add new patch to remove patman's -a option + + Suggested-by: Masahiro Yamada + Change-Id: I5821a1c75154e532c46513486ca40b808de7e2cc + +""", +"""commit 39403bb4f838153028a6f21ca30bf100f3791133 +Author: Simon Glass +Date: Thu Aug 14 21:50:52 2014 -0600 + + patman: Use --no-pager' to stop git from forking a pager + +""", +"""commit 4aca821e27e97925c039e69fd37375b09c6f129c +Author: Simon Glass +Date: Fri Aug 22 15:57:39 2014 -0600 + + patman: Avoid changing the order of tags + + patman collects tags that it sees in the commit and places them nicely + sorted at the end of the patch. However, this is not really necessary and + in fact is apparently not desirable. + + Series-changes: 9 + - Add new patch to avoid changing the order of tags + + Suggested-by: Masahiro Yamada + Change-Id: Ib1518588c1a189ad5c3198aae76f8654aed8d0db +"""] + +TEST_BRANCH = '__testbranch' + class TestFunctional(unittest.TestCase): """Functional test for buildman. @@ -60,26 +179,49 @@ class TestFunctional(unittest.TestCase): self._buildman_pathname = sys.argv[0] self._buildman_dir = os.path.dirname(sys.argv[0]) command.test_result = self._HandleCommand - self._toolchains = toolchain.Toolchains() - self._toolchains.Add('gcc', test=False) + self.setupToolchains() + self._toolchains.Add('arm-gcc', test=False) + self._toolchains.Add('powerpc-gcc', test=False) bsettings.Setup(None) bsettings.AddFile(settings_data) self._boards = board.Boards() for brd in boards: self._boards.AddBoard(board.Board(*brd)) + # Directories where the source been cloned + self._clone_dirs = [] + self._commits = len(commit_shortlog.splitlines()) + 1 + self._total_builds = self._commits * len(boards) + + # Number of calls to make + self._make_calls = 0 + + # Map of [board, commit] to error messages + self._error = {} + + # Avoid sending any output and clear all terminal output + terminal.SetPrintTestMode() + terminal.GetPrintTestLines() + def tearDown(self): shutil.rmtree(self._base_dir) + def setupToolchains(self): + self._toolchains = toolchain.Toolchains() + self._toolchains.Add('gcc', test=False) + def _RunBuildman(self, *args): return command.RunPipe([[self._buildman_pathname] + list(args)], capture=True, capture_stderr=True) - def _RunControl(self, *args): + def _RunControl(self, *args, **kwargs): sys.argv = [sys.argv[0]] + list(args) options, args = cmdline.ParseArgs() - return control.DoBuildman(options, args, toolchains=self._toolchains, - make_func=self._HandleMake, boards=self._boards) + result = control.DoBuildman(options, args, toolchains=self._toolchains, + make_func=self._HandleMake, boards=self._boards, + clean_dir=kwargs.get('clean_dir', True)) + self._builder = control.builder + return result def testFullHelp(self): command.test_result = None @@ -110,11 +252,34 @@ class TestFunctional(unittest.TestCase): def _HandleCommandGitLog(self, args): if '-n0' in args: return command.CommandResult(return_code=0) + elif args[-1] == 'upstream/master..%s' % TEST_BRANCH: + return command.CommandResult(return_code=0, stdout=commit_shortlog) + elif args[:3] == ['--no-color', '--no-decorate', '--reverse']: + if args[-1] == TEST_BRANCH: + count = int(args[3][2:]) + return command.CommandResult(return_code=0, + stdout=''.join(commit_log[:count])) # Not handled, so abort print 'git log', args sys.exit(1) + def _HandleCommandGitConfig(self, args): + config = args[0] + if config == 'sendemail.aliasesfile': + return command.CommandResult(return_code=0) + elif config.startswith('branch.badbranch'): + return command.CommandResult(return_code=1) + elif config == 'branch.%s.remote' % TEST_BRANCH: + return command.CommandResult(return_code=0, stdout='upstream\n') + elif config == 'branch.%s.merge' % TEST_BRANCH: + return command.CommandResult(return_code=0, + stdout='refs/heads/master\n') + + # Not handled, so abort + print 'git config', args + sys.exit(1) + def _HandleCommandGit(self, in_args): """Handle execution of a git command @@ -132,11 +297,18 @@ class TestFunctional(unittest.TestCase): elif arg[0] == '-': git_args.append(arg) else: - sub_cmd = arg + if git_args and git_args[-1] in ['--git-dir', '--work-tree']: + git_args.append(arg) + else: + sub_cmd = arg if sub_cmd == 'config': - return command.CommandResult(return_code=0) + return self._HandleCommandGitConfig(args) elif sub_cmd == 'log': return self._HandleCommandGitLog(args) + elif sub_cmd == 'clone': + return command.CommandResult(return_code=0) + elif sub_cmd == 'checkout': + return command.CommandResult(return_code=0) # Not handled, so abort print 'git', git_args, sub_cmd, args @@ -162,26 +334,35 @@ class TestFunctional(unittest.TestCase): A CommandResult object """ pipe_list = kwargs['pipe_list'] + wc = False if len(pipe_list) != 1: - print 'invalid pipe', kwargs - sys.exit(1) + if pipe_list[1] == ['wc', '-l']: + wc = True + else: + print 'invalid pipe', kwargs + sys.exit(1) cmd = pipe_list[0][0] args = pipe_list[0][1:] + result = None if cmd == 'git': - return self._HandleCommandGit(args) + result = self._HandleCommandGit(args) elif cmd == './scripts/show-gnu-make': return command.CommandResult(return_code=0, stdout='make') - elif cmd == 'nm': + elif cmd.endswith('nm'): return self._HandleCommandNm(args) - elif cmd == 'objdump': + elif cmd.endswith('objdump'): return self._HandleCommandObjdump(args) - elif cmd == 'size': + elif cmd.endswith( 'size'): return self._HandleCommandSize(args) - # Not handled, so abort - print 'unknown command', kwargs - sys.exit(1) - return command.CommandResult(return_code=0) + if not result: + # Not handled, so abort + print 'unknown command', kwargs + sys.exit(1) + + if wc: + result.stdout = len(result.stdout.splitlines()) + return result def _HandleMake(self, commit, brd, stage, cwd, *args, **kwargs): """Handle execution of 'make' @@ -194,18 +375,31 @@ class TestFunctional(unittest.TestCase): args: Arguments to pass to make kwargs: Arguments to pass to command.RunPipe() """ + self._make_calls += 1 if stage == 'mrproper': return command.CommandResult(return_code=0) elif stage == 'config': return command.CommandResult(return_code=0, combined='Test configuration complete') elif stage == 'build': + stderr = '' + if type(commit) is not str: + stderr = self._error.get((brd.target, commit.sequence)) + if stderr: + return command.CommandResult(return_code=1, stderr=stderr) return command.CommandResult(return_code=0) # Not handled, so abort print 'make', stage sys.exit(1) + # Example function to print output lines + def print_lines(self, lines): + print len(lines) + for line in lines: + print line + #self.print_lines(terminal.GetPrintTestLines()) + def testNoBoards(self): """Test that buildman aborts when there are no boards""" self._boards = board.Boards() @@ -214,6 +408,100 @@ class TestFunctional(unittest.TestCase): def testCurrentSource(self): """Very simple test to invoke buildman on the current source""" + self.setupToolchains(); self._RunControl() lines = terminal.GetPrintTestLines() - self.assertTrue(lines[0].text.startswith('Building current source')) + self.assertIn('Building current source for %d boards' % len(boards), + lines[0].text) + + def testBadBranch(self): + """Test that we can detect an invalid branch""" + with self.assertRaises(ValueError): + self._RunControl('-b', 'badbranch') + + def testBadToolchain(self): + """Test that missing toolchains are detected""" + self.setupToolchains(); + ret_code = self._RunControl('-b', TEST_BRANCH) + lines = terminal.GetPrintTestLines() + + # Buildman always builds the upstream commit as well + self.assertIn('Building %d commits for %d boards' % + (self._commits, len(boards)), lines[0].text) + self.assertEqual(self._builder.count, self._total_builds) + + # Only sandbox should succeed, the others don't have toolchains + self.assertEqual(self._builder.fail, + self._total_builds - self._commits) + self.assertEqual(ret_code, 128) + + for commit in range(self._commits): + for board in self._boards.GetList(): + if board.arch != 'sandbox': + errfile = self._builder.GetErrFile(commit, board.target) + fd = open(errfile) + self.assertEqual(fd.readlines(), + ['No tool chain for %s\n' % board.arch]) + fd.close() + + def testBranch(self): + """Test building a branch with all toolchains present""" + self._RunControl('-b', TEST_BRANCH) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._builder.fail, 0) + + def testCount(self): + """Test building a specific number of commitst""" + self._RunControl('-b', TEST_BRANCH, '-c2') + self.assertEqual(self._builder.count, 2 * len(boards)) + self.assertEqual(self._builder.fail, 0) + # Each board has a mrproper, config, and then one make per commit + self.assertEqual(self._make_calls, len(boards) * (2 + 2)) + + def testIncremental(self): + """Test building a branch twice - the second time should do nothing""" + self._RunControl('-b', TEST_BRANCH) + + # Each board has a mrproper, config, and then one make per commit + self.assertEqual(self._make_calls, len(boards) * (self._commits + 2)) + self._make_calls = 0 + self._RunControl('-b', TEST_BRANCH, clean_dir=False) + self.assertEqual(self._make_calls, 0) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._builder.fail, 0) + + def testForceBuild(self): + """The -f flag should force a rebuild""" + self._RunControl('-b', TEST_BRANCH) + self._make_calls = 0 + self._RunControl('-b', TEST_BRANCH, '-f', clean_dir=False) + # Each board has a mrproper, config, and then one make per commit + self.assertEqual(self._make_calls, len(boards) * (self._commits + 2)) + + def testForceReconfigure(self): + """The -f flag should force a rebuild""" + self._RunControl('-b', TEST_BRANCH, '-C') + # Each commit has a mrproper, config and make + self.assertEqual(self._make_calls, len(boards) * self._commits * 3) + + def testErrors(self): + """Test handling of build errors""" + self._error['board2', 1] = 'fred\n' + self._RunControl('-b', TEST_BRANCH) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._builder.fail, 1) + + # Remove the error. This should have no effect since the commit will + # not be rebuilt + del self._error['board2', 1] + self._make_calls = 0 + self._RunControl('-b', TEST_BRANCH, clean_dir=False) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._make_calls, 0) + self.assertEqual(self._builder.fail, 1) + + # Now use the -F flag to force rebuild of the bad commit + self._RunControl('-b', TEST_BRANCH, '-F', clean_dir=False) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._builder.fail, 0) + self.assertEqual(self._make_calls, 3) From 930c8d4ad8d179bce2426f9fca8edac904bafddc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:21 -0600 Subject: [PATCH 16/21] buildman: Expand output test to cover directory prefixes Now that buildman supports removing the build directory prefix from output, add a test for it. Also ensure that output directories are removed when the test completes. Signed-off-by: Simon Glass --- tools/buildman/test.py | 54 +++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/tools/buildman/test.py b/tools/buildman/test.py index f0c4d0e409..a2a85ac9ce 100644 --- a/tools/buildman/test.py +++ b/tools/buildman/test.py @@ -46,6 +46,20 @@ powerpc-linux-ld: u-boot: section .reloc lma 0xffffa400 overlaps previous sectio powerpc-linux-ld: u-boot: section .data lma 0xffffcd38 overlaps previous sections powerpc-linux-ld: u-boot: section .u_boot_cmd lma 0xffffeb40 overlaps previous sections powerpc-linux-ld: u-boot: section .bootpg lma 0xfffff198 overlaps previous sections +''', + '''In file included from %(basedir)sarch/sandbox/cpu/cpu.c:9:0: +%(basedir)sarch/sandbox/include/asm/state.h:44:0: warning: "xxxx" redefined [enabled by default] +%(basedir)sarch/sandbox/include/asm/state.h:43:0: note: this is the location of the previous definition +%(basedir)sarch/sandbox/cpu/cpu.c: In function 'do_reset': +%(basedir)sarch/sandbox/cpu/cpu.c:27:1: error: unknown type name 'blah' +%(basedir)sarch/sandbox/cpu/cpu.c:28:12: error: expected declaration specifiers or '...' before numeric constant +make[2]: *** [arch/sandbox/cpu/cpu.o] Error 1 +make[1]: *** [arch/sandbox/cpu] Error 2 +make[1]: *** Waiting for unfinished jobs.... +In file included from %(basedir)scommon/board_f.c:55:0: +%(basedir)sarch/sandbox/include/asm/state.h:44:0: warning: "xxxx" redefined [enabled by default] +%(basedir)sarch/sandbox/include/asm/state.h:43:0: note: this is the location of the previous definition +make: *** [sub-make] Error 2 ''' ] @@ -57,7 +71,8 @@ commits = [ ['9012', 'Third commit, error', 1, errors[0:2]], ['3456', 'Fourth commit, warning', 0, [errors[0], errors[2]]], ['7890', 'Fifth commit, link errors', 1, [errors[0], errors[3]]], - ['abcd', 'Sixth commit, fixes all errors', 0, []] + ['abcd', 'Sixth commit, fixes all errors', 0, []], + ['ef01', 'Seventh commit, check directory suppression', 1, [errors[4]]], ] boards = [ @@ -109,15 +124,19 @@ class TestBuild(unittest.TestCase): self._col = terminal.Color() def Make(self, commit, brd, stage, *args, **kwargs): + global base_dir + result = command.CommandResult() boardnum = int(brd.target[-1]) result.return_code = 0 result.stderr = '' result.stdout = ('This is the test output for board %s, commit %s' % (brd.target, commit.hash)) - if boardnum >= 1 and boardnum >= commit.sequence: + if ((boardnum >= 1 and boardnum >= commit.sequence) or + boardnum == 4 and commit.sequence == 6): result.return_code = commit.return_code - result.stderr = ''.join(commit.error_list) + result.stderr = (''.join(commit.error_list) + % {'basedir' : base_dir + '/.bm-work/00/'}) if stage == 'build': target_dir = None for arg in args: @@ -146,10 +165,12 @@ class TestBuild(unittest.TestCase): This does a line-by-line verification of the summary output. """ - output_dir = tempfile.mkdtemp() - if not os.path.isdir(output_dir): - os.mkdir(output_dir) - build = builder.Builder(self.toolchains, output_dir, None, 1, 2, + global base_dir + + base_dir = tempfile.mkdtemp() + if not os.path.isdir(base_dir): + os.mkdir(base_dir) + build = builder.Builder(self.toolchains, base_dir, None, 1, 2, checkout=False, show_unknown=False) build.do_make = self.Make board_selected = self.boards.GetSelectedDict() @@ -167,6 +188,7 @@ class TestBuild(unittest.TestCase): self.assertEqual(count, len(commits) * len(boards) + 1) build.SetDisplayOptions(show_errors=True); build.ShowSummary(self.commits, board_selected) + #terminal.EchoPrintTestLines() lines = terminal.GetPrintTestLines() self.assertEqual(lines[0].text, '01: %s' % commits[0][1]) self.assertEqual(lines[1].text, '02: %s' % commits[1][1]) @@ -230,7 +252,22 @@ class TestBuild(unittest.TestCase): self.assertEqual(lines[24].text, 'w-%s' % errors[0].rstrip().replace('\n', '\nw-')) - self.assertEqual(len(lines), 25) + self.assertEqual(lines[25].text, '07: %s' % commits[6][1]) + self.assertSummary(lines[26].text, 'sandbox', '+', ['board4']) + + # Pick out the correct error lines + expect_str = errors[4].rstrip().replace('%(basedir)s', '').split('\n') + expect = expect_str[3:8] + [expect_str[-1]] + self.assertEqual(lines[27].text, '+%s' % + '\n'.join(expect).replace('\n', '\n+')) + + # Now the warnings lines + expect = [expect_str[0]] + expect_str[10:12] + [expect_str[9]] + self.assertEqual(lines[28].text, 'w+%s' % + '\n'.join(expect).replace('\n', '\nw+')) + + self.assertEqual(len(lines), 29) + shutil.rmtree(base_dir) def _testGit(self): """Test basic builder operation by building a branch""" @@ -255,6 +292,7 @@ class TestBuild(unittest.TestCase): options.keep_outputs = False args = ['tegra20'] control.DoBuildman(options, args) + shutil.rmtree(base_dir) def testBoardSingle(self): """Test single board selection""" From f7582ce8496f19edf845d1f62c4b7f385e4cd864 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:22 -0600 Subject: [PATCH 17/21] buildman: Permit branch names with an embedded '/' At present buildman naively uses the branch name as part of its directory path, which causes problems if the name has an embedded '/'. Replace these with '_' to fix the problem. Reported-by: Steve Rae Signed-off-by: Simon Glass --- tools/buildman/control.py | 2 +- tools/buildman/func_test.py | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index ee9637591c..8146e1caf8 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -209,7 +209,7 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, # Create a new builder with the selected options if options.branch: - dirname = options.branch + dirname = options.branch.replace('/', '_') else: dirname = 'current' output_dir = os.path.join(options.output_dir, dirname) diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index 2cb5cf05fc..c37f1b6208 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -199,6 +199,8 @@ class TestFunctional(unittest.TestCase): # Map of [board, commit] to error messages self._error = {} + self._test_branch = TEST_BRANCH + # Avoid sending any output and clear all terminal output terminal.SetPrintTestMode() terminal.GetPrintTestLines() @@ -252,10 +254,10 @@ class TestFunctional(unittest.TestCase): def _HandleCommandGitLog(self, args): if '-n0' in args: return command.CommandResult(return_code=0) - elif args[-1] == 'upstream/master..%s' % TEST_BRANCH: + elif args[-1] == 'upstream/master..%s' % self._test_branch: return command.CommandResult(return_code=0, stdout=commit_shortlog) elif args[:3] == ['--no-color', '--no-decorate', '--reverse']: - if args[-1] == TEST_BRANCH: + if args[-1] == self._test_branch: count = int(args[3][2:]) return command.CommandResult(return_code=0, stdout=''.join(commit_log[:count])) @@ -270,9 +272,9 @@ class TestFunctional(unittest.TestCase): return command.CommandResult(return_code=0) elif config.startswith('branch.badbranch'): return command.CommandResult(return_code=1) - elif config == 'branch.%s.remote' % TEST_BRANCH: + elif config == 'branch.%s.remote' % self._test_branch: return command.CommandResult(return_code=0, stdout='upstream\n') - elif config == 'branch.%s.merge' % TEST_BRANCH: + elif config == 'branch.%s.merge' % self._test_branch: return command.CommandResult(return_code=0, stdout='refs/heads/master\n') @@ -505,3 +507,10 @@ class TestFunctional(unittest.TestCase): self.assertEqual(self._builder.count, self._total_builds) self.assertEqual(self._builder.fail, 0) self.assertEqual(self._make_calls, 3) + + def testBranchWithSlash(self): + """Test building a branch with a '/' in the name""" + self._test_branch = '/__dev/__testbranch' + self._RunControl('-b', self._test_branch, clean_dir=False) + self.assertEqual(self._builder.count, self._total_builds) + self.assertEqual(self._builder.fail, 0) From 950a23133d8235778ea29f5d9587edec7d9bbc0a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Fri, 5 Sep 2014 19:00:23 -0600 Subject: [PATCH 18/21] buildman: Ignore conflicting tags Tags like Series-version are normally expected to appear once, and with a unique value. But buildman doesn't actually look at these tags. So ignore conflicts. This allows bulidman to build a branch containing multiple patman series. Reported-by: Steve Rae Signed-off-by: Simon Glass --- tools/buildman/control.py | 15 +++++++-------- tools/buildman/func_test.py | 3 +++ tools/patman/patchstream.py | 4 +++- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 8146e1caf8..e97350f9a0 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -166,6 +166,10 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, # upstream/master~..branch but that isn't possible if upstream/master is # a merge commit (it will list all the commits that form part of the # merge) + # Conflicting tags are not a problem for buildman, since it does not use + # them. For example, Series-version is not useful for buildman. On the + # other hand conflicting tags will cause an error. So allow later tags + # to overwrite earlier ones by setting allow_overwrite=True if options.branch: if count == -1: range_expr = gitutil.GetRangeInBranch(options.git_dir, @@ -173,19 +177,14 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, upstream_commit = gitutil.GetUpstream(options.git_dir, options.branch) series = patchstream.GetMetaDataForList(upstream_commit, - options.git_dir, 1) + options.git_dir, 1, series=None, allow_overwrite=True) - # Conflicting tags are not a problem for buildman, since it does - # not use them. For example, Series-version is not useful for - # buildman. On the other hand conflicting tags will cause an - # error. So allow later tags to overwrite earlier ones. - series.allow_overwrite = True series = patchstream.GetMetaDataForList(range_expr, - options.git_dir, None, series) + options.git_dir, None, series, allow_overwrite=True) else: # Honour the count series = patchstream.GetMetaDataForList(options.branch, - options.git_dir, count) + options.git_dir, count, series=None, allow_overwrite=True) else: series = None options.verbose = True diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index c37f1b6208..75eb3a97bb 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -79,6 +79,7 @@ Date: Thu Aug 14 16:48:25 2014 -0600 Series-changes: 7 - Add new patch to fix the 'reverse' bug + Series-version: 8 Change-Id: I79078f792e8b390b8a1272a8023537821d45feda Reported-by: York Sun @@ -156,6 +157,8 @@ Date: Fri Aug 22 15:57:39 2014 -0600 Series-changes: 9 - Add new patch to avoid changing the order of tags + Series-version: 9 + Suggested-by: Masahiro Yamada Change-Id: Ib1518588c1a189ad5c3198aae76f8654aed8d0db """] diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index b3e66c32a9..d630157f8f 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -355,7 +355,7 @@ class PatchStream: def GetMetaDataForList(commit_range, git_dir=None, count=None, - series = None): + series = None, allow_overwrite=False): """Reads out patch series metadata from the commits This does a 'git log' on the relevant commits and pulls out the tags we @@ -367,11 +367,13 @@ def GetMetaDataForList(commit_range, git_dir=None, count=None, count: Number of commits to list, or None for no limit series: Series object to add information into. By default a new series is started. + allow_overwrite: Allow tags to overwrite an existing tag Returns: A Series object containing information about the commits. """ if not series: series = Series() + series.allow_overwrite = allow_overwrite params = gitutil.LogCmd(commit_range,reverse=True, count=count, git_dir=git_dir) stdout = command.RunPipe([params], capture=True).stdout From 1f7278851ea359063dabc235ff690a5010467956 Mon Sep 17 00:00:00 2001 From: Vadim Bendebury Date: Thu, 4 Sep 2014 10:45:13 -0700 Subject: [PATCH 19/21] patman: make run results better visible For an occasional user of patman some failures are not obvious: for instance when checkpatch reports warnings, the dry run still reports that the email would be sent. If it is not dry run, the warnings are shown on the screen, but it is not clear that the email was not sent. Add some code to report failure to send email explicitly. Tested by running the script on a patch with style violations, observed error messages in the script output. Signed-off-by: Vadim Bendebury Reviewed-by: Doug Anderson Acked-by: Simon Glass --- tools/patman/patman.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/patman/patman.py b/tools/patman/patman.py index 5ab74fa251..2ab6b351d6 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -146,13 +146,18 @@ else: # Email the patches out (giving the user time to check / cancel) cmd = '' - if ok or options.ignore_errors: + its_a_go = ok or options.ignore_errors + if its_a_go: cmd = gitutil.EmailPatches(series, cover_fname, args, options.dry_run, not options.ignore_bad_tags, cc_file, in_reply_to=options.in_reply_to) + else: + print col.Color(col.RED, "Not sending emails due to errors/warnings") # For a dry run, just show our actions as a sanity check if options.dry_run: series.ShowActions(args, cmd, options.process_tags) + if not its_a_go: + print col.Color(col.RED, "Email would not be sent") os.remove(cc_file) From f3d015cb4a84e4a7bb37e9963e4e8e97b48b7d68 Mon Sep 17 00:00:00 2001 From: Thierry Reding Date: Tue, 19 Aug 2014 10:22:39 +0200 Subject: [PATCH 20/21] buildman: Create parent directories as necessary When creating build directories also create parents as necessary. This fixes a failure when building a hierarchical branch (i.e. foo/bar). Signed-off-by: Thierry Reding Acked-by: Simon Glass Tested-by: Tom Rini --- tools/buildman/builder.py | 2 +- tools/buildman/builderthread.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 1b6517b488..8155c1681e 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -1141,7 +1141,7 @@ class Builder: self._verbose = verbose self.ResetResultSummary(board_selected) - builderthread.Mkdir(self.base_dir) + builderthread.Mkdir(self.base_dir, parents = True) self._PrepareWorkingSpace(min(self.num_threads, len(board_selected)), commits is not None) self._PrepareOutputSpace() diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 261919f127..a9cf68a801 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -12,14 +12,17 @@ import threading import command import gitutil -def Mkdir(dirname): +def Mkdir(dirname, parents = False): """Make a directory if it doesn't already exist. Args: dirname: Directory to create """ try: - os.mkdir(dirname) + if parents: + os.makedirs(dirname) + else: + os.mkdir(dirname) except OSError as err: if err.errno == errno.EEXIST: pass From d0ea61d9caf85e4285d5c2da508db9fac70e4aba Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Fri, 5 Sep 2014 02:23:27 +0900 Subject: [PATCH 21/21] buildman: fix typos of --dry-run help message try run => dry run no nothing => do nothing "..." => '...' The last one is for consistency with the other option helps. Change-Id: I1d69047d1fae6ef095a18f69f44ee13c448db9b7 Signed-off-by: Masahiro Yamada Acked-by: Simon Glass --- tools/buildman/cmdline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index fad9a1c3f3..27d3c708e6 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -54,7 +54,7 @@ def ParseArgs(): parser.add_option('--list-tool-chains', action='store_true', default=False, help='List available tool chains') parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', - default=False, help="Do a try run (describe actions, but no nothing)") + default=False, help="Do a dry run (describe actions, but do nothing)") parser.add_option('-o', '--output-dir', type='string', dest='output_dir', default='..', help='Directory where all builds happen and buildman has its workspace (default is ../)')