Repository navigation
fix: repair master — four merges landed code that does not compile - #82
Conversation
`master` does not configure, and once it does it does not build, and once it builds one suite fails. Each break is the same failure: a PR verified green on its own branch, squash-merged onto a base that had moved, and nothing re-verified the result. 1. tests/CMakeLists.txt registered test_crypto_aes and test_crypto_sha512 twice (#59 landing beside the block that already had them), so CMake refused to configure at all: add_test given test NAME "test_crypto_aes" which already exists 2. eos_task_set_current_internal was defined twice in task.c and declared twice in kernel_internal.h, with two different behaviours — one clears g_current and marks the task RUNNING, the other silently returns. Kept the first, which is what the header documents ("Handles >= EOS_MAX_TASKS clear it") and what the mutex tests rely on; folded the second declaration's comment into the surviving one, since it records why the function exists. 3. sync.c would not compile: g_blocked_on, pi_propagate and task_valid were used and defined nowhere, alongside a reference to a struct member original_prio that mtx_t does not have. #67 and #55 implement incompatible designs for the same feature. #67 recomputes effective priority from effective = min(base_priority, priority of every waiter on every mutex the task holds) on every change; #55 saves and restores a value around each lock. Merging #55 after #67 deleted #67's implementation while leaving its call sites, so neither design was left intact. Restored #67's. It is the stronger of the two — save/restore is wrong when a task holds two mutexes and releases one, and wrong again when a task is boosted while already holding a mutex — and it subsumes what #55 fixes: recomputing on timeout is one of the cases it recomputes on, and it already rejects a full waiter table rather than blocking a task that was never enqueued. 4. #74's tests were merged without its parser change. Six assertions in test_config failed because a "- name:" item arriving while section was SEC_PKG_DEPS fell into the dependency branch, so the package after a deps list was swallowed as a dependency of the previous one. Restored the SEC_PKG_DEPS arm of the outer condition, nested the dependency handling as the else of the name check, and added the deps transition under SEC_PKG_BUILD. before cmake: configure fails after build clean, ctest 28/28 Also re-checked the package-overflow guard that arrived via #61/#65, since it landed in a different shape than #78 proposed: AddressSanitizer is clean on a 129-package config that previously overflowed, and the 129th entry is refused with a message rather than silently dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| memset(&g_tasks[slot], 0, sizeof(eos_task_t)); | ||
| g_tasks[slot].id = (uint8_t)slot; | ||
| g_tasks[slot].name = name; |
eos does not build for the target it exists to build for. From a clean
checkout of master, with the toolchain the CI job installs:
$ cmake -B barm -DCMAKE_TOOLCHAIN_FILE=toolchains/arm-cortex-m4.cmake
$ cmake --build barm
core/src/log.c:113:9: error: implicit declaration of function 'clock_gettime'
core/src/log.c:113:23: error: 'CLOCK_MONOTONIC' undeclared
get_timestamp_ms() had two branches, _WIN32 and "else, assume POSIX". Bare-
metal newlib is neither: arm-none-eabi declares neither clock_gettime nor
CLOCK_MONOTONIC, so the POSIX branch cannot compile for any Cortex-M target.
The macro is the precise test — where it is absent, so is the function.
There is a timebase on a target, eos_get_tick_ms() in the HAL, but core/ sits
below hal/ and must not depend upward, so log entries carry a zero timestamp
on a freestanding build until a port supplies one. Ordering within the ring
buffer is unaffected; only absolute time is missing.
Also fixed the format specifier one line down. uint32_t is `unsigned int` on
x86-64 and `unsigned long` on 32-bit ARM, so the literal %u was wrong on
exactly the target this build is for. It is PRIu32 now, which is right on
both.
Why CI did not catch it. The ARM job points CMAKE_TOOLCHAIN_FILE at
cmake/arm-cortex-m4.cmake; the file is at toolchains/. That fails loudly --
"Could not find toolchain file" -- so the job has been red rather than
silently passing, but it dies at configure and never reaches the compile step
where this bug lives.
The path is not new breakage: #75 fixed it, #79 and #51 carried the fix, and
#59 reverted it. That is the same stale-base merge pattern as the other three
repairs on this branch.
before ARM: configure fails on a missing toolchain file; with the real
path, 2 compile errors in core/src/log.c
after ARM: 70/70 targets built, artifacts report `architecture:
armv7e-m`; host: 28/28 ctest, unchanged
Kernel and services together measure 15.8 KB flash and 26 KB RAM on
Cortex-M4, which is the first real footprint number this repo has produced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verified merge order for all nine open eos PRsNothing has merged and
Final state of the full stack: 0 build errors, The two conflicts, and their resolutionsBoth are in #83 (GPS) — the branch predates #84 (stub-crypto signing) — its new Keep the Why the order matters#88, #89 and #90 were branched from a tree with #82 and #86 already applied, so they carry those changes. Merging them before #82 would work but makes the diffs harder to read. #83 and #84 go last only because putting the two conflicts at the end keeps the first seven merges mechanical. Anything merged out of this order still lands — the sequence is what I verified, not the only one that exists. |
|
Filed #92 to track the underlying defect. Re-verified the breakage today from a Still red, so this PR is still the gate. Every other open PR here is stacked One thing #92 records that this PR deliberately does not fix, so it does not get
Repairing the four breakages here does not stop the next overlapping pair from This is |
Deduplicating test_crypto_aes and test_crypto_sha512 left the grouped
comment that had been sitting above the block:
# --- test_crypto_aes / _ecc / _rsa / _sha512: were sitting in tests/ unused ---
That records how the files got here, which stops being interesting the
moment they are wired up. #93 fixes the same two duplicates and keeps the
per-test descriptions instead:
# --- test_crypto_aes: AES-128/192/256 ECB + CBC, NIST FIPS-197 / SP 800-38A ---
# --- test_crypto_ecc: ECC point arithmetic and ECDSA ---
# --- test_crypto_rsa: RSA modular exponentiation and verify ---
# --- test_crypto_sha512: SHA-512 against NIST vectors ---
Naming the standard each vector set comes from is worth more to the next
reader than a note about the file's history. Adopting that here so the
better wording survives the conflict between the two PRs.
Comment-only; no target, source or link line changes.
28/28 tests passed, 0 build errors
Co-authored-by: prakhar7017 <prakhar7017@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#94 repairs the same spliced priority-inheritance code as this PR, and converged on the same design independently — g_blocked_on, task_valid, pi_recompute, pi_propagate, recomputing from the invariant rather than saving and restoring a value around each lock. The two implementations of sync.c differ by nothing except these two comments, which explain the consequence rather than only the action: "...it would sleep forever." -> "...it would sleep forever. Refusing also avoids boosting the owner on behalf of a caller that never waits." "Timeout — leave the queue and drop the boost we were causing." -> "...so a waiter that gave up no longer keeps the owner running elevated." Both name the priority-inheritance effect of the branch, which is the part a reader is likely to get wrong when editing it later. Comment-only; sync.c is otherwise byte-identical to #94's. 28/28 tests passed, 0 build errors Co-authored-by: prakhar7017 <prakhar7017@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
embeddedos-org#92 stays open on this point: `required_status_checks` is null on master, so nothing builds the merge result before it becomes master. Four individually green PRs combined into code that had never been compiled in the form it landed in, and the gap that allowed it is still open. Marking a check required is a settings change, but it needs a name worth pointing at. `release` is skipped on every pull request, and a required check that is skipped never reports, so the pull request waits for a status that never arrives. Requiring the four real jobs individually works only until the next one is added and silently sits outside the requirement. `ci-gate` is one job, one stable name, that succeeds only if every other job that runs on a pull request succeeded. `if: always()` so a real failure shows as a red X rather than a pull request that hangs. Any non-success result fails it, `skipped` included -- a job that did not run did not verify anything, and treating that as a pass is the fail-open shape embeddedos-org#75, embeddedos-org#82 and embeddedos-org#84 removed elsewhere. tests/unit/test_ci_gate.py fails if a job is added without being wired into the gate. Verified by mutation: dropping a job from `needs`, removing `if: always()`, renaming the gate, and adding an unwired job are each caught. pyyaml added to both pytest installers, since the test parses the workflow and neither installer had it. Verified: 16 Python tests pass, ctest 34/34.
embeddedos-org#92 stays open on this point: `required_status_checks` is null on master, so nothing builds the merge result before it becomes master. Four individually green PRs combined into code that had never been compiled in the form it landed in, and the gap that allowed it is still open. Marking a check required is a settings change, but it needs a name worth pointing at. `release` is skipped on every pull request, and a required check that is skipped never reports, so the pull request waits for a status that never arrives. Requiring the four real jobs individually works only until the next one is added and silently sits outside the requirement. `ci-gate` is one job, one stable name, that succeeds only if every other job that runs on a pull request succeeded. `if: always()` so a real failure shows as a red X rather than a pull request that hangs. Any non-success result fails it, `skipped` included -- a job that did not run did not verify anything, and treating that as a pass is the fail-open shape embeddedos-org#75, embeddedos-org#82 and embeddedos-org#84 removed elsewhere. tests/unit/test_ci_gate.py fails if a job is added without being wired into the gate. Verified by mutation: dropping a job from `needs`, removing `if: always()`, renaming the gate, and adding an unwired job are each caught. pyyaml added to both pytest installers, since the test parses the workflow and neither installer had it. Verified: 16 Python tests pass, ctest 34/34.
* fix(build): restore the low-order Ed25519 suite that #93 removed `CI — eos` and `Python Unit Tests` are red on master (769191a), and both fail for one reason: these suites exist in tests/ but no add_executable() in tests/CMakeLists.txt builds them, so they never run: ['test_crypto_ed25519_loworder.c'] 3aa8644 (#93, "remove duplicate test targets from tests/CMakeLists.txt") removed all four lines that build and register test_crypto_ed25519_loworder. It was not a duplicate — there was exactly one registration before that commit, and it took it. What stopped running is the suite that guards #101: Ed25519 rejecting low-order public keys. A low-order key makes every term of the verification equation collapse to the identity regardless of the message, so a signature of all zeros verifies against any content at all. The fix is still in services/crypto/src/ed25519_verify.c and still correct; nothing has been checking it since #93 merged, and nothing would have noticed if a later change had removed it too. Restored verbatim from 3aa8644^, in its original position before test_crypto_sha512. No other file changes. The guard that caught this is test_cmake_test_registration.py, added recently. It did exactly its job — this is the first thing it found. Verified: test_cmake_test_registration.py -> 5 passed (1 failed on master) pytest tests/ -> 15 passed (14 passed 1 failed on master) test_crypto_ed25519_loworder -> 5/5 tests passed ctest -> 39/39 passed (master builds 38) * ci: add one job branch protection can require #92 stays open on this point: `required_status_checks` is null on master, so nothing builds the merge result before it becomes master. Four individually green PRs combined into code that had never been compiled in the form it landed in, and the gap that allowed it is still open. Marking a check required is a settings change, but it needs a name worth pointing at. `release` is skipped on every pull request, and a required check that is skipped never reports, so the pull request waits for a status that never arrives. Requiring the four real jobs individually works only until the next one is added and silently sits outside the requirement. `ci-gate` is one job, one stable name, that succeeds only if every other job that runs on a pull request succeeded. `if: always()` so a real failure shows as a red X rather than a pull request that hangs. Any non-success result fails it, `skipped` included -- a job that did not run did not verify anything, and treating that as a pass is the fail-open shape #75, #82 and #84 removed elsewhere. tests/unit/test_ci_gate.py fails if a job is added without being wired into the gate. Verified by mutation: dropping a job from `needs`, removing `if: always()`, renaming the gate, and adding an unwired job are each caught. pyyaml added to both pytest installers, since the test parses the workflow and neither installer had it. Verified: 16 Python tests pass, ctest 34/34. * ci: gate every workflow that reports on a pull request, not just ci.yml Addresses the review on #121. Finding 1 was right and it was the important one: ci.yml is one of nine workflows that trigger on pull_request, so requiring `CI Gate` and stopping there left CodeQL, the QEMU boot test, the ARM64 kernel cross-build, the Python suites and the SBOM validation outside the requirement -- while the test written to stop that rot only ever read ci.yml. The scan now covers .github/workflows/*.yml, and REQUIRED_CHECKS pins the name a maintainer must require for each. Adding a workflow, or a job to one, fails the suite until the registry is updated. Six workflows are now covered. Two needed a gate job that did not exist (build.yml) or did not do its job (eos-simulation.yml, below). Three define a single job whose own name is stable and requirable, so they need no gate, and a test keeps them single-job. Three gate nothing on purpose -- auto-assign, claude-code-review and book-build all skip on ordinary pull requests, and a required check that is skipped never reports -- each recorded with its reason. eos-simulation.yml's `integration-summary` already had the shape of a gate: the right needs, if: always(), and a printed PASS/FAIL per dependency. It only exited non-zero for simulate-qemu, so build-kernel, test-middleware and test-cad-pipeline could all fail while it reported success. Anyone requiring that check would have believed three things were gated that were not. Finding 2: _only_runs_on_tags() was a substring test for "refs/tags", which also matches the inverted condition -- a job running on every pull request would have been classified as tag-only and dropped from the requirement silently. Replaced with a pinned CANNOT_RUN_ON_PR map, plus an assertion that each excluded job really does carry the condition claimed for it. Finding 3: the rule is now .github/scripts/ci-gate-check.sh, executed by the tests against real inputs -- success, failure, skipped, cancelled, mixed, and an empty context -- instead of the workflow's run: text being grepped for `!= "success"`. A test that greps an implementation passes for those tokens in a comment and fails for a correct rewrite. Every gate calls the one script, and a test enforces that too. Finding 4: dropped `2>/dev/null || true` from the pip install in python-tests.yml. Verified by mutation, each caught: a new pull_request workflow with no gate; a job added to build.yml outside its gate; a gate losing if: always(); a required display name renamed; the shared rule weakened to ignore `skipped`; and integration-summary reverted to gating only simulate-qemu. 28 Python tests pass, ctest 34/34, all 19 workflow files parse. * ci: let the full-stack gate run at all, and test that a gate can `Full-stack integration summary` invoked .github/scripts/ci-gate-check.sh with no `actions/checkout` step, so on this head the step exited 127 on a missing file. The gate's rule never ran: it was red because the script was not in the workspace, not because a dependency failed, and it could never be green -- so a maintainer following this PR's own instruction to require that name would have made master permanently unmergeable. That is the mirror image of the fail-open shape this PR exists to remove, and worse in practice: fail-open lets bad code through, this blocks everything. `test_every_gate_invokes_the_shared_script` passed on that commit, because it greps the job's `run:` text for the script name and a job that cannot execute the script still names it. That is the one remaining grep-the-YAML test in a file whose own docstring says the rule should be executed rather than pattern-matched, and it is exactly the test that should have caught this. So the fix and the test that would have caught it land together: - eos-simulation.yml: `actions/checkout@v4` as the first step of integration-summary, as ci-gate and build-gate already have. - test_every_gate_checks_out_the_repository_before_running_the_script: any gate whose `run:` names a repo-relative script must also have a checkout step. Fails without the fix. Three further hardenings to the same file: - The NOT_REQUIRED reasons were unverified free text and one had already drifted: book-build.yml was recorded as skipping because build-pdf is conditional, but its `validate` job carries no `if:` at all and the real reason it cannot be required is the `paths: ['docs/book/**']` filter on its pull_request trigger -- the never-arrives failure mode this PR is careful about everywhere else. The reason is corrected, and a test now asserts each NOT_REQUIRED workflow either carries paths/paths-ignore or gates every job behind an `if:`. auto-assign.yml and claude-code-review.yml pass it as written. - test_gate_script_exists_and_is_executable checked only `is_file()`. The gates pipe into the path itself, which needs mode +x; it is committed 100755, but `git update-index --chmod=-x` or a core.fileMode=false checkout would break all three gates with the suite green. It now asserts os.access(X_OK), and one case invokes the script directly rather than via `bash`, so the invocation path the workflows use is covered. - `_triggers_on_pull_request` handled only the mapping form of `on:`. `on: pull_request` and `on: [push, pull_request]` are both valid and both made a workflow invisible to the scan -- silently unclassified, which is the rot this file exists to catch. Latent, not live: all 19 current workflow files use the mapping form. Also: the comment at build.yml:174 pointed at `.github/STANDARDS.md`, which does not exist in this repository. STANDARDS.md is the org-wide file at embeddedos-org/.github; the comment now links it. Each of the four new assertions was mutation-checked: removing the checkout, chmod -x on the script, dropping book-build's paths filter, and rewriting a workflow to the list form of `on:` each fail the suite and each is caught by the intended test. pytest tests/ -q: 31 passed (18 -> 21 in test_ci_gate.py). ctest Release: 34/34. * ci: make each gate report under its own name The run that proved the checkout fix also showed the shared script naming the wrong check. `Full-stack integration summary` failed and logged: ::error::CI Gate failed. These jobs did not succeed: build-kernel: failure simulate-qemu: skipped The diagnosis is right and the check name is not: "CI Gate" was hardcoded in ci-gate-check.sh, so two of the three gates report a failure under a check name that is not theirs. In a pull request whose subject is checks that report something other than what they measured, that is not a cosmetic detail -- a maintainer reading the log of a red merge is told to go look at a check that is green. The script now takes `GATE_NAME` (defaulting to "Gate"), each gate step sets it to its own display name, and two tests hold it in place: one runs the script with GATE_NAME set and asserts the message uses it, one asserts each gate declares the display name that REQUIRED_CHECKS says it reports under. Both mutation-checked -- hardcoding the name in the script fails the first, giving build-gate the wrong name fails the second. pytest tests/ -q: 33 passed. --------- Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
P0.
masterdoes not configure; once it does it does not build; once it builds one suite fails.Every break has the same cause: a PR verified green on its own branch, squash-merged onto a base that had moved, and nothing re-verified the result. eBoot#58 diagnosed the identical pattern there.
1. CMake will not configure
#59 added registrations for
test_crypto_aesandtest_crypto_sha512beside a block that already had them. Git merged it cleanly — this is not a textual conflict, which is why it was not caught. I flagged this exact hunk on #59 before it merged.2.
eos_task_set_current_internaldefined twiceTwo implementations with different behaviour — one clears
g_currentand marks the task RUNNING, the other silently returns. Kept the first: it is what the header documents ("Handles >= EOS_MAX_TASKS clear it") and what the mutex tests rely on. The second declaration's comment is folded into the survivor, since it records why the function exists at all.3.
sync.cdoes not compile — two PI designs spliced togetherg_blocked_on,pi_propagateandtask_validare used and defined nowhere;original_priois not a member ofmtx_t.#67 and #55 implement incompatible designs for the same feature. #67 recomputes effective priority from
on every change; #55 saves and restores a value around each lock. Merging #55 after #67 deleted #67's implementation while leaving its call sites, so neither design survived intact.
Restored #67's. Save/restore is wrong when a task holds two mutexes and releases one, and wrong again when a task is boosted while already holding a mutex. It also subsumes #55: recomputing on timeout is one of the cases it recomputes on, and it already rejects a full waiter table rather than blocking a task that was never enqueued.
4. #74's tests merged without #74's parser change
Six assertions failed because a
- name:item arriving whilesection == SEC_PKG_DEPSfell into the dependency branch, so the package after adeps:list was swallowed as a dependency of the previous one.Verification
cmakectestAlso re-checked the package-overflow guard that arrived via #61/#65, since it landed in a different shape than #78 proposed: AddressSanitizer is clean on the 129-package config that previously overflowed, and the 129th entry is refused with a message rather than silently dropped.
🤖 Generated with Claude Code
Update — eos does not cross-compile for Cortex-M4
Pushed a second commit. With the ARM toolchain now available on my machine I built for the target this project exists to build for, and it fails:
get_timestamp_ms()had two branches —_WIN32, and "else, assume POSIX". Bare-metal newlib is neither.arm-none-eabideclares neitherclock_gettimenorCLOCK_MONOTONIC, so the POSIX branch cannot compile for any Cortex-M target.Also fixed one line down:
uint32_tisunsigned inton x86-64 andunsigned longon 32-bit ARM, so the literal%uwas wrong on exactly the target this build is for.PRIu32is right on both.Why CI did not catch it
The ARM job points
CMAKE_TOOLCHAIN_FILEatcmake/arm-cortex-m4.cmake; the file is attoolchains/. That fails loudly — "Could not find toolchain file" — so the job has been red, not silently passing. But it dies at configure and never reaches the compile step where this bug lives.The path is not new breakage: #75 fixed it, #79 and #51 carried the fix, and #59 reverted it — the same stale-base pattern as the other three repairs here.
Result
core/src/log.carmv7e-mctestKernel and services together measure 15.8 KB flash, 26 KB RAM on Cortex-M4 — the first real footprint number this repo has produced.