[PATCH 00/11] buildman: Refactor control and builderthread
From: Simon Glass <simon.glass@canonical.com> This series refactors builderthread.py to improve code organisation and reduce complexity. The main changes are: - Fix pylint warnings in control.py and builderthread.py - Document the toolchain member in BuilderThread - Introduce RunRequest named tuple to group parameters that don't change during a job, reducing run_commit() from 11 to 7 parameters - Split _config_and_build() into three methods: _setup_build(), _reconfig_if_needed(), and _build_and_get_result() - Add BuildSetup named tuple for _setup_build() return value - Extract _do_build() from run_commit() - Extract _write_toolchain_result() and _process_elf_file() from _write_result() These changes make the code easier to understand and maintain without changing functionality. Simon Glass (11): buildman: Fix pylint warnings in control.py buildman: Fix some pylint warnings in builderthread.py buildman: Document toolchain member in BuilderThread buildman: Add RunRequest named tuple for run_commit() parameters buildman: Split _config_and_build() into three methods buildman: Add BuildSetup named tuple for _setup_build() return value buildman: Extract _do_build() from run_commit() buildman: Extract _write_toolchain_result() from _write_result() buildman: Extract _process_elf_file() from _write_toolchain_result() buildman: Silence too-many-arguments warnings buildman: Fix return type documentation in builderthread.py tools/buildman/builderthread.py | 548 ++++++++++++++++++++------------ tools/buildman/control.py | 94 +++--- 2 files changed, 391 insertions(+), 251 deletions(-) -- 2.43.0 base-commit: 7c0f9b0a46f31e5f5c9793e4206cc735968b1839 branch: bmm
From: Simon Glass <simon.glass@canonical.com> Fix various pylint warnings: - Fix line too long in show_actions() - Fix docstring parameter mismatches (wrong names, missing types) - Add pylint disable comments for functions with many positional args - Remove unnecessary else after return in check_pid() - Remove unused variable running_fname in wait_for_process_limit() - Add pylint disable for import-outside-toplevel (filelock) - Shorten URL in check_pid() docstring This brings control.py to pylint 10.00/10 Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/control.py | 94 ++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 46 deletions(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index 88e8338c599..052c9fa4343 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -68,14 +68,13 @@ def get_action_summary(is_summary, commit_count, selected, threads, jobs): Args: is_summary (bool): True if this is a summary (otherwise it is building) - commits (list): List of commits being built + commit_count (int): Number of commits being built selected (list of Board): List of Board objects that are marked - step (int): Step increment through commits threads (int): Number of processor threads being used jobs (int): Number of jobs to build at once Returns: - Summary string. + str: Summary string """ if commit_count: commit_str = f'{commit_count} commit{get_plural(commit_count)}' @@ -87,22 +86,23 @@ def get_action_summary(is_summary, commit_count, selected, threads, jobs): f'{jobs} job{get_plural(jobs)} per thread)') return msg -# pylint: disable=R0913 +# pylint: disable=R0913,R0917 def show_actions(series, why_selected, boards_selected, output_dir, board_warnings, step, threads, jobs, verbose): """Display a list of actions that we would take, if not a dry run. Args: - series: Series object - why_selected: Dictionary where each key is a buildman argument - provided by the user, and the value is the list of boards - brought in by that argument. For example, 'arm' might bring - in 400 boards, so in this case the key would be 'arm' and - the value would be a list of board names. - boards_selected: Dict of selected boards, key is target name, - value is Board object + series (Series): Series object + why_selected (dict): Dictionary where each key is a buildman argument + provided by the user, and the value is the list of boards + brought in by that argument. For example, 'arm' might bring + in 400 boards, so in this case the key would be 'arm' and + the value would be a list of board names. + boards_selected (dict): Dict of selected boards, key is target name, + value is Board object output_dir (str): Output directory for builder - board_warnings: List of warnings obtained from board selected + board_warnings (list of str): List of warnings obtained from board + selected step (int): Step increment through commits threads (int): Number of processor threads being used jobs (int): Number of jobs to build at once @@ -121,7 +121,8 @@ def show_actions(series, why_selected, boards_selected, output_dir, if commits: for upto in range(0, len(series.commits), step): commit = series.commits[upto] - print(' ', col.build(col.YELLOW, commit.hash[:8], bright=False), end=' ') + print(' ', col.build(col.YELLOW, commit.hash[:8], bright=False), + end=' ') print(commit.subject) print() for arg in why_selected: @@ -143,11 +144,9 @@ def show_toolchain_prefix(brds, toolchains): the correct value for CROSS_COMPILE. Args: - boards: Boards object containing selected boards - toolchains: Toolchains object containing available toolchains - - Return: - None on success, string error message otherwise + brds (Boards): Boards object containing selected boards + toolchains (Toolchains): Toolchains object containing available + toolchains """ board_selected = brds.get_selected_dict() tc_set = set() @@ -165,10 +164,7 @@ def show_arch(brds): the correct value for ARCH. Args: - boards: Boards object containing selected boards - - Return: - None on success, string error message otherwise + brds (Boards): Boards object containing selected boards """ board_selected = brds.get_selected_dict() arch_set = set() @@ -251,6 +247,7 @@ def count_commits(branch, count, col, git_dir): return count, has_range +# pylint: disable=R0917 def determine_series(selected, col, git_dir, count, branch, work_in_output): """Determine the series which is to be built, if any @@ -348,6 +345,7 @@ def do_fetch_arch(toolchains, col, fetch_arch): return 0 +# pylint: disable=R0917 def get_toolchains(toolchains, col, override_toolchain, fetch_arch, list_tool_chains, verbose): """Get toolchains object to use @@ -383,6 +381,7 @@ def get_toolchains(toolchains, col, override_toolchain, fetch_arch, return toolchains +# pylint: disable=R0917 def get_boards_obj(output_dir, regen_board_list, maintainer_check, full_check, threads, verbose): """Object the Boards object to use @@ -501,6 +500,7 @@ def adjust_args(args, series, selected): args.show_detail = True +# pylint: disable=R0917 def setup_output_dir(output_dir, work_in_output, branch, no_subdirs, col, in_tree, clean_dir): """Set up the output directory @@ -510,9 +510,10 @@ def setup_output_dir(output_dir, work_in_output, branch, no_subdirs, col, work_in_output (bool): True to work in the output directory branch (str): Name of branch to build, or None if none no_subdirs (bool): True to put the output in the top-level output dir + col (Terminal.Color): Color object to use in_tree (bool): True if doing an in-tree build - clean_dir: Used for tests only, indicates that the existing output_dir - should be removed before starting the build + clean_dir (bool): Used for tests only, indicates that the existing + output_dir should be removed before starting the build Returns: str: Updated output directory pathname @@ -537,8 +538,9 @@ def run_builder(builder, commits, board_selected, args): """Run the builder or show the summary Args: + builder (Builder): Builder to use commits (list of Commit): List of commits being built, None if no branch - boards_selected (dict): Dict of selected boards: + board_selected (dict): Dict of selected boards: key: target name value: Board object args (Namespace): Namespace to use @@ -622,26 +624,26 @@ def read_procs(tmpdir=tempfile.gettempdir()): def check_pid(pid): """Check for existence of a unix PID - https://stackoverflow.com/questions/568271/how-to-check-if-there-exists-a-pr... + See: https://stackoverflow.com/questions/568271 Args: pid (int): PID to check Returns: - True if it exists, else False + bool: True if it exists, else False """ try: os.kill(pid, 0) except OSError: return False - else: - return True + return True def write_procs(procs, tmpdir=tempfile.gettempdir()): """Write the list of running buildman processes Args: + procs (list of int): List of process IDs to write tmpdir (str): Temporary directory to use (for testing only) """ running_fname = os.path.join(tmpdir, RUNNING_FNAME) @@ -678,9 +680,9 @@ def wait_for_process_limit(limit, tmpdir=tempfile.gettempdir(), tmpdir (str): Temporary directory to use (for testing only) pid (int): Current process ID (for testing only) """ + # pylint: disable=C0415 from filelock import Timeout, FileLock - running_fname = os.path.join(tmpdir, RUNNING_FNAME) lock_fname = os.path.join(tmpdir, LOCK_FNAME) lock = FileLock(lock_fname) @@ -719,26 +721,26 @@ def wait_for_process_limit(limit, tmpdir=tempfile.gettempdir(), tprint('starting build', newline=False) print_clear() +# pylint: disable=R0917 def do_buildman(args, toolchains=None, make_func=None, brds=None, clean_dir=False, test_thread_exceptions=False): """The main control code for buildman Args: - args: ArgumentParser 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. - brds: Boards() object to use, containing a list of available - boards. If this is None it will be created and scanned. - clean_dir: Used for tests only, indicates that the existing output_dir - should be removed before starting the build - test_thread_exceptions: Uses for tests only, True to make the threads - raise an exception instead of reporting their result. This simulates - a failure in the code somewhere + args (Namespace): ArgumentParser object + toolchains (Toolchains): Toolchains to use - this should be a + Toolchains() object. If None, then it will be created and scanned + make_func (function): 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. + brds (Boards): Boards() object to use, containing a list of available + boards. If this is None it will be created and scanned. + clean_dir (bool): Used for tests only, indicates that the existing + output_dir should be removed before starting the build + test_thread_exceptions (bool): Uses for tests only, True to make the + threads raise an exception instead of reporting their result. This + simulates a failure in the code somewhere """ # Used so testing can obtain the builder: pylint: disable=W0603 global TEST_BUILDER -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Fix various pylint warnings: - Remove unused import sys - Add ValueError to raises documentation in mkdir() - Use "from err" when re-raising ValueError - Change "!= None" to "is not None" (singleton comparison) - Fix long line in run_commit() - Add pylint disable for broad-exception-caught in run() This brings builderthread.py to pylint 9.97/10 Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 0fd589934fe..1102f61ef45 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -13,7 +13,6 @@ import glob import io import os import shutil -import sys import threading from buildman import cfgutil @@ -71,6 +70,7 @@ def mkdir(dirname, parents=False): Raises: OSError: File already exists + ValueError: Trying to create the current working directory """ if not dirname or os.path.exists(dirname): return @@ -83,7 +83,8 @@ def mkdir(dirname, parents=False): if err.errno == errno.EEXIST: if os.path.realpath('.') == os.path.realpath(dirname): raise ValueError( - f"Cannot create the current working directory '{dirname}'!") + f"Cannot create the current working directory " + f"'{dirname}'!") from err else: raise @@ -464,7 +465,7 @@ class BuilderThread(threading.Thread): config_args.append(fname) else: config_args = [f'{brd.target}_defconfig'] - if fragments != None: + if fragments is not None: config_args.extend(fragments.split(',')) config_out = io.StringIO() @@ -553,7 +554,8 @@ class BuilderThread(threading.Thread): except ValueError as err: result.return_code = 10 result.stdout = '' - result.stderr = f'Tool chain error for {brd.arch}: {str(err)}' + result.stderr = (f'Tool chain error for {brd.arch}: ' + f'{str(err)}') if self.toolchain: commit = self._checkout(commit_upto, work_dir) @@ -848,7 +850,7 @@ class BuilderThread(threading.Thread): job = self.builder.queue.get() try: self.run_job(job) - except Exception as exc: + except Exception as exc: # pylint: disable=W0718 print('Thread exception (use -T0 to run without threads):', exc) self.builder.thread_exceptions.append(exc) -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Add missing documentation for the toolchain member in the BuilderThread class docstring. This member holds the selected toolchain for building and is used throughout the class. Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 1102f61ef45..c480f4888a1 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -189,6 +189,8 @@ class BuilderThread(threading.Thread): board rather than a thread-specific directory test_exception: Used for testing; True to raise an exception instead of reporting the build result + toolchain: Toolchain object to use for building, or None if not yet + selected """ def __init__(self, builder, thread_num, mrproper, per_board_out_dir, test_exception=False): -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Introduce a RunRequest named tuple to group the parameters that don't change during a job's for loop in run_job() This includes: - brd: Board to build - work_dir: Directory for source checkout - work_in_output: Whether to use output directory as work directory - adjust_cfg: Config adjustments - fragments: Config fragments This reduces the run_commit() signature from 11 parameters to 7, and simplifies passing these values through to _decide_dirs() and _config_and_build() Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 158 ++++++++++++++++---------------- 1 file changed, 79 insertions(+), 79 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index c480f4888a1..eda2b0083d4 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -8,6 +8,7 @@ This module provides the BuilderThread class, which handles calling the builder based on the jobs provided. """ +from collections import namedtuple import errno import glob import io @@ -20,6 +21,23 @@ from u_boot_pylib import command from u_boot_pylib import gitutil from u_boot_pylib import tools +# Named tuple for run_commit() options that don't change during a job +# +# Members: +# brd (Board): Board to build +# work_dir (str): Directory to which the source will be checked out +# work_in_output (bool): Use the output directory as the work directory and +# don't write to a separate output directory +# adjust_cfg (list of str): List of changes to make to .config file before +# building. Each is one of (where C is either CONFIG_xxx or just xxx): +# C to enable C +# ~C to disable C +# C=val to set the value of C (val must have quotes if C is a +# string Kconfig +# fragments (str): Config fragments added to defconfig +RunRequest = namedtuple('RunRequest', ['brd', 'work_dir', 'work_in_output', + 'adjust_cfg', 'fragments']) + RETURN_CODE_RETRY = -1 BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl'] @@ -384,28 +402,26 @@ class BuilderThread(threading.Thread): return will_build, result - def _decide_dirs(self, brd, work_dir, work_in_output): + def _decide_dirs(self, req): """Decide the output directory to use Args: - work_dir (str): Directory to which the source will be checked out - work_in_output (bool): Use the output directory as the work - directory and don't write to a separate output directory. + req (RunRequest): Run request (see RunRequest for details) Returns: tuple: out_dir (str): Output directory for the build - out_rel_dir (str): Output directory relatie to the current dir + out_rel_dir (str): Output directory relative to the current dir """ - if work_in_output or self.builder.in_tree: + if req.work_in_output or self.builder.in_tree: out_rel_dir = None - out_dir = work_dir + out_dir = req.work_dir else: if self.per_board_out_dir: - out_rel_dir = os.path.join('..', brd.target) + out_rel_dir = os.path.join('..', req.brd.target) else: out_rel_dir = 'build' - out_dir = os.path.join(work_dir, out_rel_dir) + out_dir = os.path.join(req.work_dir, out_rel_dir) return out_dir, out_rel_dir def _checkout(self, commit_upto, work_dir): @@ -427,24 +443,20 @@ class BuilderThread(threading.Thread): commit = 'current' return commit - def _config_and_build(self, commit_upto, brd, work_dir, do_config, mrproper, - config_only, adjust_cfg, commit, out_dir, out_rel_dir, - fragments, result): + def _config_and_build(self, req, commit_upto, do_config, mrproper, + config_only, commit, out_dir, out_rel_dir, result): """Do the build, configuring first if necessary Args: + req (RunRequest): Run request (see RunRequest for details) commit_upto (int): Commit number to build (0...n-1) - brd (Board): Board to create arguments for - work_dir (str): Directory to which the source will be checked out do_config (bool): True to run a make <board>_defconfig on the source mrproper (bool): True to run mrproper first config_only (bool): Only configure the source, do not build it - adjust_cfg (list of str): See the cfgutil module and run_commit() - commit (Commit): Commit only being built + commit (Commit): Commit being built out_dir (str): Output directory for the build, or None to use current - out_rel_dir (str): Output directory relatie to the current dir - fragments (str): config fragments added to defconfig + out_rel_dir (str): Output directory relative to the current dir result (CommandResult): Previous result Returns: @@ -458,17 +470,17 @@ class BuilderThread(threading.Thread): if out_dir and not os.path.exists(out_dir): mkdir(out_dir) - args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir, - work_dir, commit_upto) - if brd.extended: - config_args = [f'{brd.orig_target}_defconfig'] - for frag in brd.extended.fragments: + args, cwd, src_dir = self._build_args(req.brd, out_dir, out_rel_dir, + req.work_dir, commit_upto) + if req.brd.extended: + config_args = [f'{req.brd.orig_target}_defconfig'] + for frag in req.brd.extended.fragments: fname = os.path.join(f'{frag}.config') config_args.append(fname) else: - config_args = [f'{brd.target}_defconfig'] - if fragments is not None: - config_args.extend(fragments.split(',')) + config_args = [f'{req.brd.target}_defconfig'] + if req.fragments is not None: + config_args.extend(req.fragments.split(',')) config_out = io.StringIO() _remove_old_outputs(out_dir) @@ -476,26 +488,26 @@ class BuilderThread(threading.Thread): # If we need to reconfigure, do that now cfg_file = os.path.join(out_dir or '', '.config') cmd_list = [] - if do_config or adjust_cfg: + if do_config or req.adjust_cfg: result = self._reconfigure( - commit, brd, cwd, args, env, config_args, config_out, cmd_list, - mrproper) + commit, req.brd, cwd, args, env, config_args, config_out, + cmd_list, mrproper) do_config = False # No need to configure next time - if adjust_cfg: - cfgutil.adjust_cfg_file(cfg_file, adjust_cfg) + if req.adjust_cfg: + cfgutil.adjust_cfg_file(cfg_file, req.adjust_cfg) # Now do the build, if everything looks OK if result.return_code == 0: - if adjust_cfg: + if req.adjust_cfg: oldc_args = list(args) + ['oldconfig'] - oldc_result = self.make(commit, brd, 'oldconfig', cwd, + oldc_result = self.make(commit, req.brd, 'oldconfig', cwd, *oldc_args, env=env) if oldc_result.return_code: return oldc_result - result = self._build(commit, brd, cwd, args, env, cmd_list, + result = self._build(commit, req.brd, cwd, args, env, cmd_list, config_only) - if adjust_cfg: - errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg) + if req.adjust_cfg: + errs = cfgutil.check_cfg_file(cfg_file, req.adjust_cfg) if errs: result.stderr += errs result.return_code = 1 @@ -505,34 +517,22 @@ class BuilderThread(threading.Thread): result.cmd_list = cmd_list return result, do_config - def run_commit(self, commit_upto, brd, work_dir, do_config, mrproper, - config_only, force_build, force_build_failures, - work_in_output, adjust_cfg, fragments): + def run_commit(self, req, commit_upto, do_config, mrproper, config_only, + force_build, force_build_failures): """Build a particular commit. If the build is already done, and we are not forcing a build, we skip the build and just return the previously-saved results. Args: + req (RunRequest): Run request (see RunRequest for details) commit_upto (int): Commit number to build (0...n-1) - brd (Board): Board to build - work_dir (str): Directory to which the source will be checked out do_config (bool): True to run a make <board>_defconfig on the source mrproper (bool): True to run mrproper first config_only (bool): Only configure the source, do not build it force_build (bool): Force a build even if one was previously done - force_build_failures (bool): Force a bulid if the previous result + force_build_failures (bool): Force a build if the previous result showed failure - work_in_output (bool) : Use the output directory as the work - directory and don't write to a separate output directory. - adjust_cfg (list of str): List of changes to make to .config file - before building. Each is one of (where C is either CONFIG_xxx - or just xxx): - C to enable C - ~C to disable C - C=val to set the value of C (val must have quotes if C is - a string Kconfig - fragments (str): config fragments added to defconfig Returns: tuple containing: @@ -541,10 +541,11 @@ class BuilderThread(threading.Thread): """ # Create a default result - it will be overwritte by the call to # self.make() below, in the event that we do a build. - out_dir, out_rel_dir = self._decide_dirs(brd, work_dir, work_in_output) + out_dir, out_rel_dir = self._decide_dirs(req) # Check if the job was already completed last time - will_build, result = self._read_done_file(commit_upto, brd, force_build, + will_build, result = self._read_done_file(commit_upto, req.brd, + force_build, force_build_failures) kconfig_reconfig = False @@ -552,33 +553,33 @@ class BuilderThread(threading.Thread): # We are going to have to build it. First, get a toolchain if not self.toolchain: try: - self.toolchain = self.builder.toolchains.select(brd.arch) + self.toolchain = self.builder.toolchains.select( + req.brd.arch) except ValueError as err: result.return_code = 10 result.stdout = '' - result.stderr = (f'Tool chain error for {brd.arch}: ' + result.stderr = (f'Tool chain error for {req.brd.arch}: ' f'{str(err)}') if self.toolchain: - commit = self._checkout(commit_upto, work_dir) + commit = self._checkout(commit_upto, req.work_dir) # Check if Kconfig files have changed since last config if self.builder.kconfig_check: config_file = os.path.join(out_dir, '.config') - if kconfig_changed_since(config_file, work_dir, - brd.target): + if kconfig_changed_since(config_file, req.work_dir, + req.brd.target): kconfig_reconfig = True do_config = True result, do_config = self._config_and_build( - commit_upto, brd, work_dir, do_config, mrproper, - config_only, adjust_cfg, commit, out_dir, out_rel_dir, - fragments, result) + req, commit_upto, do_config, mrproper, config_only, + commit, out_dir, out_rel_dir, result) result.already_done = False result.kconfig_reconfig = kconfig_reconfig result.toolchain = self.toolchain - result.brd = brd + result.brd = req.brd result.commit_upto = commit_upto result.out_dir = out_dir return result, do_config @@ -761,29 +762,29 @@ class BuilderThread(threading.Thread): brd = job.brd work_dir = self.builder.get_thread_dir(self.thread_num) self.toolchain = None + req = RunRequest(brd, work_dir, job.work_in_output, job.adjust_cfg, + job.fragments) if job.commits: # Run 'make board_defconfig' on the first commit do_config = True commit_upto = 0 force_build = False for commit_upto in range(0, len(job.commits), job.step): - result, request_config = self.run_commit(commit_upto, brd, - work_dir, do_config, self.mrproper, + result, request_config = self.run_commit( + req, commit_upto, do_config, self.mrproper, self.builder.config_only, force_build or self.builder.force_build, - self.builder.force_build_failures, - job.work_in_output, job.adjust_cfg, job.fragments) + self.builder.force_build_failures) failed = result.return_code did_config = do_config if failed and not do_config and not self.mrproper: # If our incremental build failed, try building again # with a reconfig. if self.builder.force_config_on_failure: - result, request_config = self.run_commit(commit_upto, - brd, work_dir, True, + result, request_config = self.run_commit( + req, commit_upto, True, self.mrproper or self.builder.fallback_mrproper, - False, True, False, job.work_in_output, - job.adjust_cfg, job.fragments) + False, True, False) did_config = True if not self.builder.force_reconfig: do_config = request_config @@ -826,17 +827,16 @@ class BuilderThread(threading.Thread): self._send_result(result) else: # Just build the currently checked-out build - result, request_config = self.run_commit(None, brd, work_dir, True, - self.mrproper, self.builder.config_only, True, - self.builder.force_build_failures, job.work_in_output, - job.adjust_cfg, job.fragments) + result, request_config = self.run_commit( + req, None, True, self.mrproper, + self.builder.config_only, True, + self.builder.force_build_failures) failed = result.return_code if failed and not self.mrproper: - result, request_config = self.run_commit(None, brd, work_dir, - True, self.builder.fallback_mrproper, + result, request_config = self.run_commit( + req, None, True, self.builder.fallback_mrproper, self.builder.config_only, True, - self.builder.force_build_failures, - job.work_in_output, job.adjust_cfg, job.fragments) + self.builder.force_build_failures) result.commit_upto = 0 self._write_result(result, job.keep_outputs, job.work_in_output) -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Refactor the _config_and_build() method to improve readability by splitting it into three separate methods: - _setup_build(): Set up the build environment and arguments - _reconfig_if_needed(): Handle reconfiguration if needed - _build_and_get_result(): Perform the build and finalise the result Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 110 +++++++++++++++++++++++++++----- 1 file changed, 94 insertions(+), 16 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index eda2b0083d4..67a0b486b47 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -443,29 +443,24 @@ class BuilderThread(threading.Thread): commit = 'current' return commit - def _config_and_build(self, req, commit_upto, do_config, mrproper, - config_only, commit, out_dir, out_rel_dir, result): - """Do the build, configuring first if necessary + def _setup_build(self, req, commit_upto, out_dir, out_rel_dir): + """Set up the build environment and arguments Args: req (RunRequest): Run request (see RunRequest for details) commit_upto (int): Commit number to build (0...n-1) - do_config (bool): True to run a make <board>_defconfig on the source - mrproper (bool): True to run mrproper first - config_only (bool): Only configure the source, do not build it - commit (Commit): Commit being built out_dir (str): Output directory for the build, or None to use current out_rel_dir (str): Output directory relative to the current dir - result (CommandResult): Previous result Returns: tuple: - result (CommandResult): Result of the build - do_config (bool): indicates whether 'make config' is needed on - the next incremental build + env (dict): Environment variables for the build + args (list of str): Arguments to pass to make + config_args (list of str): Arguments for configuration + cwd (str): Current working directory for the build + src_dir (str): Source directory path """ - # Set up the environment and command line env = self.builder.make_environment(self.toolchain) if out_dir and not os.path.exists(out_dir): mkdir(out_dir) @@ -481,13 +476,37 @@ class BuilderThread(threading.Thread): config_args = [f'{req.brd.target}_defconfig'] if req.fragments is not None: config_args.extend(req.fragments.split(',')) - config_out = io.StringIO() _remove_old_outputs(out_dir) - # If we need to reconfigure, do that now + return env, args, config_args, cwd, src_dir + + def _reconfig_if_needed(self, req, commit, cwd, args, env, config_args, + config_out, cmd_list, out_dir, do_config, mrproper, + result): + """Reconfigure the build if needed + + Args: + req (RunRequest): Run request (see RunRequest for details) + commit (Commit): Commit being built + cwd (str): Current working directory + args (list of str): Arguments to pass to make + env (dict): Environment strings + config_args (list of str): Arguments for configuration + config_out (StringIO): Buffer for configuration output + cmd_list (list of str): List to add the commands to, for logging + out_dir (str): Output directory for the build + do_config (bool): True to run a make <board>_defconfig on the source + mrproper (bool): True to run mrproper first + result (CommandResult): Previous result + + Returns: + tuple: + result (CommandResult): Result of the reconfiguration + do_config (bool): Whether config is still needed + cfg_file (str): Path to the .config file + """ cfg_file = os.path.join(out_dir or '', '.config') - cmd_list = [] if do_config or req.adjust_cfg: result = self._reconfigure( commit, req.brd, cwd, args, env, config_args, config_out, @@ -495,8 +514,29 @@ class BuilderThread(threading.Thread): do_config = False # No need to configure next time if req.adjust_cfg: cfgutil.adjust_cfg_file(cfg_file, req.adjust_cfg) + return result, do_config, cfg_file + + def _build_and_get_result(self, req, commit, cwd, args, env, cmd_list, + config_out, cfg_file, src_dir, config_only, + result): + """Perform the build and finalise the result + + Args: + req (RunRequest): Run request (see RunRequest for details) + commit (Commit): Commit being built + cwd (str): Current working directory + args (list of str): Arguments to pass to make + env (dict): Environment strings + cmd_list (list of str): List to add the commands to, for logging + config_out (StringIO): Buffer for configuration output + cfg_file (str): Path to the .config file + src_dir (str): Source directory path + config_only (bool): Only configure the source, do not build it + result (CommandResult): Previous result - # Now do the build, if everything looks OK + Returns: + CommandResult: Result of the build + """ if result.return_code == 0: if req.adjust_cfg: oldc_args = list(args) + ['oldconfig'] @@ -515,6 +555,44 @@ class BuilderThread(threading.Thread): if self.builder.verbose_build: result.stdout = config_out.getvalue() + result.stdout result.cmd_list = cmd_list + return result + + def _config_and_build(self, req, commit_upto, do_config, mrproper, + config_only, commit, out_dir, out_rel_dir, result): + """Do the build, configuring first if necessary + + Args: + req (RunRequest): Run request (see RunRequest for details) + commit_upto (int): Commit number to build (0...n-1) + do_config (bool): True to run a make <board>_defconfig on the source + mrproper (bool): True to run mrproper first + config_only (bool): Only configure the source, do not build it + commit (Commit): Commit being built + out_dir (str): Output directory for the build, or None to use + current + out_rel_dir (str): Output directory relative to the current dir + result (CommandResult): Previous result + + Returns: + tuple: + result (CommandResult): Result of the build + do_config (bool): indicates whether 'make config' is needed on + the next incremental build + """ + env, args, config_args, cwd, src_dir = self._setup_build( + req, commit_upto, out_dir, out_rel_dir) + + config_out = io.StringIO() + cmd_list = [] + + result, do_config, cfg_file = self._reconfig_if_needed( + req, commit, cwd, args, env, config_args, config_out, cmd_list, + out_dir, do_config, mrproper, result) + + result = self._build_and_get_result( + req, commit, cwd, args, env, cmd_list, config_out, cfg_file, + src_dir, config_only, result) + return result, do_config def run_commit(self, req, commit_upto, do_config, mrproper, config_only, -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Introduce a BuildSetup named tuple to hold the return value from _setup_build(). This groups the build environment setup values: - env: Environment variables for the build - args: Arguments to pass to make - config_args: Arguments for configuration - cwd: Current working directory for the build - src_dir: Source directory path Update _reconfig_if_needed() and _build_and_get_result() to accept the setup tuple instead of individual parameters, simplifying their signatures and the calls from _config_and_build() Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 67 ++++++++++++++++----------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 67a0b486b47..e0164b30a4e 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -38,6 +38,17 @@ from u_boot_pylib import tools RunRequest = namedtuple('RunRequest', ['brd', 'work_dir', 'work_in_output', 'adjust_cfg', 'fragments']) +# Named tuple for _setup_build() return value +# +# Members: +# env (dict): Environment variables for the build +# args (list of str): Arguments to pass to make +# config_args (list of str): Arguments for configuration +# cwd (str): Current working directory for the build +# src_dir (str): Source directory path +BuildSetup = namedtuple('BuildSetup', ['env', 'args', 'config_args', 'cwd', + 'src_dir']) + RETURN_CODE_RETRY = -1 BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl'] @@ -454,12 +465,7 @@ class BuilderThread(threading.Thread): out_rel_dir (str): Output directory relative to the current dir Returns: - tuple: - env (dict): Environment variables for the build - args (list of str): Arguments to pass to make - config_args (list of str): Arguments for configuration - cwd (str): Current working directory for the build - src_dir (str): Source directory path + BuildSetup: Build setup (see BuildSetup for details) """ env = self.builder.make_environment(self.toolchain) if out_dir and not os.path.exists(out_dir): @@ -479,20 +485,16 @@ class BuilderThread(threading.Thread): _remove_old_outputs(out_dir) - return env, args, config_args, cwd, src_dir + return BuildSetup(env, args, config_args, cwd, src_dir) - def _reconfig_if_needed(self, req, commit, cwd, args, env, config_args, - config_out, cmd_list, out_dir, do_config, mrproper, - result): + def _reconfig_if_needed(self, req, setup, commit, config_out, cmd_list, + out_dir, do_config, mrproper, result): """Reconfigure the build if needed Args: req (RunRequest): Run request (see RunRequest for details) + setup (BuildSetup): Build setup (see BuildSetup for details) commit (Commit): Commit being built - cwd (str): Current working directory - args (list of str): Arguments to pass to make - env (dict): Environment strings - config_args (list of str): Arguments for configuration config_out (StringIO): Buffer for configuration output cmd_list (list of str): List to add the commands to, for logging out_dir (str): Output directory for the build @@ -509,28 +511,24 @@ class BuilderThread(threading.Thread): cfg_file = os.path.join(out_dir or '', '.config') if do_config or req.adjust_cfg: result = self._reconfigure( - commit, req.brd, cwd, args, env, config_args, config_out, - cmd_list, mrproper) + commit, req.brd, setup.cwd, setup.args, setup.env, + setup.config_args, config_out, cmd_list, mrproper) do_config = False # No need to configure next time if req.adjust_cfg: cfgutil.adjust_cfg_file(cfg_file, req.adjust_cfg) return result, do_config, cfg_file - def _build_and_get_result(self, req, commit, cwd, args, env, cmd_list, - config_out, cfg_file, src_dir, config_only, - result): + def _build_and_get_result(self, req, setup, commit, cmd_list, config_out, + cfg_file, config_only, result): """Perform the build and finalise the result Args: req (RunRequest): Run request (see RunRequest for details) + setup (BuildSetup): Build setup (see BuildSetup for details) commit (Commit): Commit being built - cwd (str): Current working directory - args (list of str): Arguments to pass to make - env (dict): Environment strings cmd_list (list of str): List to add the commands to, for logging config_out (StringIO): Buffer for configuration output cfg_file (str): Path to the .config file - src_dir (str): Source directory path config_only (bool): Only configure the source, do not build it result (CommandResult): Previous result @@ -539,19 +537,19 @@ class BuilderThread(threading.Thread): """ if result.return_code == 0: if req.adjust_cfg: - oldc_args = list(args) + ['oldconfig'] - oldc_result = self.make(commit, req.brd, 'oldconfig', cwd, - *oldc_args, env=env) + oldc_args = list(setup.args) + ['oldconfig'] + oldc_result = self.make(commit, req.brd, 'oldconfig', setup.cwd, + *oldc_args, env=setup.env) if oldc_result.return_code: return oldc_result - result = self._build(commit, req.brd, cwd, args, env, cmd_list, - config_only) + result = self._build(commit, req.brd, setup.cwd, setup.args, + setup.env, cmd_list, config_only) if req.adjust_cfg: errs = cfgutil.check_cfg_file(cfg_file, req.adjust_cfg) if errs: result.stderr += errs result.return_code = 1 - result.stderr = result.stderr.replace(src_dir + '/', '') + result.stderr = result.stderr.replace(setup.src_dir + '/', '') if self.builder.verbose_build: result.stdout = config_out.getvalue() + result.stdout result.cmd_list = cmd_list @@ -579,19 +577,18 @@ class BuilderThread(threading.Thread): do_config (bool): indicates whether 'make config' is needed on the next incremental build """ - env, args, config_args, cwd, src_dir = self._setup_build( - req, commit_upto, out_dir, out_rel_dir) + setup = self._setup_build(req, commit_upto, out_dir, out_rel_dir) config_out = io.StringIO() cmd_list = [] result, do_config, cfg_file = self._reconfig_if_needed( - req, commit, cwd, args, env, config_args, config_out, cmd_list, - out_dir, do_config, mrproper, result) + req, setup, commit, config_out, cmd_list, out_dir, do_config, + mrproper, result) result = self._build_and_get_result( - req, commit, cwd, args, env, cmd_list, config_out, cfg_file, - src_dir, config_only, result) + req, setup, commit, cmd_list, config_out, cfg_file, config_only, + result) return result, do_config -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Move the build logic from the 'if will_build:' block in run_commit() into a separate _do_build() method. This improves readability and reduces the complexity of run_commit() The new method handles: - Obtaining a toolchain if needed - Checking for Kconfig changes that require reconfiguration - Calling _config_and_build() to perform the actual build Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 81 +++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index e0164b30a4e..8420698023a 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -592,6 +592,56 @@ class BuilderThread(threading.Thread): return result, do_config + def _do_build(self, req, commit_upto, do_config, mrproper, config_only, + out_dir, out_rel_dir, result): + """Perform a build if a toolchain can be obtained + + Args: + req (RunRequest): Run request (see RunRequest for details) + commit_upto (int): Commit number to build (0...n-1) + do_config (bool): True to run a make <board>_defconfig on the source + mrproper (bool): True to run mrproper first + config_only (bool): Only configure the source, do not build it + out_dir (str): Output directory for the build + out_rel_dir (str): Output directory relative to the current dir + result (CommandResult): Previous result + + Returns: + tuple: + result (CommandResult): Result of the build + do_config (bool): Whether config is needed next time + kconfig_reconfig (bool): Whether Kconfig triggered a reconfig + """ + kconfig_reconfig = False + + # We are going to have to build it. First, get a toolchain + if not self.toolchain: + try: + self.toolchain = self.builder.toolchains.select(req.brd.arch) + except ValueError as err: + result.return_code = 10 + result.stdout = '' + result.stderr = f'Tool chain error for {req.brd.arch}: {err}' + + if self.toolchain: + commit = self._checkout(commit_upto, req.work_dir) + + # Check if Kconfig files have changed since last config + if self.builder.kconfig_check: + config_file = os.path.join(out_dir, '.config') + if kconfig_changed_since(config_file, req.work_dir, + req.brd.target): + kconfig_reconfig = True + do_config = True + + result, do_config = self._config_and_build( + req, commit_upto, do_config, mrproper, config_only, + commit, out_dir, out_rel_dir, result) + + result.already_done = False + result.kconfig_reconfig = kconfig_reconfig + return result, do_config, kconfig_reconfig + def run_commit(self, req, commit_upto, do_config, mrproper, config_only, force_build, force_build_failures): """Build a particular commit. @@ -623,35 +673,10 @@ class BuilderThread(threading.Thread): force_build, force_build_failures) - kconfig_reconfig = False if will_build: - # We are going to have to build it. First, get a toolchain - if not self.toolchain: - try: - self.toolchain = self.builder.toolchains.select( - req.brd.arch) - except ValueError as err: - result.return_code = 10 - result.stdout = '' - result.stderr = (f'Tool chain error for {req.brd.arch}: ' - f'{str(err)}') - - if self.toolchain: - commit = self._checkout(commit_upto, req.work_dir) - - # Check if Kconfig files have changed since last config - if self.builder.kconfig_check: - config_file = os.path.join(out_dir, '.config') - if kconfig_changed_since(config_file, req.work_dir, - req.brd.target): - kconfig_reconfig = True - do_config = True - - result, do_config = self._config_and_build( - req, commit_upto, do_config, mrproper, config_only, - commit, out_dir, out_rel_dir, result) - result.already_done = False - result.kconfig_reconfig = kconfig_reconfig + result, do_config, _ = self._do_build( + req, commit_upto, do_config, mrproper, config_only, + out_dir, out_rel_dir, result) result.toolchain = self.toolchain result.brd = req.brd -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Move the toolchain-result-writing logic from the 'if result.toolchain:' block in _write_result() into a separate _write_toolchain_result() method. This improves readability and reduces the complexity of _write_result() The new method handles: - Writing the done file with the return code - Writing toolchain information (gcc, path, cross, arch) - Writing environment and command list files - Running nm, objdump, and size on ELF files - Extracting and copying the U-Boot environment - Writing image sizes file Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 179 +++++++++++++++++--------------- 1 file changed, 97 insertions(+), 82 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 8420698023a..1855e2cc2cb 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -684,6 +684,101 @@ class BuilderThread(threading.Thread): result.out_dir = out_dir return result, do_config + def _write_toolchain_result(self, result, done_file, build_dir, + maybe_aborted, work_in_output): + """Write build result and toolchain information + + Args: + result (CommandResult): Result to write + done_file (str): Path to the 'done' file + build_dir (str): Build directory path + maybe_aborted (bool): True if the build may have been aborted + work_in_output (bool): Use the output directory as the work + directory and don't write to a separate output directory + """ + # Write the build result and toolchain information. + with open(done_file, 'w', encoding='utf-8') as outf: + if maybe_aborted: + # Special code to indicate we need to retry + outf.write(f'{RETURN_CODE_RETRY}') + else: + outf.write(f'{result.return_code}') + with open(os.path.join(build_dir, 'toolchain'), 'w', + encoding='utf-8') as outf: + print('gcc', result.toolchain.gcc, file=outf) + print('path', result.toolchain.path, file=outf) + print('cross', result.toolchain.cross, file=outf) + print('arch', result.toolchain.arch, file=outf) + outf.write(f'{result.return_code}') + + # Write out the image and function size information and an objdump + env = self.builder.make_environment(self.toolchain) + with open(os.path.join(build_dir, 'out-env'), 'wb') as outf: + for var in sorted(env.keys()): + outf.write(b'%s="%s"' % (var, env[var])) + + with open(os.path.join(build_dir, 'out-cmd'), 'w', + encoding='utf-8') as outf: + for cmd in result.cmd_list: + print(' '.join(cmd), file=outf) + + lines = [] + for fname in BASE_ELF_FILENAMES: + cmd = [f'{self.toolchain.cross}nm', '--size-sort', fname] + nm_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + if nm_result.stdout: + nm_fname = self.builder.get_func_sizes_file( + result.commit_upto, result.brd.target, fname) + with open(nm_fname, 'w', encoding='utf-8') as outf: + print(nm_result.stdout, end=' ', file=outf) + + cmd = [f'{self.toolchain.cross}objdump', '-h', fname] + dump_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + rodata_size = '' + if dump_result.stdout: + objdump = self.builder.get_objdump_file(result.commit_upto, + result.brd.target, fname) + with open(objdump, 'w', encoding='utf-8') as outf: + print(dump_result.stdout, end=' ', file=outf) + for line in dump_result.stdout.splitlines(): + fields = line.split() + if len(fields) > 5 and fields[1] == '.rodata': + rodata_size = fields[2] + + cmd = [f'{self.toolchain.cross}size', fname] + size_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + if size_result.stdout: + lines.append(size_result.stdout.splitlines()[1] + ' ' + + rodata_size) + + # Extract the environment from U-Boot and dump it out + cmd = [f'{self.toolchain.cross}objcopy', '-O', 'binary', + '-j', '.rodata.default_environment', + 'env/built-in.o', 'uboot.env'] + command.run_one(*cmd, capture=True, capture_stderr=True, + cwd=result.out_dir, raise_on_error=False, env=env) + if not work_in_output: + copy_files(result.out_dir, build_dir, '', ['uboot.env']) + + # Write out the image sizes file. This is similar to the output + # of binutil's 'size' utility, but it omits the header line and + # adds an additional hex value at the end of each line for the + # rodata size + if lines: + sizes = self.builder.get_sizes_file(result.commit_upto, + result.brd.target) + with open(sizes, 'w', encoding='utf-8') as outf: + print('\n'.join(lines), file=outf) + def _write_result(self, result, keep_outputs, work_in_output): """Write a built result to the output directory. @@ -729,88 +824,8 @@ class BuilderThread(threading.Thread): done_file = self.builder.get_done_file(result.commit_upto, result.brd.target) if result.toolchain: - # Write the build result and toolchain information. - with open(done_file, 'w', encoding='utf-8') as outf: - if maybe_aborted: - # Special code to indicate we need to retry - outf.write(f'{RETURN_CODE_RETRY}') - else: - outf.write(f'{result.return_code}') - with open(os.path.join(build_dir, 'toolchain'), 'w', - encoding='utf-8') as outf: - print('gcc', result.toolchain.gcc, file=outf) - print('path', result.toolchain.path, file=outf) - print('cross', result.toolchain.cross, file=outf) - print('arch', result.toolchain.arch, file=outf) - outf.write(f'{result.return_code}') - - # Write out the image and function size information and an objdump - env = self.builder.make_environment(self.toolchain) - with open(os.path.join(build_dir, 'out-env'), 'wb') as outf: - for var in sorted(env.keys()): - outf.write(b'%s="%s"' % (var, env[var])) - - with open(os.path.join(build_dir, 'out-cmd'), 'w', - encoding='utf-8') as outf: - for cmd in result.cmd_list: - print(' '.join(cmd), file=outf) - - lines = [] - for fname in BASE_ELF_FILENAMES: - cmd = [f'{self.toolchain.cross}nm', '--size-sort', fname] - nm_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - if nm_result.stdout: - nm_fname = self.builder.get_func_sizes_file( - result.commit_upto, result.brd.target, fname) - with open(nm_fname, 'w', encoding='utf-8') as outf: - print(nm_result.stdout, end=' ', file=outf) - - cmd = [f'{self.toolchain.cross}objdump', '-h', fname] - dump_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - rodata_size = '' - if dump_result.stdout: - objdump = self.builder.get_objdump_file(result.commit_upto, - result.brd.target, fname) - with open(objdump, 'w', encoding='utf-8') as outf: - print(dump_result.stdout, end=' ', file=outf) - for line in dump_result.stdout.splitlines(): - fields = line.split() - if len(fields) > 5 and fields[1] == '.rodata': - rodata_size = fields[2] - - cmd = [f'{self.toolchain.cross}size', fname] - size_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - if size_result.stdout: - lines.append(size_result.stdout.splitlines()[1] + ' ' + - rodata_size) - - # Extract the environment from U-Boot and dump it out - cmd = [f'{self.toolchain.cross}objcopy', '-O', 'binary', - '-j', '.rodata.default_environment', - 'env/built-in.o', 'uboot.env'] - command.run_one(*cmd, capture=True, capture_stderr=True, - cwd=result.out_dir, raise_on_error=False, env=env) - if not work_in_output: - copy_files(result.out_dir, build_dir, '', ['uboot.env']) - - # Write out the image sizes file. This is similar to the output - # of binutil's 'size' utility, but it omits the header line and - # adds an additional hex value at the end of each line for the - # rodata size - if lines: - sizes = self.builder.get_sizes_file(result.commit_upto, - result.brd.target) - with open(sizes, 'w', encoding='utf-8') as outf: - print('\n'.join(lines), file=outf) + self._write_toolchain_result(result, done_file, build_dir, + maybe_aborted, work_in_output) else: # Indicate that the build failure due to lack of toolchain tools.write_file(done_file, '2\n', binary=False) -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Move the ELF file processing logic from the for loop in _write_toolchain_result() into a separate _process_elf_file() method. The new method handles processing a single ELF file: - Running nm to get function sizes - Running objdump to get section headers and rodata size - Running size to get overall sizes - Writing output to appropriate files Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 88 ++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 35 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 1855e2cc2cb..d5c8c6141eb 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -684,6 +684,56 @@ class BuilderThread(threading.Thread): result.out_dir = out_dir return result, do_config + def _process_elf_file(self, result, fname, env): + """Process an ELF file to extract size information + + Runs nm, objdump, and size on the ELF file and writes the output + to appropriate files. + + Args: + result (CommandResult): Result containing build information + fname (str): ELF filename to process + env (dict): Environment variables for the commands + + Returns: + str: Size line with rodata size appended, or None if size failed + """ + cmd = [f'{self.toolchain.cross}nm', '--size-sort', fname] + nm_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + if nm_result.stdout: + nm_fname = self.builder.get_func_sizes_file( + result.commit_upto, result.brd.target, fname) + with open(nm_fname, 'w', encoding='utf-8') as outf: + print(nm_result.stdout, end=' ', file=outf) + + cmd = [f'{self.toolchain.cross}objdump', '-h', fname] + dump_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + rodata_size = '' + if dump_result.stdout: + objdump = self.builder.get_objdump_file(result.commit_upto, + result.brd.target, fname) + with open(objdump, 'w', encoding='utf-8') as outf: + print(dump_result.stdout, end=' ', file=outf) + for line in dump_result.stdout.splitlines(): + fields = line.split() + if len(fields) > 5 and fields[1] == '.rodata': + rodata_size = fields[2] + + cmd = [f'{self.toolchain.cross}size', fname] + size_result = command.run_one(*cmd, capture=True, + capture_stderr=True, + cwd=result.out_dir, + raise_on_error=False, env=env) + if size_result.stdout: + return size_result.stdout.splitlines()[1] + ' ' + rodata_size + return None + def _write_toolchain_result(self, result, done_file, build_dir, maybe_aborted, work_in_output): """Write build result and toolchain information @@ -724,41 +774,9 @@ class BuilderThread(threading.Thread): lines = [] for fname in BASE_ELF_FILENAMES: - cmd = [f'{self.toolchain.cross}nm', '--size-sort', fname] - nm_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - if nm_result.stdout: - nm_fname = self.builder.get_func_sizes_file( - result.commit_upto, result.brd.target, fname) - with open(nm_fname, 'w', encoding='utf-8') as outf: - print(nm_result.stdout, end=' ', file=outf) - - cmd = [f'{self.toolchain.cross}objdump', '-h', fname] - dump_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - rodata_size = '' - if dump_result.stdout: - objdump = self.builder.get_objdump_file(result.commit_upto, - result.brd.target, fname) - with open(objdump, 'w', encoding='utf-8') as outf: - print(dump_result.stdout, end=' ', file=outf) - for line in dump_result.stdout.splitlines(): - fields = line.split() - if len(fields) > 5 and fields[1] == '.rodata': - rodata_size = fields[2] - - cmd = [f'{self.toolchain.cross}size', fname] - size_result = command.run_one(*cmd, capture=True, - capture_stderr=True, - cwd=result.out_dir, - raise_on_error=False, env=env) - if size_result.stdout: - lines.append(size_result.stdout.splitlines()[1] + ' ' + - rodata_size) + line = self._process_elf_file(result, fname, env) + if line: + lines.append(line) # Extract the environment from U-Boot and dump it out cmd = [f'{self.toolchain.cross}objcopy', '-O', 'binary', -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Many functions in builderthread have too many arguments, but it seems easier to keep it that way. Silence the warnings. Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index d5c8c6141eb..bcebae5f327 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -221,6 +221,7 @@ class BuilderThread(threading.Thread): toolchain: Toolchain object to use for building, or None if not yet selected """ + # pylint: disable=R0913 def __init__(self, builder, thread_num, mrproper, per_board_out_dir, test_exception=False): """Set up a new builder thread""" -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Update the return type documentation format in make() and run_commit() to satisfy pylint's missing-return-type-doc check. Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/buildman/builderthread.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index bcebae5f327..dee31972e4b 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -252,7 +252,7 @@ class BuilderThread(threading.Thread): command.run_one() Returns: - CommandResult object + CommandResult: Result of the make operation """ return self.builder.do_make(commit, brd, stage, cwd, *args, **kwargs) @@ -661,9 +661,9 @@ class BuilderThread(threading.Thread): showed failure Returns: - tuple containing: - - CommandResult object containing the results of the build - - boolean indicating whether 'make config' is still needed + tuple: + CommandResult: Results of the build + bool: Indicates whether 'make config' is still needed """ # Create a default result - it will be overwritte by the call to # self.make() below, in the event that we do a build. -- 2.43.0
participants (1)
-
Simon Glass