test: skip GCC-dependent test when compiler is unavailable

[chandupatel-ai]

Summary

Fixes test collection failures on environments where GCC is not installed.

The end-to-end build test previously invoked gcc --version directly while evaluating its pytest.mark.skipif condition. When GCC was unavailable, subprocess.run() raised FileNotFoundError, causing test collection to fail instead of skipping the compiler-dependent test.

This change uses shutil.which("gcc") to detect whether GCC is available before running the test.

Testing

Targeted test:

python -m pytest tests/ebuild/test_build_dir_resolution.py -vv
7 passed, 1 skipped

[srpatcha]

Review — ebuild#114 "test: skip GCC-dependent test when compiler is unavailable"

head: 6c6166a author: chandupatel-ai ci: none reported

Verdict: Correct one-line change that matches the convention already used elsewhere in
this suite, but it does not reach the stated objective — the suite still fails without gcc,
by the author's own numbers — and no CI run exists to confirm anything.

The substitution itself is right. subprocess.run(["gcc", ...]) raises FileNotFoundError
when gcc is absent, and because the call sits in the skipif argument it is evaluated at
import time, so the exception surfaces as a collection error rather than a skip.
shutil.which("gcc") is None is the pattern tests/ebuild/test_ninja_backend.py:121
already uses (shutil.which("cc") is None and shutil.which("gcc") is None), so this makes
the two consistent rather than introducing a new idiom.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	tests/unit/test_footprint.py:66 (master)	TestMeasure::test_measures_a_real_binary runs subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) with no guard of any kind. The PR body records this as the one remaining failure ("553 passed, 7 skipped, 1 failed") and leaves it. The PR's stated goal is that the suite behaves correctly where gcc is not installed; after this change it still does not. Verified against origin/master; no other open ebuild PR (#103–#113) touches this file.	Add the same guard to the test: @pytest.mark.skipif(shutil.which("gcc") is None, reason="needs gcc to produce a binary to measure"), importing shutil in that module. One skipif, and the suite is green without a compiler.
2	Low	tests/ebuild/test_build_dir_resolution.py:246–248	The predicate and the reason no longer agree. reason="needs a working gcc to link the executable" but shutil.which only proves a file named gcc is on PATH. A gcc that is present and broken previously skipped (non-zero --version); now it runs the test and fails. Running is arguably the better behaviour — a broken toolchain should be loud — but the reason string states a check that is no longer performed.	Reword to reason="needs gcc on PATH to link the executable", so the text describes the predicate that is actually evaluated.
3	Low	—	No independent evidence. gh pr checks 114 --repo embeddedos-org/ebuild returns "no checks reported on the 'fix/gcc-test-skip' branch", although .github/workflows/ci.yml:9 declares pull_request: branches: [master, main] and this PR targets master. mergeStateStatus is BLOCKED. The only evidence for the change is the pass/skip counts typed into the PR body — no command output, no CI run.	Most likely first-time-contributor workflow approval is pending; a maintainer approving the run is enough. If CI stays absent, that is a repo-level defect worth its own issue, not this author's.

Architecture conformance

ebuild is Tier 1 — Foundation (§21). The change is confined to tests/, adds one
stdlib import, and introduces no #include, import, link line or manifest entry, so §5.1
is not engaged in either direction. §9's rule that eBuild is the developer control plane
and not a runtime dependency is untouched. Conforms.

Worth noting for the record: CI runs ubuntu-22.04, macos-latest and windows-2022
(ci.yml:30), and gcc resolves on all three, so this defect never reddened CI — it only
broke local runs on machines without a compiler. That is still worth fixing; §25.2 puts
"one-command environment diagnosis" and a working local loop on the MLP path, and a suite
that cannot be collected on a fresh machine is squarely in that column.

Proposed changes

1. In tests/unit/test_footprint.py, add import shutil and decorate
   test_measures_a_real_binary with
   @pytest.mark.skipif(shutil.which("gcc") is None, reason="needs gcc on PATH to build the binary under test").
   That closes finding 1 and makes the PR's claim true.
2. Reword the reason in the hunk under review per finding 2.
3. Re-run python -m pytest tests/ -q on a machine with no gcc on PATH and paste the
   summary line into the PR body, replacing the current prose count.

Not checked

• The change was not executed. The local ebuild clone is on branch v90 with a dirty
  tree (4 files) and was skipped by the sync step; head 6c6166a8 is not fetched locally,
  so no worktree at this PR's tree exists to run pytest in. Findings above come from
  reading the diff against origin/master, not from a run.
• Windows and macOS behaviour of shutil.which("gcc") was not exercised, only reasoned
  about from the stdlib contract (PATHEXT handling on Windows).
• The claim "553 passed, 7 skipped, 1 failed" was not reproduced; finding 1 was verified
  by reading origin/master:tests/unit/test_footprint.py, which independently supports it.

---

Automated architecture review of 6c6166a8d765 — 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.

[chandupatel-ai]

Summary

Addressed the requested review changes.

• Added a GCC availability check to tests/unit/test_footprint.py so the GCC-dependent test is skipped when GCC is not installed.
• Updated the skip reason in tests/ebuild/test_build_dir_resolution.py to accurately state that GCC must be on PATH.
• No production code was changed.

Testing

Full test suite on Windows without GCC:

python -m pytest tests/ -q
553 passed, 8 skipped in 15.34s

[srpatcha]

Review — ebuild#114 "test: skip GCC-dependent test when compiler is unavailable"

head: cc47987 author: chandupatel-ai ci: none reported (bundle checks.txt is empty; no workflow has run on this head)

Verdict: Correct, minimal, and it fixes more than the title claims — I reproduced both halves rather than taking the numbers on trust. On the base commit e5d8052 with gcc off PATH, pytest tests/ does not merely fail one test: tests/ebuild/test_build_dir_resolution.py:246 evaluates subprocess.run(["gcc", ...]) at import time, so collection raises FileNotFoundError and pytest exits with Interrupted: 1 error during collection — all 561 tests never run. On this head the same command gives 556 passed, 5 skipped, 0 failed. That is 556 tests recovered on every runner without a host compiler, i.e. the three windows-2022 jobs in ci.yml's matrix. shutil.which is also the convention already used at tests/ebuild/test_ninja_backend.py:132 and tests/ebuild/test_integration_initramfs_security.py:38, so this aligns the outlier rather than inventing a pattern. Two small gaps below; neither blocks the fix.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Low	tests/unit/test_footprint.py:60-63	The new guard covers gcc but not the other external tool this test needs. measure() calls find_size_tool(), so on a host with a compiler and no size the test still hard-fails with the same class of environment error the PR exists to remove. Executed on this head with gcc present and size masked off PATH: FAILED TestMeasure::test_measures_a_real_binary — FootprintError: no 'size' tool on PATH, so the footprint cannot be measured (install binutils, ...). This is not hypothetical: the author of ebuild#109 has reported both failure modes on their own host across two rounds ("test_footprint needs an external size binary", later "needs system gcc"), so the half you guarded and the half you did not have both been hit in this org.	One predicate, using a symbol the file already imports at line 30: shutil.which("gcc") is None or find_size_tool() is None. Check find_size_tool's contract first — it may raise rather than return None, in which case wrap it or reuse the module's own probe. Fold the reason string into one sentence naming both tools so the skip says which one was missing.
2	Low	tests/unit/test_footprint.py:60	Recorded because the brief treats a newly-skipped test as a finding regardless of the reason given: TestMeasure::test_measures_a_real_binary was unconditional and is now conditional. Measured, so the cost is stated rather than assumed — base at e5d8052, tests/unit only, gcc masked: 1 failed, 345 passed, the failure being this test; head, same conditions: it skips. So the change converts a hard failure into a skip on hosts without a compiler and alters nothing on hosts with one. Against ci.yml's matrix (ubuntu-22.04, macos-latest, windows-2022 × py3.10/3.11/3.12) the assertion still executes on the six jobs that ship a compiler and is skipped on the three windows-2022 jobs, where it previously could only fail. No measured coverage is lost; what is lost is any signal if the skip ever starts firing on Linux too, because nothing asserts an expected skip set.	No change requested — the guard is the right call for a host-toolchain dependency and matches the repo's convention. If you want the residual risk closed, the cheap version is an assertion in the ubuntu-22.04 CI job that this test ran rather than skipped; that is a CI change, not a test change, and belongs in its own PR.

Architecture conformance

Conforms; no architectural surface is touched. Both files are under tests/, no production module changes (the PR body's "No production code was changed" is accurate — files.txt lists exactly two test files, 3+ 2- and 5+ 1-). No #include, import, link line or manifest entry is added, so §5.1's dependency direction is unaffected, and the only new imports are stdlib shutil in two test modules. §21 tier placement is correct: ebuild is Tier 1 - Foundation and this is Tier-1 test hygiene in the Tier-1 repo. Relevant to the design only in that it serves §17's CI contract and §25.2's "one-command environment diagnosis" — a suite that aborts at collection on a supported developer host is the opposite of both.

Proposed changes

1. Add the size predicate to the test_footprint.py guard (Finding 1). One line.
2. Optional tidy in the file you are already editing: the diff removes the second blank line before class TestMeasure:, leaving one where PEP 8 asks for two. CI does not care — verified ruff check --select=E,F,W --ignore=E501 (the exact selection ci.yml:52 uses) passes on both changed files, because ruff's E set does not enable the blank-line rules E301–E306 outside preview mode. Restoring the line costs nothing and keeps the diff to its subject.

Nothing else. Do not widen this PR.

Not checked

• CI: nothing ran. checks.txt is empty for this head and pr.json carries no check rollup, so none of the repo's pull_request-triggered workflows have produced a result on cc479879. The claim that matters here is inherently platform-specific and cannot be confirmed from CI output that does not exist.
• Windows was not tested. Your 553 passed, 8 skipped is from a Windows host without GCC and I have no Windows runner. I reproduced the equivalent scenario on Linux by masking gcc off PATH and got 556 passed, 5 skipped — three fewer skips and three more passes, consistent with the Windows-only skips in the tree (e.g. test_integration_initramfs_security.py:190, os.name == "nt"), so your figure is plausible but is neither confirmed nor disputed. Your targeted figure I did reproduce exactly: pytest tests/ebuild/test_build_dir_resolution.py with gcc absent → 7 passed, 1 skipped.
• What I ran, so it is not mistaken for CI: full tests/ on Linux with gcc+ninja present → 561 passed, 0 failed, 0 skipped; full tests/ with gcc masked → 556 passed, 5 skipped; base e5d8052 with gcc masked → collection error, 0 tests run; base tests/unit with gcc masked → 1 failed, 345 passed; tests/unit/test_footprint.py with size masked → 1 failed, 42 passed; ruff with ci.yml's selection on both changed files → clean. Scratch checkout of the head tree, not the working-root clone, which is dirty and was left untouched by the sync step.
• Not assessed: whether macos-latest provides gcc on PATH. Apple's Command Line Tools install a gcc shim over clang, so the guard most likely does not skip there, but I did not verify it and the PR's coverage claim on macOS therefore rests on an assumption, not a measurement.
• Not assessed: the other two subprocess-using test modules for the same import-time pattern. I grepped every skipif in tests/ and the remaining three predicates are shutil.which or os.name checks with no subprocess call, so test_build_dir_resolution.py:246 was the only instance — but that is a grep over skipif occurrences, not a proof that no other module executes an external tool at import time.

---

Automated architecture review of cc479879c140 — 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.

[chandupatel-ai]

Thanks for the review. I added the size tool availability guard as suggested and kept the change scoped to this PR. I also verified the relevant tests locally: 49 passed, 2 skipped.

[srpatcha]

Review — ebuild#114 "test: skip GCC-dependent test when compiler is unavailable"

head: 8330640 author: chandupatel-ai ci: none reported (checks.txt empty; statusCheckRollup: [] — third consecutive head with no workflow run)

Verdict: Follow-up review. The one open finding from the previous round is fixed and I
reproduced the fix rather than taking the comment on trust — size masked off PATH went
from 1 failed at cc479879 to 1 skipped at this head. The find_size_tool() contract
caveat I raised last time is answered: it returns Optional[str] and cannot raise, so the
new predicate is safe to evaluate at import. Two style nits remain in the file, one of them
newly introduced by 8330640. Neither blocks. The substantive issue now is outside the
diff: #115 is a duplicate of this PR and one of the two has to go.

Status of the previous review's findings (ebuild-114-cc479879.md)

Prev #	Status	Evidence
1 — guard covers gcc but not size	Resolved in 8330640	tests/unit/test_footprint.py:61 now reads shutil.which("gcc") is None or find_size_tool() is None. Executed at this head with size masked off PATH: 42 passed, 1 skipped, skip reason needs gcc and a size tool. Same command at cc479879: 1 failed, 42 passed — FootprintError: no 'size' tool on PATH.
2 — newly-conditional test, no change requested	No action needed, as stated last round	Still measured: the test executes on hosts with both tools, skips otherwise. Full suite at this head with both present: 561 passed.
Proposed change 2 — restore the blank line before class TestMeasure:	Untouched	tests/unit/test_footprint.py:58-59 still has one blank line where the file's own convention and PEP 8 use two. See finding 2.

find_size_tool() verified at ebuild/build/footprint.py:100-115: signature
-> Optional[str], every branch returns a value or None, terminal call is
shutil.which("size"). No raise path, so the import-time evaluation the PR exists to fix
is not reintroduced. Confirmed empirically — no collection error in any of the four PATH
scenarios below.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	— (repo-level)	#115 "Fix compiler-dependent tests on systems without GCC" (anandrishavvvv, head b20e546e) changes the same two files for the same reason. This PR was opened first (05:43Z vs 07:56Z) and is strictly the better of the two: #115 misindents the predicate to 3 spaces (shutil.which("gcc") is None), leaves the stale reason="needs a working gcc to link the executable" that this PR corrects, inserts import shutil after import subprocess out of alphabetical order, uses an in-body pytest.skip() rather than a marker so the skip is invisible to --collect-only, and has no size guard at all. Whichever merges first leaves the other with a conflict in both files. Not a defect in your diff — flagged because neither PR mentions the other and nothing else will surface it.	Maintainer call, not the authors': keep #114, close #115 as superseded. Nothing to change here.
2	Low	tests/unit/test_footprint.py:65	New in 8330640: a blank line sits between the @pytest.mark.skipif(...) block and def test_measures_a_real_binary. Functionally harmless — I confirmed the decorator still binds (the skip fires correctly in scenarios B, C and D below) — and invisible to CI, because ruff check . --select=E,F,W --ignore=E501 (ci.yml:52) passes on both changed files; blank-line-after-decorator is preview-only in this ruff version. Recording it because it is a regression the fix commit introduced, and because a decorator visually detached from its function reads like a bug to the next person in this file.	Delete line 65.
3	Low	tests/unit/test_footprint.py:58-59	Carried over unresolved from the previous review. The diff removes one of the two blank lines before class TestMeasure:, leaving one. ruff check --select=E3 --preview reports tests/unit/test_footprint.py:59:1 blank-lines-top-level: Expected 2 blank lines, found 1. Also not caught by CI's selection.	Restore the blank line. Together with finding 2 this keeps the diff to its subject.

Both style items are [*] fixable; ruff check tests/unit/test_footprint.py --select=E3 --preview --fix clears them and touches nothing else.

Architecture conformance

Conforms. ebuild is Tier 1 — Foundation (§21) and this is Tier-1 test hygiene in the
Tier-1 repo, so placement is correct. §5.1's dependency law is not engaged in either
direction: files.txt is two files under tests/, the only additions are stdlib shutil
and a call to find_size_tool, which the module already imports from
ebuild.build.footprint at line 30 — that is a test reaching into its own repo's package,
not a tier crossing. No #include, link line, target_link_libraries entry or manifest
dependency is added, and §9's rule that eBuild is never a runtime dependency is untouched.
No public signature, struct layout, manifest field or serialized format changes, so brief
item 8 does not apply. The PR body's "No production code was changed" is accurate.

On brief item 4 (weakened checks): a previously unconditional assertion is now conditional,
which I am obliged to record and did last round. Measured again here rather than assumed —
the assertion still executes wherever both tools exist, and where they do not it replaces a
hard failure or a collection abort, so no coverage that was ever running is lost.

Proposed changes

1. ruff check tests/unit/test_footprint.py --select=E3 --preview --fix — closes findings
   2 and 3, two lines, no behaviour change.
2. Post the actual command output in the PR body. The current body reports
   7 passed, 1 skipped from the first head and the two follow-up comments give bare
   counts (553 passed, 8 skipped, 49 passed, 2 skipped) with no command shown. Per
   brief item 5 that is an unsupported claim; the numbers happen to be consistent with what
   I measured, but the body does not evidence them.
3. Nothing else. Do not widen this PR.

Verification performed

Scratch clone of head 83306404, Python 3.12.14, pytest 9.1.1, ruff, mypy 2.3.1.
Tools masked by rebuilding PATH from a directory of symlinks with the named binaries
removed. The working-root clone is on branch v90 with a dirty tree and was left untouched.

#	Scenario	Result
A	pytest tests/ -q, gcc+size present	561 passed
B	pytest tests/ -q, gcc masked	558 passed, 3 skipped
C	pytest tests/ -q, size masked	560 passed, 1 skipped
D	pytest tests/ -q, both masked	558 passed, 3 skipped
E	pytest tests/unit/test_footprint.py, size masked, at cc479879	1 failed, 42 passed — the regression this head fixes
F	ruff check . --select=E,F,W --ignore=E501 on the two changed files (ci.yml:52)	clean
G	ruff check --select=E3 --preview	2 errors, findings 2 and 3
H	mypy --ignore-missing-imports --no-strict-optional on the two changed files	0 errors in the changed files (8 pre-existing errors in ebuild/cli/integration.py and ebuild/eos_ai/__init__.py, unrelated to this PR; CI's mypy step is continue-on-error)

No collection error in A–D. That is the property the PR exists to establish, and it holds.

Blocked / stale

mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED.
gh pr checks 114 still returns "no checks reported on the 'fix/gcc-test-skip' branch"
after three heads, although ci.yml:9 declares pull_request: branches: [master, main]
and this PR targets master. The blocker is human, not technical: a maintainer approving
the workflow run for a first-time contributor, then a review. Nothing the author can do.

Not checked

• CI: nothing ran, on any of the three heads. Every result above is mine, from a Linux
  scratch checkout. None of it is CI output and none of it should be read as CI passing.
• Windows not tested. Your 553 passed, 8 skipped and 49 passed, 2 skipped are from a
  Windows host; I have no Windows runner. My Linux equivalents differ in skip count in the
  direction the Windows-only skips in the tree predict (e.g.
  test_integration_initramfs_security.py:190, os.name == "nt"), so the figures are
  plausible — neither confirmed nor disputed.
• macOS not assessed. Whether macos-latest puts gcc and size on PATH is still
  unverified; Apple's Command Line Tools ship a gcc shim over clang and a size, so the
  guard most likely does not fire there, but that is reasoning from the platform, not a
  measurement.
• Cross-toolchain path not exercised. The guard calls find_size_tool() with no
  argument, so only the host branch (shutil.which("size")) is covered. The
  toolchain_prefix branch at footprint.py:108-114 is untouched by this PR and I did not
  run it.
• #115's own test run not reproduced. I read its diff and compared it to this one; I did
  not check out b20e546e or execute it.
• ruff version sensitivity. Findings 2 and 3 are preview-only in the ruff I ran. If
  #113 ("move ruff config to pyproject.toml") lands and enables a wider selection, they may
  become CI-visible. I did not test that interaction.

---

Automated architecture review of 833064046b28 — 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.

[chandupatel-ai]

I’ve updated the test to skip the GCC-dependent test when the compiler is unavailable. Could you please review the changes and approve the PR if everything looks good? Thanks!