fix(ebuild): preserve toolchain linker flags for shared libraries

[SJ-14-SJ]

Reuse toolchain linker flags and sysroot when linking shared libraries, matching executable targets. Add manifest and real CLI regression tests.

Implemented and tested with assistance from OpenAI Codex.

Summary

Type of Change

• eat — New feature
• ix — Bug fix
• docs — Documentation only
• style — Formatting, no code change
• [ ]
  efactor — Code restructuring without behavior change
• est — Add or fix tests
• �uild — Build system or dependency changes
• ci — CI/CD pipeline changes
• perf — Performance improvement

Changes

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure)
• Integration tests pass
• Manual testing performed
• New tests added for new functionality

Pre-Submission Checklist

• Code compiles without warnings (-Wall -Wextra -Werror for C)
• All existing tests pass
• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow (): convention
• Branch is rebased on latest master

Related Issues

Screenshots / Logs

Additional Notes

[srpatcha]

Review — ebuild#125 "fix(ebuild): preserve toolchain linker flags for shared libraries"

head: 75d9249 author: SJ-14-SJ ci: none reported

Verdict: Clean. A one-line fix that makes the shared_library branch symmetric with the executable/test branch two blocks above it, a test that would have caught the original defect, and a CHANGELOG entry. I reproduced the generated manifest locally and it matches the test's expectations exactly, with no mutation of the toolchain or target flag lists. No new lint errors. The only things I would change are presentational.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Low	PR body	The body is the unmodified repository template: every Type-of-Change, Testing and Pre-Submission box unchecked, "Changes" left as two empty bullets, no linked issue. The actual verification record — which is thorough and unusually honest, including two FAIL (pre-existing) lines and two NOT RUN lines — is buried in TASKS.md:51-77 where a GitHub reviewer will not look. master at 52e1f94 has just tightened the linked-issue policy.	Copy the Verification results block from TASKS.md into the PR body's Testing section and tick fix + test. The content already exists; it is in the wrong file for the audience.
2	Low	TASKS.md:12-16, :23-77	The T-119 entry is inserted above the Active table, and no | T-119 | ... | row is added to that table — so the task does not appear in the ledger the file's header describes ("each owning role updates its own row"). It also uses inline Goal: / Acceptance criteria: rather than the definition-list form and the | Check | Command | Result | verification table that the T-000 template at TASKS.md:65-96 specifies.	Add the | T-119 | Preserve toolchain linker settings for shared libraries | backend | Verification | review | none | row to the Active table, and move the prose block below it in template form. The verification prose converts to the template's table almost line for line.
3	Low	tests/unit/test_shared_library_toolchain.py:157-160	The compiler-level regression — the test that actually proves -Wl,--no-undefined reaches the linker — is skipped on the macOS and Windows legs of the matrix. That is correct and the reason= string says so plainly, so this is not a hidden gap; noting it only so the coverage is not read as nine legs deep. On the three ubuntu-22.04 legs it does run: ninja>=1.11 is a hard dependency in pyproject.toml:16, so pytest.importorskip("ninja") will not skip after pip install -e ., and the runner has gcc.	None needed. Leave as is.

Architecture conformance

Conforms. §21 places ebuild in Tier 1 — Foundation; ebuild/build/ninja_backend.py is build-graph generation, which is squarely eBuild's job. §5.1 holds in both directions: the diff adds no import, no link line and no manifest entry, and nothing here becomes a runtime dependency — eBuild understands the complete graph but is not a runtime dependency.

The change is also the right kind of fix under §9.2, "One source of truth for CLI, VS Code and EoStudio": rather than re-deriving toolchain flags for the shared-library case, it reuses the already-computed toolchain_ldflags from :219, so executables, tests and shared libraries now all read the same value from _get_toolchain_ldflags() at :137. One source, three consumers. Per the brief's duplication check, this is the opposite of duplication — it removes an inconsistency rather than adding a second implementation.

Ordering is correct. link_shared's command template at :214 is $cc -shared $ldflags $in -o $out $libs, so toolchain -L paths and --sysroot precede the object files, and package -l entries land last in $libs where the linker needs them. Putting toolchain_ldflags ahead of target.ldflags matches :236 exactly, which means a target can still override a toolchain flag by repeating it — the same precedence executables already had.

No API or wire-format change (brief item 8): _write_ninja is private, no signature moves, and the emitted build.ninja gains flags rather than changing shape. No hot path is touched (brief item 9) — this runs once per configure, over self.config.targets, with no new loop.

Proposed changes

The code needs no change. Two housekeeping items, in order:

1. Fill in the PR body from the TASKS.md verification block, per finding 1.
2. Add the missing T-119 row to the Active table and reshape the block to the T-000 template, per finding 2.

One observation for the maintainer rather than the author: this PR's TASKS.md block independently reports the same two pre-existing failures that #122 and #124 are each addressing — nine tests/unit/test_index_sync.py failures from the missing PackageRecipe.to_dict, and four repository-wide ruff findings. Three separate contributors have now diagnosed the same broken gate from scratch. That is the cost of leaving master red; landing #122 (or #132) and one of #119/#124 would stop it recurring.

Not checked

• pytest — NOT RUN. pytest is not importable on this host, and the local ebuild clone has a dirty working tree, left untouched per the rules of engagement. Neither of the two new tests was executed by me. The author's claim that all three cases fail against the original backend and pass with the fix is unverified as a pytest run; I verified the underlying manifest assertion by other means, below.
• The GCC link test — NOT RUN. test_cli_shared_library_honors_toolchain_no_undefined was not executed. The claim that an unresolved symbol now fails the build is unverified.
• mypy — NOT RUN. Not installed. yamllint — NOT RUN. Not installed; no YAML in the diff.
• CMake/CTest — NOT RUN, matching the author's own NOT RUN line for the same reason.
• CI — no checks reported on fix/shared-library-toolchain-ldflags. GitHub returns no check runs for this branch at all, so nothing here has CI evidence behind it. mergeable and mergeStateStatus are both UNKNOWN.
• I did not test cross-compilation with a real sysroot; the --sysroot= emission is verified only as a string in the generated manifest.

Verified locally: against a git archive export of head 75d9249c — (a) ruff 0.16.5 check . reports 4 errors, all of them the pre-existing origin/master findings, so this PR adds none; (b) driving NinjaBackend(...).generate() directly with ResolvedToolchain(ldflags=["-Wl,--no-undefined"], sysroot="/opt/target-sysroot") and two shared_library targets emits ldflags = -Wl,--no-undefined --sysroot=/opt/target-sysroot -Wl,-soname,libfirst.so -Lvendor/first and the matching line for second, and afterwards toolchain.ldflags and target.ldflags are both unchanged — the flag-leak acceptance criterion holds.

---

Automated architecture review of 75d9249cebf6 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[srpatcha]

Thanks — this is clearly right: ninja_backend.py:283 now mirrors the executable branch at :236, and the -Wl,--no-undefined CLI test is a nice way to prove the flags reach the linker rather than only the manifest. I ran both new tests locally with ninja installed and they pass; the rest of the suite is unchanged, ruff and mypy are clean.

One request before merge:

• TASKS.md:12-66 — the 56-line T-119 block sits ahead of the "Active" table and largely repeats the PR description (handoffs, local PASS/FAIL log). It will be stale the moment this merges. Could you reduce it to a table row in the existing format (or drop it, since the PR itself records the verification)? "T-119" also reads as a PR number; the next free task ID is T-006/T-007.
  Optional: once #131 (automatic -fPIC for shared libraries) lands, the explicit cflags: [-fPIC] in tests/unit/test_shared_library_toolchain.py:72 can be removed. Expect a trivial CHANGELOG.md conflict with #131/#126.

Approving with that nit.

[srpatcha]

Review — ebuild#125 "fix(ebuild): preserve toolchain linker flags for shared libraries"

head: ed3a2ee author: SJ-14-SJ ci: fail

Verdict: Follow-up review. The code and tests are byte-identical to the head I reviewed at 75d9249c — only TASKS.md and the PR body moved. Prior finding 2 is resolved; prior finding 1 is only half done, and the half that was skipped is now a red required check. One new finding: the verification record was deleted rather than relocated, so a behaviour-changing linker fix now carries no evidence anywhere.

Status of the previous review's findings

Prior #	Status	Evidence
1 — PR body is the unmodified template	Partially addressed	A two-line summary was prepended to pr.body. Everything else is unchanged: all nine Type-of-Change boxes, all four Testing boxes and all six Pre-Submission boxes are still - [ ]; "Changes" is still - / - ; "Related Issues" is still empty. The recommended action — copy the verification block into Testing — did not happen; see finding 2 below for what happened to it instead.
2 — T-119 not in the Active table, wrong format	Resolved in ed3a2eed	TASKS.md now adds exactly one line: | T-006 | Preserve toolchain linker settings for shared libraries ([#125](…)) | backend | Verification | review | none |, in the Active table, in the documented column order. The out-of-template prose block is gone. T-006 does not collide with T-002–T-005 in origin/master.
3 — GCC link test skipped off Linux	Untouched, and correctly so	tests/unit/test_shared_library_toolchain.py hashes identical to the prior head (b3be1864…). The finding said no change was needed; none was made.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	CI — policy / Policy / Linked Issue	The required policy check fails (run 34816962349, 4s). mergeStateStatus is BLOCKED and reviewDecision is REVIEW_REQUIRED; mergeable is MERGEABLE, so this is the gate, not a conflict. The cause is visible in the PR body: the ## Related Issues section is empty, which is the one part of prior finding 1 that was not addressed. This is the single thing standing between this PR and mergeability.	Open (or find) an issue for the shared-library toolchain-flag defect and put Fixes #NN under ## Related Issues. Nothing in the tree needs to change.
2	Medium	TASKS.md (removed vs 75d9249c)	The verification record was deleted, not moved. At 75d9249c, TASKS.md carried a 30-line Verification results (Linux, Python 3.12) block — three PASS lines with commands, two FAIL (pre-existing) lines naming the nine test_index_sync.py failures and the four repo-wide ruff findings, and two NOT RUN lines for CMake/CTest and for Windows/macOS linking. That block is gone at ed3a2eed and nothing replaced it: not the PR body (all Testing boxes still unchecked), not the Evidence section, not the Done table. The T-006 row says Mode Verification, Status review, and the ledger now holds no verification for it — which is exactly what TASKS.md's own header says each owning role is responsible for. Per .ai/reviewer.md ("was anything deleted or overwritten without explanation?") and master design §28, the evidence for a behaviour-changing linker fix should not vanish in a formatting cleanup.	Restore the block, in the T-000 template's | Check | Command | Result | form, under the Evidence heading where T-002–T-004 already keep theirs — or paste it into the PR body's Testing section, which is what prior finding 1 asked for. The text still exists verbatim at git show 75d9249c:TASKS.md, lines 39-66.

No new defect in the new commits: there is no new code in them to defect. The one-line backend change and the 88-line test file are unchanged.

Architecture conformance

Conforms; unchanged from the prior review and re-checked against the mirror rather than recalled.

§21 places ebuild in Tier 1 — Foundation, and ebuild/build/ninja_backend.py is build-graph generation, which is eBuild's own job. §5.1 holds in both directions: the diff adds no import, no link line and no manifest entry, and nothing here becomes a runtime dependency — "eBuild understands the complete graph but is not a runtime dependency". §9.2's "one source of truth" is what the fix actually restores: toolchain_ldflags from _get_toolchain_ldflags() now feeds executables, tests and shared libraries, instead of the shared-library branch re-deriving its own flag list. Per brief item 10 this removes duplication rather than adding it.

Brief item 8 (API/wire compatibility): none. _write_ninja is private, no signature changes, and build.ninja gains flags without changing shape. Brief item 9 (performance): none — this runs once per configure over self.config.targets, with no new loop or allocation. Brief item 4 (weakened checks): none; no test disabled, no lint loosened, no assertion removed.

Proposed changes

1. Add Fixes #NN to ## Related Issues. That clears the only red check and unblocks the PR.
2. While editing the body, tick fix and test under Type of Change, and fill the two empty Changes bullets — one line each for the backend change and the regression test.
3. Restore the deleted verification block under TASKS.md's Evidence heading, in the T-000 table form.

None of these touch the code. The code is finished.

Not checked

• pytest — NOT RUN. pytest is not importable on this host (ModuleNotFoundError: No module named 'pytest'), so neither new test was executed by me, at this head or any other. The author's claim that all three cases fail pre-fix and pass post-fix remains unverified as a pytest run — and as of this head that claim is no longer written down anywhere in the repo.
• The GCC link regression — NOT RUN. test_cli_shared_library_honors_toolchain_no_undefined was not executed. That an unresolved symbol now fails the link is unverified here.
• mypy — NOT RUN (not installed). yamllint — NOT RUN (not installed; no YAML in the diff). CMake/CTest — NOT RUN.
• The commits between 75d9249c and ed3a2eed — NOT INSPECTED as commits. The local clone does not have object ed3a2eed (git cat-file: could not get object info), and the ebuild working tree is dirty (TASKS.md, ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py modified), so per the rules of engagement I did not fetch into it. I compared end states instead, below; commit messages and any intermediate commit are unreviewed.
• Cross-compilation with a real sysroot is still untested; --sysroot= is verified only as a string in the generated manifest.

Verified locally, by exporting origin/master with git archive and applying the bundle's diff.patch to it:

• git apply --check then git apply — applies cleanly, confirming no conflict with master, consistent with mergeable: MERGEABLE.
• sha256sum of the resulting ebuild/build/ninja_backend.py (dd109022…) and tests/unit/test_shared_library_toolchain.py (b3be1864…) are identical to the same two files exported from 75d9249c. This is the evidence for "code and tests unchanged since the last review", and it means the prior review's verification of the generated manifest still applies to this head.
• ruff 0.16.5 check ebuild/build/ninja_backend.py tests/unit/test_shared_library_toolchain.py — All checks passed, exit 0.
• ruff 0.16.5 check . on the patched tree — 4 errors; the identical run on an unpatched origin/master export — 4 errors. The PR introduces none; the four are the known pre-existing F811/W292/E402 findings in unchanged test files.

---

Automated architecture review of ed3a2eed4ff2 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.