From aee2231cea538c10a3cf054cbd5e2749813f5e59 Mon Sep 17 00:00:00 2001 From: Srikanth Patchava Date: Wed, 30 Sep 2026 23:33:19 -0700 Subject: [PATCH] fix: restore #146, #160 and #161 work dropped by the 09-30 batch merge The #141 merge (40d5a65) took an old copy of ebuild/cli/commands.py and deleted three merged features. It now has runner args for flash (#161), the lockfile read before resolving (#146) and ninja_command() (#160) again. #141's own change stays: executable_output_path comes from ebuild.build.layout. The #137 merge (18b3ee5) replaced #138's archive rule, which runs recreate_archive.py by path, with an inline python -c command. #138's helper and tests were still in the tree, so its rule is back. The helper checks that the archiver can run before it unlinks $out. The archive-member tests from #137 and #138 expected keep.o. Since #133, _object_path keeps the source extension, so the member is keep.c.o. Also fixes ruff's 13 trailing-whitespace errors in tests/unit/cli/test_commands_utils.py. Fixes #165 --- ebuild/build/ninja_backend.py | 24 +++-- ebuild/cli/commands.py | 147 +++++++++++++++++++++++--- tests/ebuild/test_ninja_backend.py | 6 +- tests/unit/cli/test_commands_utils.py | 26 ++--- tests/unit/test_ninja_backend.py | 8 +- 5 files changed, 171 insertions(+), 40 deletions(-) diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index d1a6e04..cd6acaa 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -171,9 +171,15 @@ def _object_path(self, target, src: str) -> Path: def _write_ninja(self) -> None: """Write the build.ninja file.""" ninja_path = self.build_dir / "build.ninja" - python_command = ( - subprocess.list2cmdline([sys.executable]) - if sys.platform == "win32" else shlex.quote(sys.executable) + # Invoke recreate_archive.py by absolute path so Ninja does not need + # `ebuild` on sys.path (cwd/PYTHONPATH checkouts, not only installs). + # Quote for the shell Ninja will use, and escape `$` so Ninja does not + # treat path fragments as variables. + helper = Path(__file__).with_name("recreate_archive.py") + archive_command = ( + subprocess.list2cmdline([sys.executable, str(helper)]) + if sys.platform == "win32" + else f"{shlex.quote(sys.executable)} {shlex.quote(str(helper))}" ).replace("$", "$$") lines = [ f"# Generated by ebuild", @@ -192,12 +198,12 @@ def _write_ninja(self) -> None: " description = LINK $out", "", "rule ar_rule", - # ar replaces supplied members but keeps omitted ones. Recreate - # the archive when Ninja rebuilds it so removed sources cannot - # survive as stale code. Python also works on Windows, unlike rm. - f' command = {python_command} -c "from pathlib import Path; import subprocess, sys; ' - 'Path(sys.argv[1]).unlink(missing_ok=True); ' - 'sys.exit(subprocess.call(sys.argv[2:]))" $out $ar rcs $out $in', + # `ar rcs` replaces named members but keeps omitted ones, so an + # incremental rebuild after removing a source can leave its .o in + # the archive. recreate_archive deletes $out first. A path-based + # helper keeps this portable (no `rm`) without requiring `import + # ebuild` at build time. + f" command = {archive_command} $out $ar rcs $out $in", " description = AR $out", "", # A shared_library edge names this rule. Without the rule the diff --git a/ebuild/cli/commands.py b/ebuild/cli/commands.py index e4f8b65..ad039bb 100644 --- a/ebuild/cli/commands.py +++ b/ebuild/cli/commands.py @@ -12,12 +12,13 @@ import glob import os import re +import shlex import shutil import subprocess import threading import sys from pathlib import Path -from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING +from typing import Any, Callable, Dict, List, Optional, Tuple, TYPE_CHECKING if TYPE_CHECKING: from ebuild.eos_ai.eos_hw_analyzer import HardwareProfile @@ -40,7 +41,7 @@ from ebuild.packages.builder import BuildError, PackageBuilder from ebuild.packages.cache import PackageCache from ebuild.packages.fetcher import FetchError, PackageFetcher -from ebuild.packages.lockfile import Lockfile +from ebuild.packages.lockfile import Lockfile, LockfileError from ebuild.packages.recipe import RecipeError from ebuild.packages.registry import create_registry, find_recipe_dirs from ebuild.packages.resolver import PackageResolver, ResolveError @@ -51,6 +52,80 @@ # Canonical recipe search path discovery _find_recipe_dirs = find_recipe_dirs +# yaml key for the runner args config +_RUNNER_ARGS_CONFIG_KEY = "runner_args" + + +def _resolve_runner_args( + cli_args: tuple, + env_key: str, + cfg_loader: "Callable[[], ProjectConfig]", + config_section: str, + config_key: str, + log: "Logger", + clear_args: bool = False, +) -> list[str]: + """Resolve tool arguments following a precedence chain: CLI > Environment > Configuration File. + + This function short-circuits to avoid unnecessary config file reads if CLI or environment + arguments are present. + + Args: + cli_args: Tuple of unparsed CLI arguments captured by Click. + env_key: The environment variable name to check for arguments. + cfg_loader: A callable that returns the parsed ProjectConfig. + config_section: The section name within the parsed ProjectConfig (e.g. 'flash_config'). + config_key: The key within the section mapping that holds the arguments list. + log: Logger instance for recording debug or warning messages. + clear_args: If true, explicitly clear the arguments, ignoring config and environment. + + Returns: + A list of string arguments to pass to the underlying tool. Returns empty list if none found. + """ + if clear_args: + log.debug("Runner args explicitly cleared via flag") + return [] + + # CLI args (highest precedence) + if cli_args: + args_list = list(cli_args) + log.debug(f"Loaded runner args from CLI: {args_list}") + return args_list + + # Environment variable + env_args = os.environ.get(env_key) + if env_args and env_args.strip(): + args_list = shlex.split(env_args.strip()) + log.debug(f"Loaded runner args from environment ({env_key}): {args_list}") + return args_list + + # Config file + try: + cfg = cfg_loader() + section = getattr(cfg, config_section, {}) + if not isinstance(section, dict): + section = {} + + config_args = section.get(config_key, []) + if isinstance(config_args, list): + args_list = [str(a) for a in config_args] + if args_list: + log.debug(f"Loaded runner args from config: {args_list}") + return args_list + elif isinstance(config_args, str): + args_list = shlex.split(config_args) + if args_list: + log.debug(f"Loaded runner args from config: {args_list}") + return args_list + else: + log.warning(f"Invalid format for {config_key} in config file: must be a list or string") + return [] + except FileNotFoundError: + pass # config is optional + except Exception as e: + log.warning(f"Failed to load config: {e}") + + return [] def _install_packages( @@ -81,16 +156,26 @@ def _install_packages( registry = create_registry(*recipe_dirs) log.debug(f"Registry: {registry.package_count} recipes from {[str(p) for p in registry.search_paths]}") + # The lockfile is read before resolving, so an unpinned package lands on + # the version the last resolution recorded rather than on whatever is + # newest now. It was only ever written before, which pinned nothing. + lock_path = cfg.source_dir / Lockfile.FILENAME + lockfile = Lockfile(lock_path) + try: + lockfile.load() + except LockfileError as e: + # A lock that cannot be read is a resolution the user has to settle, + # reported through the handler every caller of this function has. + raise ResolveError(str(e)) from e + if lockfile.package_names: + log.debug(f"Lockfile read: {lock_path} ({len(lockfile.package_names)} packages)") + resolver = PackageResolver(registry) requested = [{"name": p.name, "version": p.version} for p in cfg.packages] - resolved = resolver.resolve(requested) + resolved = resolver.resolve(requested, lockfile=lockfile) log.info(f"Packages to install: {', '.join(r.name + ' v' + r.version for r in resolved)}") - # Lockfile - lock_path = cfg.source_dir / Lockfile.FILENAME - lockfile = Lockfile(lock_path) - # Cache and fetcher pkg_cache_dir = build_dir / "packages" cache = PackageCache(pkg_cache_dir) @@ -1029,7 +1114,8 @@ def build(log: Logger, config_path: str, build_dir: str, backend: Optional[str], log.success(f"Generated {_shown(build_path / 'compile_commands.json')}") log.step("Invoking ninja...") - ninja_cmd = [sys.executable, "-m", "ninja", "-f", str(build_path / "build.ninja")] + from ebuild.build.dispatch import ninja_command + ninja_cmd = ninja_command() + ["-f", str(build_path / "build.ninja")] if log.verbose: ninja_cmd.append("-v") @@ -1572,21 +1658,42 @@ def firmware(log: Logger, config_path: str, build_dir: str, rtos: str, board: st raise SystemExit(1) -@cli.command() +@cli.command(context_settings=dict(ignore_unknown_options=True)) @click.argument("image", type=click.Path(exists=True)) @click.option("--tool", default="openocd", type=click.Choice(["openocd", "pyocd", "nrfjprog", "esptool", "stflash"]), help="Flash tool to use.") @click.option("--target", default="stm32f4", help="Target MCU/board.") @click.option("--address", default="0x08000000", help="Flash base address (hex).") -@click.option("--reset-after", is_flag=True, default=False, help="Reset target after flashing.") +@click.option( + "--reset-after", is_flag=True, default=False, help="Reset target after flashing." +) +@click.option( + "--config", + "config_path", + default="build.yaml", + type=click.Path(), + help="Path to build config.", +) +@click.option( + "--no-runner-args", + is_flag=True, + default=False, + help="Clear runner args (overrides config and environment).", +) +@click.argument("cli_args", nargs=-1, type=click.UNPROCESSED) @click.pass_obj def flash(log: Logger, image: str, tool: str, target: str, address: str, - reset_after: bool) -> None: + reset_after: bool, config_path: str, no_runner_args: bool, cli_args: tuple) -> None: """Flash a firmware image to the target device. Supports OpenOCD, pyOCD, nrfjprog, esptool, and st-flash. + Underlying tool arguments (runner args) can be set by, in order of precedence, + extra args on the CLI (unrecognized options will be passed as runner args), + by setting the EBUILD_FLASH_RUNNER_ARGS environment variable, + or by defining a `runner_args` list in the `flash` section of build.yaml. + Examples: ebuild flash firmware.bin --tool openocd --target stm32f4 @@ -1596,6 +1703,8 @@ def flash(log: Logger, image: str, tool: str, target: str, address: str, ebuild flash firmware.bin --tool esptool --address 0x10000 ebuild flash firmware.bin --tool pyocd --target nrf52840 --reset-after + + ebuild flash firmware.bin --tool esptool -- --port /dev/ttyUSB0 """ log.header("ebuild — Flash") @@ -1605,10 +1714,24 @@ def flash(log: Logger, image: str, tool: str, target: str, address: str, image_path = Path(image) addr = int(address, 0) + runner_args = _resolve_runner_args( + cli_args, + "EBUILD_FLASH_RUNNER_ARGS", + lambda: load_config(config_path), + "flash_config", + _RUNNER_ARGS_CONFIG_KEY, + log, + clear_args=no_runner_args, + ) + log.step(f"Flashing {image_path.name} to {target} via {tool}...") log.info(f" Address: {hex(addr)}") + if runner_args: + log.info(f" Runner args: {' '.join(runner_args)}") - do_flash(image_path, tool=tool, target=target, address=addr) + do_flash( + image_path, tool=tool, target=target, address=addr, extra_args=runner_args + ) log.success(f"Flash complete: {image_path.name}") if reset_after: diff --git a/tests/ebuild/test_ninja_backend.py b/tests/ebuild/test_ninja_backend.py index bbca780..1189fea 100644 --- a/tests/ebuild/test_ninja_backend.py +++ b/tests/ebuild/test_ninja_backend.py @@ -310,8 +310,10 @@ def link(): (source_dir / "removed.c").unlink() build() members = subprocess.check_output([ar, "t", str(archive)], text=True) - assert "removed.o" not in members.splitlines(), members - assert "keep.o" in members.splitlines(), members + # Objects keep their source extension (src/main.c -> main.c.o) so that + # start.c and start.S in one target cannot collide; see _object_path. + assert "removed.c.o" not in members.splitlines(), members + assert "keep.c.o" in members.splitlines(), members assert link().returncode != 0, "deleted function still links from stale code" # The retained function still links, so this is not merely a broken archive. diff --git a/tests/unit/cli/test_commands_utils.py b/tests/unit/cli/test_commands_utils.py index 9ce8dab..c6a5282 100644 --- a/tests/unit/cli/test_commands_utils.py +++ b/tests/unit/cli/test_commands_utils.py @@ -40,7 +40,7 @@ def test_resolve_runner_args_unit(tmp_path, monkeypatch, config_val, env_val, cl monkeypatch.setenv(env_key, env_val) else: monkeypatch.delenv(env_key, raising=False) - + config_file = tmp_path / "build.yaml" if config_val is not None: if isinstance(config_val, list): @@ -48,13 +48,13 @@ def test_resolve_runner_args_unit(tmp_path, monkeypatch, config_val, env_val, cl config_file.write_text(f'project:\n name: test\nsystem:\n test_args: [{args_str}]\n', encoding="utf-8") else: config_file.write_text(f'project:\n name: test\nsystem:\n test_args: {config_val}\n', encoding="utf-8") - + res = _resolve_runner_args( - cli_args=cli_val, - env_key=env_key, - cfg_loader=create_loader(str(config_file)), - config_section="system_config", - config_key="test_args", + cli_args=cli_val, + env_key=env_key, + cfg_loader=create_loader(str(config_file)), + config_section="system_config", + config_key="test_args", log=DummyLogger(), clear_args=clear_args_val, ) @@ -64,13 +64,13 @@ def test_resolve_runner_args_null_safety(): """Verify null safety with empty or non-dict sections.""" class FakeConfig: system_config = None # Explicitly None to test null safety - + res = _resolve_runner_args( - cli_args=(), - env_key="TEST_ENV_KEY", - cfg_loader=lambda: FakeConfig(), - config_section="system_config", - config_key="test_args", + cli_args=(), + env_key="TEST_ENV_KEY", + cfg_loader=lambda: FakeConfig(), + config_section="system_config", + config_key="test_args", log=DummyLogger() ) assert res == [] diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index a01d550..1ac5c98 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -565,19 +565,19 @@ def members(): ).splitlines() build() - assert set(members()) == {"keep.o", "removed.o"} + assert set(members()) == {"keep.c.o", "removed.c.o"} library.sources.remove("removed.c") (source_dir / "removed.c").unlink() # Drop the stale object file too: Ninja would not rebuild it, but a # real ``ar rcs`` update would still leave its member in the archive. - removed_obj = build_dir / "obj" / "helpers" / "removed.o" + removed_obj = build_dir / "obj" / "helpers" / "removed.c.o" if removed_obj.exists(): removed_obj.unlink() build() - assert "removed.o" not in members(), members() - assert "keep.o" in members(), members() + assert "removed.c.o" not in members(), members() + assert "keep.c.o" in members(), members() def test_archive_rule_works_without_ebuild_on_path(self, tmp_path): """The AR helper must not require ``import ebuild`` at build time.