macos-amd64: don't hard-fail the checked-in-binary check when tcc.exe isn't the permanent legacy build - #82
Closed
quaesitor-scientiam wants to merge 5 commits into
Conversation
…ncient binary
is_known_checkedin_error's signature ("library 'c' not found") was
reverse-engineered against one specific legacy v0.9.27 build. That
binary isn't actually permanent: vlang/v's update_tccbin.yml can
replace thirdparty/tcc/tcc.exe with a fresh build from an arbitrary
TinyCC commit, recording it in build_source_hash.txt - a file the
ancient binary never had.
Seen in practice: d712d0f rebuilt this branch against TinyCC 85ba3ae8
(0.9.28rc). The direct crash.c probe returned exit 0 with empty output
instead of the expected "library 'c' not found", which the existing
logic treated as an unexpected regression and hard-failed on - even
though nothing regressed, it's just a different build the check wasn't
designed to characterize. Gate the hard-fail on build_source_hash.txt's
absence (i.e. still the pinned legacy binary); when present, report a
mismatch as a warning and let the rebuild-then-test lanes further down
the same job - which fully exercise a fresh build - be the real gate.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46f1c50bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ked mismatch as non-blocking Addresses two Codex findings on vlang#82: P1: the previous version of this fix downgraded ANY checked-in-binary failure to a warning whenever build_source_hash.txt was present, with no positive evidence the binary actually works. That let a genuinely broken bot-rebuilt tcc.exe pass CI silently, since the later rebuild-then-test lanes exercise a freshly-built binary from current mob, not the one this commit actually ships - reintroducing the exact gap the original "verify checked-in tcc.exe" step (vlang#74) was written to close. Now require the checked-in binary to compile, link, AND run a trivial no-GC program to completion (the same bar the "verify no-GC fallback path" step already holds the rebuilt binary to) before a signature mismatch is treated as informational; a binary that fails this sanity check stays a hard failure regardless of provenance. P3: a provenance-tracked binary that fully passes (code 0) logged success but then fell through into the failure-classification case statement anyway, which - since a passing run's output doesn't match "0 passed, 3 failed" - hit the default branch and emitted a contradictory ::warning:: claiming failure. Guard the case statement to skip entirely once a provenance-tracked pass is already accepted. Verified both fixes directly: 6 scenarios (provenance pass, provenance failure+sane, provenance failure+broken, legacy-match, legacy-mismatch, legacy-pass) each produce the intended result, with the 3 legacy-binary cases behaving byte-for-byte as before this change.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f7e175a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ding a checked-in-binary mismatch Addresses two more Codex findings on vlang#82: Round 2, finding 1 (P1): a no-GC-only sanity check isn't applicable enough - a bundled libgc.a that's a well-formed x86_64 archive defining GC_init can still be semantically broken under real GC load, and the no-GC check would never exercise that path at all, letting a genuine distribution regression through as a warning. Add a second, independent check that reuses thirdparty/tccbin_tests/shared/gc_alloc.c directly (100k real GC allocations under load) with the exact same flags/archive run.sh already uses, requiring it to compile, link, AND run to completion. Both checks must pass for a signature mismatch to be downgraded to a warning; either failing stays a hard failure. Round 2, finding 2 (P1): the sanity check only ran when the full suite had already failed, leaving it (and direct_code_checkedin) unused on the passing path. Made both sanity checks run unconditionally whenever build_source_hash.txt is present - harmless extra work when the suite already passed (same binary/libgc.a/flags, so they'd trivially pass too), and it removes the asymmetry Codex flagged. Verified 6 scenarios directly, including the exact failure mode Codex's first finding described (no-GC path fine, GC-allocation test specifically broken) - that case now correctly hard-fails instead of being downgraded. The 2 legacy-binary cases remain unchanged.
…shly rebuilt 29d5da8 tried to close the circularity Codex flagged by re-testing thirdparty/tccbin_tests/shared/gc_alloc.c against the checked-in tcc.exe+libgc.a - but gc_alloc.c is literally one of the 3 tests run.sh already ran as part of the "0 passed, 3 failed" aggregate, so re-running it against the identical pair can never independently pass in the one branch it's meant to gate. That made the whole warning path unreachable and turned every provenance-tracked mismatch into a hard failure again, which is what actually broke CI on this PR just now. Revert that check and restructure properly instead: preserve the checked-in tcc.exe before "build tcc.exe from current tinycc (mob)" overwrites it, defer classification for a provenance-tracked binary that fails without matching the legacy signature (but passes the no-GC check) rather than deciding immediately, and resolve it in a new step right after "rebuild libgc.a from current bdwgc source" - testing the PRESERVED checked-in compiler against a freshly rebuilt libgc.dylib it has never been tested against before is a genuinely independent signal, unlike re-testing against the same artifacts that just failed. Uses dylib+rpath, not the static archive, matching the existing dynamic-blocking lane's own reasoning: tcc's known archive-parsing limitation with a modern-toolchain-built static libgc.a would otherwise fail this probe for an unrelated reason. Verified end-to-end by actually executing both step scripts (extracted via a real YAML parse, not hand-transcribed) against mocked tcc.exe/ run.sh across 6 scenarios: legacy-match, legacy-mismatch, provenance+ deferred-check-passes, provenance+deferred-check-fails, provenance+ no-GC-fails, and provenance+full-pass. This caught a real bug before pushing: passing free-form reason text (containing an apostrophe) through a sourced .env file broke on the unescaped quote; switched to plain files read via command substitution instead, which doesn't re-parse the content as shell syntax.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17dfad9c4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ary failures unconditionally blocking Three successive attempts on this PR (58f7e17, 29d5da8, 17dfad9) tried to let a provenance-tracked tcc.exe's conformance mismatch be downgraded to a warning via progressively narrower proxy checks - a no-GC compile, then also a GC-allocation test (circular: the same test run.sh had already run against the same artifacts), then a deferred test against a freshly-rebuilt libgc.dylib. Codex correctly rejected each one on the same grounds: none of them test what a real consumer checking out this exact commit actually receives (this checked-in tcc.exe paired with this checked-in libgc.a, not some other libgc nobody ships), so none of them can license treating a real, accurate failure signal as informational. There is no proxy that resolves this without changing what's actually being verified. Revert to the simple, original vlang#74 design: any checked- in tcc.exe/libgc.a failure that doesn't match the ancient binary's exact known signature is a hard failure, full stop - whether or not build_source_hash.txt is present. That file is now used only to annotate failure messages with a diagnostic hint (update_tccbin.yml rebuilds tcc.exe without ever rebuilding libgc.a to match, which is the likely root cause), never to relax the check itself. This means d712d0f - the current tip of this branch, and the reason this PR exists - now correctly fails CI again, honestly. That failure reflects a genuine defect in the distributed pair, not a false alarm: fixing it requires update_tccbin.yml (in vlang/v) to also rebuild libgc.a whenever it rebuilds tcc.exe, or a human to do so manually - not a CI workaround. Verified the reverted logic end-to-end (extracted from the real YAML, mocked tools) across 5 scenarios: legacy-match, legacy-mismatch, provenance-mismatch (now correctly hard-fails), provenance-full-pass (still demands XFAIL removal, unchanged), and provenance non-aggregate shape. No dangling references to the removed sanity-check/deferred- verification machinery remain.
quaesitor-scientiam
deleted the
fix/checkedin-binary-not-permanent-legacy
branch
July 30, 2026 19:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
d712d0f(a manualvlang-bottest run ofupdate_tccbin.ymlagainstTinyCC
85ba3ae8/ 0.9.28rc, not the scheduled monthly cron) is thecurrent tip of this branch and fails CI in the "verify the checked-in
tcc.exe (as distributed, before rebuild)" step.
Root cause:
is_known_checkedin_error's signature check ("library 'c'not found") was reverse-engineered in #74 against one specific,
permanent-seeming legacy binary (the ancient v0.9.27
tcc.exethatpredates this CI). That binary isn't actually permanent -
update_tccbin.ymlcan (and here, did) replace it with a fresh buildfrom any TinyCC commit, recording that commit in
build_source_hash.txt- a file the legacy binary never had (absentat #74's original merge commit
b85b420, present from the firstrebuild onward).
On this fresh 0.9.28rc build, the direct
crash.cprobe now returnsexit 0 with empty output instead of the expected "library 'c' not
found" text. That's not a regression - it's just a different build
this specific check wasn't written to characterize - but the existing
logic couldn't tell the difference and hard-failed the job, which in
turn skipped the entire rebuild-from-source pipeline further down (the
actual meaningful correctness gate for what this commit ships).
Fix: gate the hard-fail on
build_source_hash.txt's absence. Whenit's present (checked-in binary has been refreshed by the automated
pipeline), a signature mismatch is now a
::warning::instead ofexit 1, and the job proceeds to the real rebuild-then-test lanes.When absent (still the pinned legacy binary), all existing hard-fail
behavior is unchanged byte-for-byte.
Verified the changed branching logic directly against 4 cases (real
observed failure, legacy binary matching signature, legacy binary
mismatched, legacy binary different shape) - only the first now avoids
a hard fail; the other 3 (including both regression-guard cases)
behave exactly as before. Didn't touch the
lipo/otool/ar/nmcalls elsewhere in the step - those validate
libgc.a, which the botscript always copies through unchanged regardless of
TCC_COMMIT, sothey remain valid regardless of which
tcc.exeis checked in.