From b1b6c05db70240c37204bae6d090517ce1e87417 Mon Sep 17 00:00:00 2001 From: Dhruv Joshi Date: Tue, 1 Sep 2026 22:53:37 +0530 Subject: [PATCH 1/3] fix(ci): ensure signing tests run with declared dependencies --- .github/workflows/ci.yml | 9 +- requirements.txt | 1 + tests/unit/test_requirements.py | 152 ++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_requirements.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9135584..1cfbad9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,7 +36,14 @@ jobs: cmake ninja-build gcc-12 g++-12 \ gcc-arm-none-eabi binutils-arm-none-eabi \ lcov gcovr python3-pip - pip3 install pytest pytest-cov + # Install from requirements.txt rather than a hand-maintained list. + # tests/unit/test_sign_image.py guards itself with + # pytest.importorskip("cryptography"), so a dependency that is + # declared nowhere does not fail the job -- it skips the 14 cases + # that pin the signed header format and the run still goes green. + # pytest-cov stays separate: it is a CI-only coverage plugin, not a + # dependency of anything in the repository. + pip3 install -r requirements.txt pytest-cov - name: Configure (host) run: | diff --git a/requirements.txt b/requirements.txt index 9250803..6b225cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ pyyaml>=6.0 pytest>=9.0 pyserial>=3.5 +cryptography>=41.0 diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py new file mode 100644 index 0000000..27c9226 --- /dev/null +++ b/tests/unit/test_requirements.py @@ -0,0 +1,152 @@ +"""Regression tests for the Python dependencies the test suite needs to run. + +tests/unit/test_sign_image.py guards itself with +pytest.importorskip("cryptography"). That is the right behaviour for a +contributor who has not installed the signing tools, but it also means an +undeclared dependency fails nothing: the 14 cases that pin the signed .efw +header wire format are skipped and the run still reports success. + +cryptography is imported by tools/sign_image.py and tools/eos_sign.py, but it +was named neither in requirements.txt nor by the CI job that runs pytest -- +that job installed a hand-written list instead of the repository requirements +-- so those 14 tests had never executed in CI. + +A hand-maintained dependency list beside requirements.txt is the thing that +goes stale, so these check that the list is the requirements file, rather than +checking any particular package name twice over. + +These parse requirements.txt and the workflow files as text, in the style of +test_cmake_test_registration.py, so no cmake, compiler, network or GitHub API +is needed. +""" + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +REQUIREMENTS = REPO_ROOT / "requirements.txt" +WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows" +SIGN_IMAGE_SUITE = Path(__file__).resolve().parent / "test_sign_image.py" + +# A requirement line: the distribution name, up to any extras, version +# specifier or environment marker. +REQUIREMENT_NAME_RE = re.compile(r"^\s*([A-Za-z0-9][A-Za-z0-9._-]*)") + +# Job ids are the only keys at two-space indent inside a workflow's jobs: block. +JOB_RE = re.compile(r"^ ([A-Za-z0-9_-]+):$", re.M) + +# `pytest tests/` or `python3 -m pytest tests/ -v ...` +RUNS_PYTEST_RE = re.compile(r"\bpytest\b[^\n]*\btests/") + +# `pip install -r requirements.txt`, with or without the pip3 spelling. +INSTALLS_REQUIREMENTS_RE = re.compile(r"\bpip3?\s+install\b[^\n]*-r\s+requirements\.txt") + + +def _strip_comments(text): + """Drop whole-line YAML comments so prose about pytest is not mistaken + for a step that runs it.""" + return "\n".join( + line for line in text.splitlines() if not line.lstrip().startswith("#") + ) + + +def _declared_requirements(): + """Distribution names declared in requirements.txt, lowercased.""" + names = set() + for line in REQUIREMENTS.read_text(encoding="utf-8").splitlines(): + line = line.split("#", 1)[0].strip() + if not line or line.startswith("-"): + continue + match = REQUIREMENT_NAME_RE.match(line) + if match: + names.add(match.group(1).lower()) + return names + + +def _workflow_jobs(): + """(workflow name, job id, job text) for every job in .github/workflows.""" + jobs = [] + for workflow in sorted(WORKFLOWS_DIR.glob("*.yml")): + text = _strip_comments(workflow.read_text(encoding="utf-8")) + + start = text.find("\njobs:") + if start == -1: + continue + body = text[start:] + + headers = list(JOB_RE.finditer(body)) + for i, header in enumerate(headers): + end = headers[i + 1].start() if i + 1 < len(headers) else len(body) + jobs.append((workflow.name, header.group(1), body[header.start():end])) + return jobs + + +def _importorskip_modules(): + """Module names test_sign_image.py refuses to run without.""" + text = SIGN_IMAGE_SUITE.read_text(encoding="utf-8") + return set(re.findall(r"importorskip\(\s*[\"']([A-Za-z0-9._-]+)[\"']", text)) + + +def test_cryptography_is_declared_in_requirements(): + """Pin the specific dependency that was missing, by name.""" + assert "cryptography" in _declared_requirements(), ( + "cryptography is not declared in requirements.txt -- it is imported by " + "tools/sign_image.py and tools/eos_sign.py, and without it " + "tests/unit/test_sign_image.py skips all 14 of its cases instead of " + "running them" + ) + + +def test_every_importorskip_dependency_is_declared(): + """A suite may only opt out of running for a dependency the repo declares. + + Guards the general case: adding a new importorskip for some package that + requirements.txt never names would silently park that suite in the skipped + column, exactly as happened to the signing tests. + """ + modules = _importorskip_modules() + assert modules, ( + "expected test_sign_image.py to guard itself with " + "pytest.importorskip(...); if that guard is gone this test no longer " + "checks anything and should be updated" + ) + + declared = _declared_requirements() + undeclared = sorted(name for name in modules if name.lower() not in declared) + + assert not undeclared, ( + "test_sign_image.py skips itself when these modules are missing, but " + f"requirements.txt does not declare them, so they will never be " + f"installed and the suite will never run: {undeclared}" + ) + + +def test_jobs_that_run_pytest_install_the_repository_requirements(): + """The environment the tests run in has to come from requirements.txt. + + Installing a hand-written package list beside requirements.txt is what let + cryptography go missing: the list was complete enough for the suite to + collect and import, so nothing failed -- the signing cases just skipped. + """ + pytest_jobs = [ + (workflow, job, text) + for workflow, job, text in _workflow_jobs() + if RUNS_PYTEST_RE.search(text) + ] + + assert pytest_jobs, ( + "no workflow job appears to run pytest over tests/; if the Python " + "suite moved, this guard needs to move with it" + ) + + missing = [ + f"{workflow}:{job}" + for workflow, job, text in pytest_jobs + if not INSTALLS_REQUIREMENTS_RE.search(text) + ] + + assert not missing, ( + "these jobs run pytest over tests/ but never install from " + "requirements.txt, so a declared dependency is absent at run time and " + f"the suites that need it skip while the job still passes: {missing}" + ) From dc321126996640944ba384bc1692760706f00000 Mon Sep 17 00:00:00 2001 From: Dhruv Joshi Date: Tue, 1 Sep 2026 22:53:44 +0530 Subject: [PATCH 2/3] fix(core): preserve boot log head when flash write fails --- core/boot_log.c | 10 +++++++++- tests/unit/test_boot_log.c | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/core/boot_log.c b/core/boot_log.c index c270f02..78e08e4 100644 --- a/core/boot_log.c +++ b/core/boot_log.c @@ -41,7 +41,15 @@ void eos_boot_log_append(uint32_t event, uint32_t slot, uint32_t detail) uint32_t offset = log_head * sizeof(eos_boot_log_entry_t); uint32_t addr = ops->log_addr + offset; - eos_hal_flash_write(addr, &entry, sizeof(entry)); + /* Advance the head only once the entry is actually on flash. Advancing + * unconditionally skips a slot that was never written, and the head is + * persisted in the boot control block, so the gap survives the reset: + * eos_boot_log_read() then hands the reader erased flash as if it were an + * entry, and the slot is never reused. eos_boot_log_clear() guards its own + * state change against a failed erase for the same reason. */ + int rc = eos_hal_flash_write(addr, &entry, sizeof(entry)); + if (rc != EOS_OK) + return; log_head = (log_head + 1) % EOS_BOOT_LOG_MAX; } diff --git a/tests/unit/test_boot_log.c b/tests/unit/test_boot_log.c index 30503f1..5770780 100644 --- a/tests/unit/test_boot_log.c +++ b/tests/unit/test_boot_log.c @@ -239,6 +239,43 @@ static void test_clear_reports_erase_failure(void) ASSERT(eos_boot_log_get_head() == 1); } +/* The same reasoning on the write path. append() cannot report a failure -- + * it returns void -- but it can decline to advance past a slot it never wrote. + * The head is persisted in the boot control block, so advancing anyway leaves + * a permanent hole: the slot is skipped for good and reads back as erased + * flash that a reader cannot tell apart from a real entry. */ +static void test_append_does_not_advance_head_when_write_fails(void) +{ + eos_boot_log_init(0); + eos_boot_log_append(EOS_LOG_BOOT_START, EOS_SLOT_A, 111); + ASSERT(eos_boot_log_get_head() == 1); + + write_result = EOS_ERR_FLASH; + eos_boot_log_append(EOS_LOG_BOOT_FAIL, EOS_SLOT_A, 222); + + /* Head stays put and slot 1 is still erased flash. */ + ASSERT(eos_boot_log_get_head() == 1); + + eos_boot_log_entry_t entry; + ASSERT(eos_boot_log_read(1, &entry) == EOS_OK); + ASSERT(entry.event == 0xFFFFFFFFu); + + /* The next successful append reuses that slot instead of skipping it. */ + write_result = EOS_OK; + eos_boot_log_append(EOS_LOG_CONFIRM, EOS_SLOT_B, 333); + ASSERT(eos_boot_log_get_head() == 2); + + ASSERT(eos_boot_log_read(1, &entry) == EOS_OK); + ASSERT(entry.event == EOS_LOG_CONFIRM); + ASSERT(entry.slot == EOS_SLOT_B); + ASSERT(entry.detail == 333); + + /* The entry written before the failure is untouched. */ + ASSERT(eos_boot_log_read(0, &entry) == EOS_OK); + ASSERT(entry.event == EOS_LOG_BOOT_START); + ASSERT(entry.detail == 111); +} + static void test_entry_layout_is_stable(void) { /* The log is parsed by host tooling and by application firmware through @@ -261,7 +298,8 @@ int main(void) RUN(test_read_rejects_bad_arguments); RUN(test_clear_erases_the_sector_and_resets_head); RUN(test_clear_reports_erase_failure); + RUN(test_append_does_not_advance_head_when_write_fails); RUN(test_entry_layout_is_stable); - printf("\n%d/10 tests passed\n", tests_passed); + printf("\n%d/11 tests passed\n", tests_passed); return 0; } From 7521325ddb4790e217cf28a6790361bf4e937227 Mon Sep 17 00:00:00 2001 From: Dhruv Joshi Date: Tue, 1 Sep 2026 22:53:58 +0530 Subject: [PATCH 3/3] fix(build): remove avoidable host build warnings --- core/boot_menu.c | 8 ++++++++ tests/CMakeLists.txt | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/boot_menu.c b/core/boot_menu.c index 4ab05a8..71b2be5 100644 --- a/core/boot_menu.c +++ b/core/boot_menu.c @@ -12,13 +12,21 @@ #include "eos_bootctl.h" #include +/* eos_hal_uart_write()/eos_hal_uart_read() are compatibility macros over the + * HAL's single-UART API (eos_hal_uart_send()/eos_hal_uart_recv()), so they + * expand without their port argument and it reads as unused under -Wextra. + * The parameter is kept because eos_boot_menu_config_t carries a uart_port and + * the board ports address one; it starts being honoured the moment the HAL + * grows a per-port call. */ static void uart_puts(uint8_t port, const char *str) { + (void)port; eos_hal_uart_write(port, (const uint8_t *)str, strlen(str)); } static int uart_getc(uint8_t port, uint32_t timeout_ms) { + (void)port; uint8_t ch; int rc = eos_hal_uart_read(port, &ch, 1, timeout_ms); return (rc == EOS_OK) ? (int)ch : -1; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f221ba2..8398eac 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -27,8 +27,12 @@ target_link_libraries(eboot_test_secure_boot PRIVATE eboot_core) add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot) # --- test_recovery: UART recovery write range --- +# eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on +# the link line twice -- ahead of the target that needs it, which is the wrong +# order for a left-to-right archive scan, so CMake appended it again anyway and +# the linker reported the duplicate. The transitive dependency covers it. add_executable(eboot_test_recovery unit/test_recovery.c) -target_link_libraries(eboot_test_recovery PRIVATE eboot_core eboot_stage1) +target_link_libraries(eboot_test_recovery PRIVATE eboot_stage1) add_test(NAME test_recovery COMMAND eboot_test_recovery) # --- test_fw_transport: UART raw/XMODEM/YMODEM firmware transport ---