Skip to content

Fix compiler-dependent tests on systems without GCC - #115

Open
anandrishavvvv wants to merge 1 commit into
embeddedos-org:masterfrom
anandrishavvvv:assessment-fix
Open

Fix compiler-dependent tests on systems without GCC#115
anandrishavvvv wants to merge 1 commit into
embeddedos-org:masterfrom
anandrishavvvv:assessment-fix

Conversation

@anandrishavvvv

Copy link
Copy Markdown

Summary

Fix compiler-dependent tests so they skip cleanly when GCC is unavailable instead of raising FileNotFoundError.

Type of Change

  • fix — Bug fix
  • test — Add or fix tests

Problem

The affected tests invoked gcc directly. On systems where GCC is not installed, this caused FileNotFoundError and made the test suite fail.

Solution

Check whether GCC is available using shutil.which("gcc"). If it is unavailable, the compiler-dependent test is skipped with a clear reason.

Testing

Full test suite:

555 passed, 6 skipped, 0 failed

The GCC-dependent tests now skip cleanly on the current Windows environment.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#115 "Fix compiler-dependent tests on systems without GCC"

head: b20e546 author: anandrishavvvv ci: none (no checks recorded on this head)

Verdict: The underlying bug is real and this fixes it — with gcc absent the old guard raised FileNotFoundError at collection, and on this head both tests skip cleanly. But open PR #114 already makes the same change to the same two tests with the same mechanism, and the shutil.which("gcc") guard is shallow enough that the two tests still fail hard on a host that has the gcc driver but not binutils.

Findings

# Severity File:line Finding Recommended fix
1 Medium (whole PR) Duplicate of open PR #114 ("test: skip GCC-dependent test when compiler is unavailable", chandupatel-ai). #114 touches the same two files, the same two tests, and uses the same shutil.which("gcc") is None condition. It was opened first, and its version is closer to the file conventions: 4-space continuation indent, import shutil in alphabetical position, and a declarative @pytest.mark.skipif on both tests rather than a runtime skip on one. Two PRs racing on the same four lines will collide. Decide between this and #114 before either merges. If #115 is the one kept, fold in finding 2 (which neither PR addresses) so the extra round-trip buys something; otherwise close this in favour of #114.
2 Medium tests/unit/test_footprint.py:62, tests/ebuild/test_build_dir_resolution.py:247 shutil.which("gcc") is not None proves only that a gcc driver is on PATH, not that it can compile and link. Both tests still fail hard when the driver is present and binutils are not. Verified: with PATH containing only gcc, TestMeasure::test_measures_a_real_binary fails with subprocess.CalledProcessError / gcc: fatal error: cannot execute 'as', and test_end_to_end_build_from_outside_produces_the_binary fails the same way — 2 failed, 49 passed. measure() additionally shells out to size (ebuild/build/footprint.py, FootprintError("no 'size' tool on PATH…")), which the guard never checks; the same test file's test_a_missing_size_tool_raises shows the project already treats a missing size as a real environment. This is the same failure class the PR sets out to fix, one layer down. Replace the PATH probe with a probe of the capability. A session-scoped fixture is the durable version: compile a one-line .c to an executable once in tmp_path_factory, cache the result, and pytest.skip on any OSError/CalledProcessError. If you want the minimal change instead: shutil.which("gcc") is None or shutil.which("size") is None in test_footprint.py, and shutil.which("gcc") is None or shutil.which("as") is None in test_build_dir_resolution.py.
3 Low PR body, "Testing" "555 passed, 6 skipped, 0 failed" is given with no command and no output, and "the current Windows environment" is not named. .ai/reviewer.md asks every PASS to point at command output; a bare count is not that. Paste the pytest invocation and its tail, and state the OS/Python version the run came from.
4 Low tests/ebuild/test_build_dir_resolution.py:247 The continuation line is indented 3 spaces where the file (and PR #114) uses 4. Confirmed it does not fail CI: ruff check --select=E,F,W --ignore=E501 (the exact command in ci.yml:52) passes on both changed files, ruff does not implement the E12x continuation rules outside preview, and weekly.yml's flake8 lints only ebuild/, not tests/. So this is style, not a broken check — but it is a one-character fix. shutil.which("gcc") is None,
5 Low tests/ebuild/test_build_dir_resolution.py:30 import shutil is inserted after import subprocess, breaking the alphabetical order the file already keeps (os, subprocess, textwrap). The same import in test_footprint.py:18 is placed correctly. Move it above import subprocess.
6 Low tests/unit/test_footprint.py:62-63 The guard is a runtime pytest.skip inside the test body, while the other half of this PR — and test_ninja_backend.py:133, test_integration_initramfs_security.py:39 — use @pytest.mark.skipif. A runtime skip is invisible to --collect-only and to marker-based selection, so a CI job that reports "which tests will run" reports it as a test that will run. Use @pytest.mark.skipif(shutil.which("gcc") is None, reason=...) on the method, as #114 does, for one convention across the file.

Architecture conformance

No architectural surface. Master design §21 places ebuild in Tier 1 — Foundation; this diff touches only tests/, adds no import, link or manifest edge, and moves nothing between tiers. §5.1 is not engaged. Nothing here weakens a check in the sense of .ai/reviewer.md — converting a hard environmental failure into a declared skip is the correct direction, and neither test's assertions were touched.

Proposed changes

  1. Resolve the overlap with #114 first — nothing below is worth doing twice.

  2. In whichever PR survives, replace the PATH probe with a capability probe. In tests/conftest.py:

    @pytest.fixture(scope="session")
    def host_cc_works(tmp_path_factory):
        """True only if the host can actually compile *and* link, not merely
        resolve `gcc` on PATH — a driver without binutils fails at `as`."""
        if shutil.which("gcc") is None:
            return False
        d = tmp_path_factory.mktemp("cc_probe")
        (d / "p.c").write_text("int main(void){return 0;}\n")
        try:
            subprocess.run(["gcc", str(d / "p.c"), "-o", str(d / "p")],
                           check=True, capture_output=True)
        except (OSError, subprocess.CalledProcessError):
            return False
        return True

    then in each test if not host_cc_works: pytest.skip("needs a host compiler that can link"), plus the shutil.which("size") check in test_footprint.py since measure() needs it independently of gcc.

  3. Fix the indent at :247 and the import order at :30.

Verification I ran (not claims)

Fetched b20e546e into a scratch clone; the local ebuild checkout was left untouched (it is dirty and the sync step skipped it).

  • The bug is real: subprocess.run(["gcc","--version"], capture_output=True) with an empty PATH raises FileNotFoundError: [Errno 2] No such file or directory: 'gcc'. Because that expression sits in a module-level skipif, it fired at import time and took the whole module's collection with it.
  • The fix works: pytest tests/ebuild/test_build_dir_resolution.py tests/unit/test_footprint.py -q with an empty PATH49 passed, 2 skipped, no collection error. With a normal PATH51 passed.
  • The fix is incomplete: same two files, PATH containing only a gcc symlink → 2 failed, 49 passed, both failures on gcc: fatal error: cannot execute 'as'. This is finding 2.
  • ruff check tests/ebuild/test_build_dir_resolution.py tests/unit/test_footprint.py --select=E,F,W --ignore=E501 → All checks passed. import subprocess is still used at :82/:85, so no F401.
  • Read PR #114's diff via gh pr diff 114 and compared it line by line with this one — same two files, same two tests, same condition. This is finding 1.

Not checked

  • No CI has run on this head — checks.txt is empty, no workflow result of any kind. The "555 passed, 6 skipped" figure in the body is unverified by anything I can see, and I could not reproduce it: my environment lacks several optional dependencies, so my totals are lower.
  • I could not test on Windows, which is the environment the PR body describes. The FileNotFoundError reproduction above is from Linux with an emptied PATH; the Windows failure mode (WinError 2) is the same class but I did not observe it.
  • I did not check whether any other test in the tree carries the same module-level subprocess.run([...]) probe pattern. A grep for skipif( combined with subprocess.run/returncode found none remaining after this diff, but a probe written differently would not match that grep.

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

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