Skip to content

fix(ebuild): backend precedence, out-of-tree --build-dir resolution, and recipe version ordering - #96

Merged
srpatcha merged 4 commits into
embeddedos-org:masterfrom
dhruv-joshi15:fix/assessment-improvement
Sep 1, 2026
Merged

fix(ebuild): backend precedence, out-of-tree --build-dir resolution, and recipe version ordering#96
srpatcha merged 4 commits into
embeddedos-org:masterfrom
dhruv-joshi15:fix/assessment-improvement

Conversation

@dhruv-joshi15

Copy link
Copy Markdown

Summary

Three independent correctness fixes, one commit each. They touch unrelated code but share a theme: eBuild doing something other than what the project declared, without saying so.

  • Fix 1ebuild build could report success having built none of the declared targets.
  • Fix 2--config pointing outside the current directory broke build loudly and configure silently.
  • Fix 3 — one recipe with a non-numeric version crashed package discovery for every recipe.

Each fix has:

  • a concrete reproduction with observed output,
  • a minimal, narrowly-scoped change,
  • regression tests observed failing against the unfixed code first, per TESTING.md,
  • a CHANGELOG.md entry under [Unreleased]/Fixed.

Totals: 7 files changed, +735 / −17, +41 regression tests.


Problems addressed

1. Declared targets overridden by backend auto-detection

Issue

  • A build.yaml declaring targets: was ignored whenever the project directory also contained an external build-system marker file.
  • eBuild ran the external tool instead, built none of the declared targets, produced no build directory — and exited 0 with Build completed successfully.
  • Reproduced for all five marker types: CMakeLists.txt, meson.build, Cargo.toml, Makefile, Kconfig.

Reproduction

$ cat build.yaml          # declares one executable target "app"
$ ls                      # build.yaml, src/, Makefile  (Makefile only holds "make flash")
$ ebuild build
[info] Auto-detected backend: make
   Using make backend...
   Building (make)...
[ok] Build completed successfully (make).
$ echo $?
0
$ ls _build
ls: _build: No such file or directory

Impact

  • A silent wrong result — CI goes green and the artifacts are simply absent.
  • The trigger is ordinary, not exotic:
    • a Makefile holding make flash / make openocd helpers is normal in embedded repositories;
    • a CMakeLists.txt belonging to one subcomponent is normal in any mixed tree.
  • Adding either file to a working eBuild project silently stops it being built, with no warning and no non-zero exit to catch it.

Root cause

  • detect_backend(source_dir) receives only a path. It inspects the filesystem and cannot see build.yaml, so a declared target list cannot influence it.
  • _resolve_backend_request accepted its answer unconditionally.
  • build then routed on if resolved_backend != "ninja" or not cfg.targets:, which sends the (external backend, has targets) combination into the dispatcher.
  • Nothing raises, because from the dispatcher's point of view nothing is wrong: it was handed make and it ran make correctly.

2. Out-of-tree --config build path inconsistency

Issue

  • ebuild build --config <subdir>/build.yaml failed.
  • ebuild configure --config <subdir>/build.yaml reported success while writing its output where a later build would not look.

Reproduction

$ ebuild build --config myproj/build.yaml
   Generating build.ninja in _build/...
[ok] Generated _build/build.ninja
[ok] Generated _build/compile_commands.json
   Invoking ninja...
ninja: error: loading '_build/build.ninja': No such file or directory
[error] Build failed.
$ echo $?
1

The output contradicts itself in adjacent lines. Two controls confirmed the diagnosis:

  • an absolute --build-dir succeeded;
  • running from inside the project succeeded.

Impact

  • --config is unusable for any out-of-tree invocation — the monorepo and CI case.
  • The configure half is worse than the build half: build fails loudly, but configure reports success and leaves an inconsistent tree, so the failure surfaces later and somewhere else.
  • Secondary consequence: with a cwd-relative build directory, two projects built from one working directory share a single ./_build and overwrite each other's build.ninja and objects.

Root cause

  • One relative path, two different bases.
  • NinjaBackend.generate() created and reported the build directory relative to the process working directory.
  • ninja was launched with cwd=cfg.source_dir — necessary, so build.yaml's relative source paths resolve — and given a relative -f, which it therefore resolved against the project.
  • The two bases coincide exactly when the working directory is the project directory: the documented golden path, and the only case the committed examples exercise.

3. Package registry crashes on non-numeric versions

Issue

  • Version ordering parsed every dot-separated component with int().
  • get(), list_packages() and list_all_versions() all raised ValueError for any version that is not purely numeric.

Reproduction

$ ls recipes/            # zlib.yaml (1.3.1), littlefs.yaml (v2.9.3)
$ ebuild list-packages
...
ValueError: invalid literal for int() with base 10: 'v2'
$ echo $?
1

The valid zlib recipe was not listed either. Separately, resolving a missing package against that registry raised ValueError instead of ResolveError.

Impact

  • One out-of-format recipe breaks package discovery for every package, not just its own.
  • The trigger is already present in this repository: recipes/littlefs.yaml downloads .../archive/refs/tags/v2.9.3.tar.gz, so a contributor copying that upstream tag into version: hits it immediately.
  • The same failure occurs for:
    • prereleases — 3.6.0-rc1 (a form mbedtls publishes),
    • distribution revisions — 1.2.13-1,
    • SemVer build metadata — 1.0.0+build2.
  • It also damaged an unrelated error path: PackageResolver's "package not found" message enumerates the registry to report what is available — so the diagnostic meant to help you was the thing that crashed.

Root cause

  • [int(x) for x in v.split('.')] used as a sort key, duplicated verbatim at three call sites, so all three failed identically.

What changed

Fix 1 — ebuild/cli/commands.py (22 lines in _resolve_backend_request)

  • When the backend was auto-detected and cfg.targets is non-empty, select the ninja backend and log the substitution, naming both opt-outs.
  • Explicit backends still win. backend: in build.yaml or --backend on the command line bypasses this branch entirely, so a project can keep both a target list and an external build.
  • detect_backend() is deliberately unchanged. It takes only a path and its ten tests assert pure filesystem probing; the targets-aware decision belongs in the CLI resolution layer, which already has cfg.

Fix 2 — ebuild/cli/commands.py (one helper + four call sites)

  • New _resolve_build_dir():
    • an absolute --build-dir is returned unchanged;
    • a relative one resolves against the directory containing build.yaml, as an absolute path.
  • Why absolute and not merely project-relative: a project-relative path is still re-interpreted by ninja running in cfg.source_dir, so --config myproj/build.yaml yields myproj/_build and ninja looks for myproj/myproj/_build/build.ninja. Absolute is the only form that means the same thing to the process writing the tree and the ninja reading it, and it keeps build.ninja independent of the directory it was generated from.
  • Why project-anchored and not cwd-anchored — this follows what the repository already assumes:
    • the committed examples keep _build/ beside each build.yaml;
    • README and demo.md both walk through that layout;
    • compile_commands.json already records directory as the source directory with build-dir-relative outputs;
    • it also stops two projects sharing one ./_build.
  • Applied to the four commands that pair --config with --build-dir and feed the ninja or package path: build, configure, install, test.
  • _run_native_tests gains the cwd its own comment already claimed it matched ebuild build on.
  • New _shown() helper prints build paths relative to the working directory where possible, so the in-project output documented in demo.md is byte-identical and no documentation change was needed.

Fix 3 — ebuild/packages/registry.py (one helper replacing three inline keys)

  • New _version_sort_key():
    • dot-separated integers keep exact numeric ordering, so 1.10.0 still sorts above 1.9.0;
    • anything else is not interpreted — ranked below every numeric version and ordered lexicographically among its peers.
  • Ranking out-of-format versions below is deliberate: with 3.6.0 and 3.6.0-rc1 both registered, get() returns the plain release rather than the prerelease.
  • Deliberately not a SemVer parser. The recipe format documents no version grammar, every bundled recipe uses dot-separated integers, and inventing prerelease semantics would exceed the defect.
  • No dependency added. No validation added — rejecting v2.9.3 at load time would break a legitimate recipe rather than fix anything.
  • resolver.py needed no change: it failed only through list_packages(), so fixing the registry fixes the error path too.

Test results — before and after

Regression suites (each observed failing against the unfixed code first)

Suite Tests Before the fix After the fix
tests/ebuild/test_backend_targets_precedence.py (new) 25 10 failed / 15 passed 25 passed
tests/ebuild/test_build_dir_resolution.py (new) 8 6 failed / 2 passed 8 passed
tests/ebuild/test_package_registry.py (+8) 9 8 failed / 1 passed 9 passed

In each suite the tests that pass in both states are the deliberate preservation guards — explicit backends still win, marker-only projects are unaffected, in-project and absolute build dirs are unchanged, and PR #51's ordering test is unmodified.

Existing tests covering the modified components

Files Result
test_cli_backend_resolution.py, tests/ebuild/test_dispatch.py, tests/unit/test_dispatch.py, test_golden_path_commands.py, test_build_failure_output.py, test_package_fetcher.py, test_resolver.py 99 passed, 0 failed

Full suite

Command Result
Before (all changes stashed) pytest tests/ 290 passed, 1 failed
After pytest tests/ 331 passed, 1 failed

290 + 41 new tests = 331.

The suite is not fully green — before or after

One test fails in both states:

tests/ebuild/test_ninja_backend.py::test_shared_library_uses_shared_link_rule
  • It is pre-existing and macOS-specific. It asserts command = $cc -shared, while _shared_flag() correctly emits -dynamiclib on Darwin — so the implementation is right and the assertion is platform-blind.
  • It is unrelated to these changes and deliberately left untouched; fixing it belongs in its own PR.
  • I compared the failing-test identifier sets before and after: identical. No failure was introduced or removed.

Validation beyond the unit tests

  • Real example builds. examples/hello_world and examples/multi_target were copied to a scratch directory and built both from inside the project and out-of-tree; binaries were produced and executed successfully. In-project console output is byte-identical to the block documented in demo.md.
  • Lint — zero new findings. flake8 --select=E,F,W --ignore=E501 is clean (exit 0) on all new/changed test files. The 9 findings on the two modified source files are pre-existing with identical codes at baseline (six E241 in generate_boot, three E121/E126 from PR Fix numeric version ordering in package registry #51's continuation indentation).
  • Line endings preserved. ebuild/cli/commands.py has mixed line endings (1849 CRLF of 2410 lines). Every edit was made at byte level with per-site line-ending detection; the CRLF count is identical before and after. git diff --check reports four "trailing whitespace" warnings, which a control test confirmed are the CR of pre-existing CRLF lines, not whitespace introduced here.
  • Not run: mypy and coverage measurement locally (CI runs mypy with continue-on-error: true); Linux and Windows legs.

Relationship to previous work

  • PR fix(dispatch): raise on unhandled backend instead of silent no-op #46 (commit 04f4951) addressed the case where an unsupported backend reached BackendDispatcher; its commit message records the symptom as "both methods silently did nothing, and the CLI then printed 'Build completed successfully' with exit code 0." It added RuntimeError branches so that case fails loudly.
    → While reviewing that path I found a separate unresolved case: a supported but incorrectly auto-detected backend could still override declared targets, and it does not raise because the dispatcher is doing its job correctly. The silent-success symptom that change eliminated for one cause remained reachable through another.

  • PR fix(cli): centralize backend resolution for configure #40 (commit f4c50c0) centralised backend resolution into _resolve_backend_request and added tests/ebuild/test_cli_backend_resolution.py. Fix 1 lands in that helper.
    → I verified beforehand that no existing test encoded the old precedence: across test_cli_backend_resolution.py, tests/ebuild/test_dispatch.py and tests/unit/test_dispatch.py, no test combines a non-empty targets: list with a marker file (test_cmake_takes_priority_over_makefile encodes marker-versus-marker priority).

  • Commit b56bf9d ("fix: ninja backend library linking, cwd path, and project generator duplication") addressed relative source path resolution: before it, ninja -C <build_dir> ran inside the build directory and resolved build.yaml's relative sources against it. The commit changed the invocation to ninja -f <build_dir>/build.ninja with cwd=cfg.source_dir, which fixes source resolution and works. I could find no pull request merge referencing b56bf9d in the repository history, so I am not attributing a PR number to it.
    → While reviewing that path I found the -f argument and the build output paths are relative as well, and are now read against that new working directory, while the Python side still resolved against the process working directory. The two therefore agree only when the working directory is the project directory — which is the only case the examples/*/_build/ artifacts committed in that same change exercise.

  • PR Fix numeric version ordering in package registry #51 (commit 03afa1a) addressed version ordering in list_all_versions(), changing sorted(versions.keys()) to a numeric key and adding the registry's first test (1.2.0 < 1.9.0 < 1.10.0). That fixed a real bug — lexicographic sort ranks 1.10.0 below 1.9.0.
    → While reviewing that path I found the numeric key has no tolerance for non-integer components, and its test uses only purely numeric versions, so the fragility was never exercised. That change propagated an existing key (introduced in 7a1cab3) to a third call site rather than introducing it. Fix 3 preserves its ordering guarantee — its test is unmodified and still passes — and adds the missing tolerance at all three sites.

  • PR fix(packages): honour explicitly requested package versions #57 (commit 02c48ce, fix/resolver-version-pinning) addressed version selection in resolver.py, collecting explicit pins before walking the graph so resolution is order-independent. It does not touch parsing or ordering, and is distinct from this change: selection decides which version is wanted, ordering decides how versions compare.

All of these were legitimate improvements that closed the failure modes they targeted. What is fixed here are the adjacent cases that remained outside their scope.


Additional considerations / limitations

  • Validated on macOS/arm64 only. Linux and Windows were not validated locally. Path.is_absolute() and Path.resolve() behave differently on Windows for drive-relative paths such as C:foo; the windows-2022 and ubuntu-22.04 CI legs are the check. TASKS.md T-002 already records an open Windows Ninja path defect, so that platform has known path issues independent of this change.

  • The pre-existing macOS failure remains (test_shared_library_uses_shared_link_rule) and was deliberately not fixed — it belongs in its own change.

  • Fix 1 is a behaviour change. A project relying on the old precedence — an unused targets: list plus a real external build — will now take the ninja path. The logged message names the one-line opt-out (backend: or --backend). I judged an explicit, logged, reversible override better than continuing to silently discard a declaration.

  • Fix 2 leaves clean, pipeline, system and firmware alone. None launches ninja from a different directory or shares the package cache, so none exhibits the defect. One consequence follows: after an out-of-tree build, ebuild clean run from the working directory will not find the project's _build. That is pre-existing — clean takes no --config, so it has no project to anchor to — but this change makes it easier to encounter.

  • Fix 3's version-ordering contract is deliberately limited. The ordering of out-of-format versions is a documented choice, not a specification: 1.2.13-1 sorts below 1.2.13, which is arguably wrong under Debian conventions. Such versions are outside the format the registry claims to order, and a fuller treatment should follow a written version contract rather than inferred semantics.


Scope

Other findings from a broader read of the repository were intentionally not bundled into this PR:

  • shared-library and transitive static-archive linking (depends: links only direct static_library targets);
  • toolchain resolution (compiler: clang currently resolves to cc = gcc);
  • malformed-recipe YAML escaping PackageRegistry.scan()'s skip logic;
  • missing -fPIC for shared_library targets (and the unreferenced _PIC_FLAGS constant);
  • several ebuild add and configure error paths that emit raw tracebacks;
  • the macOS test expectation noted above.

Each is reproducible and worth fixing, and each deserves its own reproduction, tests and review rather than turning three focused fixes into an unrelated refactor.


Commits

Commit Subject
b345bde fix(packages): tolerate non-numeric recipe versions when ordering
bba6f75 fix(cli): don't let backend auto-detection override declared targets
d1a752e fix(cli): anchor a relative --build-dir to the project

All three are DCO signed-off. Each fix is self-contained and independently revertable, and the history is bisectable — commit 2 was verified to pass the full suite on its own (323 passed, same single pre-existing failure).

The registry sorted versions with [int(x) for x in v.split(".")], so any
version that is not purely dot-separated integers raised ValueError from
get(), list_packages() and list_all_versions(). "v2.9.3" -- the upstream
tag form recipes/littlefs.yaml already downloads -- is enough to break
`ebuild list-packages` entirely, and it hides every other recipe in the
directory rather than just its own.

It also broke an unrelated error path. PackageResolver's "package not
found" message enumerates the registry to report what is available, so
asking for a missing package raised ValueError instead of the actionable
ResolveError. The diagnostic meant to help was the thing that crashed.

Replace the three duplicated inline keys with one _version_sort_key().
Dot-separated integers keep the numeric ordering added in 03afa1a, so
1.10.0 still sorts above 1.9.0 and that commit's test passes unchanged.
Anything else -- a prerelease, a distribution revision, a v-prefixed tag
-- is ranked below every numeric version and ordered lexicographically
rather than guessed at, so a plain release still wins over a prerelease
of the same number.

Deliberately not a SemVer parser: the recipe format documents no version
grammar, every bundled recipe uses dot-separated integers, and inventing
prerelease semantics would be a larger change than the defect warrants.

Regression tests: 8 added to tests/ebuild/test_package_registry.py,
covering all three lookups for four out-of-format version shapes, the
numeric-beats-prerelease choice, ordering determinism, and the resolver
error path. 8 failed before this change, 9 pass after.

Signed-off-by: Dhruv Joshi <dhruvjoshi1520@gmail.com>
detect_backend() receives only a path. It inspects the filesystem and
cannot see build.yaml, so a Makefile kept for `make flash` -- or a
CMakeLists.txt belonging to one subcomponent -- won over a build.yaml
that declared its own targets. The dispatcher ran the external tool, none
of the declared targets were built, no build directory was produced, and
`ebuild build` still printed "Build completed successfully" and exited 0.

04f4951 made the dispatcher raise for backends it does not implement,
which closed the silent-success path for unsupported backend names. This
is the inverse case: a backend the dispatcher supports and runs correctly
that was never the right choice for the project, so nothing raises.

When the backend was auto-detected and build.yaml declares targets, use
the ninja backend and log the substitution along with both ways to opt
out. An explicit `backend:` in build.yaml or --backend on the command
line still wins, so a project that wants both a target list and an
external build keeps that option.

detect_backend() itself is unchanged: it takes only a path, and its tests
assert pure filesystem probing. The targets-aware decision belongs in the
CLI resolution layer that f4c50c0 introduced, which already has cfg.

Verified beforehand that no existing test encoded the old precedence:
across tests/ebuild/test_cli_backend_resolution.py,
tests/ebuild/test_dispatch.py and tests/unit/test_dispatch.py, no test
combines a non-empty targets list with a backend marker file.

Regression tests: 25 added in
tests/ebuild/test_backend_targets_precedence.py, parametrised over all
five marker types -- the precedence fix, the end-to-end "reported success
without building" symptom, and three suites asserting that explicit
backends and marker-only projects are unaffected. 10 failed before this
change, 25 pass after.

Signed-off-by: Dhruv Joshi <dhruvjoshi1520@gmail.com>
Commit b56bf9d changed the ninja invocation from `ninja -C <build_dir>`
to `ninja -f <build_dir>/build.ninja` with cwd=cfg.source_dir, so that
the relative source paths recorded in build.ninja resolve against the
project rather than the build directory. That fixed source resolution.

The -f argument and the build output paths are relative too, and are now
read against that working directory, while the Python side still created
and reported the build directory against the process working directory.
The two bases agree only when the working directory is the project
directory -- the documented golden path, and the only case the
examples/*/_build/ trees committed in that same change exercise.

With --config naming a project elsewhere they diverge:

  * `ebuild build --config myproj/build.yaml` failed with "ninja: error:
    loading '_build/build.ninja': No such file or directory", one line
    after reporting "[ok] Generated _build/build.ninja".
  * `ebuild configure --config myproj/build.yaml` reported success having
    written build.ninja where the following build would not look.

Resolve a relative --build-dir against the directory containing
build.yaml, as an absolute path. Project-relative alone is not enough: it
is still re-interpreted by ninja running in cfg.source_dir, so
myproj/_build becomes myproj/myproj/_build. Absolute is the only form
that means the same thing to the process writing the tree and to the
ninja reading it, and it keeps build.ninja independent of the directory
it was generated from.

Anchoring to the project rather than the working directory follows what
the repository already assumes: the committed examples keep _build/
beside each build.yaml, README and demo.md walk through that layout, and
compile_commands.json already records "directory" as the source directory
with build-dir-relative outputs. It also stops two projects built from
one working directory sharing a single ./_build.

Applied to the four commands that pair --config with --build-dir and feed
the ninja or package path: build, configure, install and test.
_run_native_tests gains the cwd its own comment claimed it already
matched `ebuild build` on. Build paths are printed relative to the
working directory where possible, so the in-project output documented in
demo.md is byte-identical and needs no change.

clean, pipeline, system and firmware are left alone: none launches ninja
from a different directory or shares the package cache, so none exhibits
the defect. One consequence is that after an out-of-tree build, `ebuild
clean` run from the working directory will not find the project's
_build -- clean takes no --config, so this is pre-existing.

Two assertions in tests/ebuild/test_cli_backend_resolution.py compared
the build directory to a bare Path("_build"), the un-anchored value that
is the defect itself; they now pin the resolved location. Those tests
also used to create a real _build/ directory in the repository root as a
side effect, and no longer do.

Regression tests: 8 added in tests/ebuild/test_build_dir_resolution.py --
the out-of-tree regression, a relative --config, two projects not sharing
a build directory, a named relative --build-dir, configure/build
agreement, preservation guards for in-project and absolute build dirs,
and one real-gcc end-to-end test asserting the binary exists. 6 failed
before this change, 8 pass after.

Signed-off-by: Dhruv Joshi <dhruvjoshi1520@gmail.com>
srpatcha
srpatcha previously approved these changes Sep 1, 2026
@srpatcha
srpatcha merged commit d7aaf6a into embeddedos-org:master Sep 1, 2026
srpatcha pushed a commit that referenced this pull request Sep 1, 2026
…green

The six remaining failures were all the same shape as the ones already
fixed: work that landed as tests while its implementation was discarded
during conflict resolution, or two implementations of one thing left side
by side with the wrong one wired in.

* backend precedence (#96). The 22 lines that stop detect_backend()
  outranking a build.yaml that declares targets were gone; the test file
  survived. A Makefile kept for `make flash` still won, the dispatcher ran
  the external tool, nothing declared was built, and the build reported
  success. Restored from bba6f75.

* dispatch errors. Three mechanisms sat on top of each other:
  BackendError + _validate_backend, and UnknownBackendError + a
  _unknown_backend() -- twice, the class and the function each defined
  two ways, the later shadowing the earlier. _validate_backend ran first
  and preempted the per-step error with a wrong type and a message that
  contradicted itself by listing ninja as supported while rejecting it.
  Consolidated on the surviving pair; BackendError stays as an alias so
  callers that catch it keep working. The guard moved ahead of the mkdir
  it used to precede, so rejecting a backend no longer leaves a build
  directory behind.

* registry ordering. registry.py carried both version_sort_key and a
  private _version_sort_key, and every caller used the private one, which
  ranks any non-numeric version below every numeric one -- so v2.9.3,
  littlefs's own tag format, sorted below 1.10.0 and get() picked the
  wrong latest. Dropped the duplicate; the callers now use the tested
  public key. One assertion in tests/ebuild encoded the private key's
  ordering and is updated, which is the one judgement call here.

* system_config. Parsed and validated in load_config(), then never passed
  to ProjectConfig, so the whole [system] section was inert.

* footprint on macOS. Apple size(1) prints "__TEXT __DATA __OBJC others",
  and _SIZE_LINE matched that row too -- reading __OBJC, always 0, as bss.
  Every footprint measured on a Mac silently reported RAM containing no
  zero-initialised data. Falls back to `size -m` and sums sections, so
  bss is the real 4096 rather than 0.

* doctor (#72) and package (#77). Both commands were absent from the
  group; ebuild/system/doctor.py and ebuild/build/firmware_image.py were
  already on master, only the CLI registration was lost. Restoring
  `package` surfaced a genuine collision: integration.py defines another
  `package`, and register_commands() runs last, so it had been silently
  replacing the .efw one. Renamed that to `package-deliverable` -- see the
  PR description, this one wants a maintainer's eye.

Also adds a missing `import glob`: _serial_ports() raised NameError on
every call, the same defect as the missing `import re`.

525 passed, 0 failed. mypy goes from 23 errors to 15; ruff is unchanged.
Verified end to end: `ebuild doctor` reports the environment, and
`ebuild build` links a running Mach-O executable and prints a footprint.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants