Jens-G opened a new pull request, #3657:
URL: https://github.com/apache/thrift/pull/3657
## Summary
The `test_keyword_escape.py` regression test added in THRIFT-5927 has never
actually exercised the compiler in CI, and can pick up the wrong compiler
locally. Confirmed the THRIFT-5927 generator fix itself is correct on master
(built the compiler fresh and ran it against `Thrift5927.thrift` directly) —
this PR only fixes the test/build-infrastructure gap that was masking it.
Root causes:
- `lib/py/Makefile.am`'s `check-local` recipe invoked the test as plain
`$(PYTHON) test/test_compiler/test_keyword_escape.py`, without passing the
`THRIFT` make variable through as an environment variable, unlike other
Makefile.am files in the tree that need the compiler at test time.
- `find_thrift()`'s relative-path fallback was off by one directory level
(3x `../` instead of 4, from `lib/py/test/test_compiler/`), so it could never
resolve to `compiler/cpp/thrift` or `build/compiler/cpp/bin/thrift` either.
With neither path working and no system-wide `thrift` on PATH,
`find_thrift()` returns `None` and the test prints "thrift compiler not found,
skipping test" and returns success — a false green on every CI run since
THRIFT-5927 merged (`lib-python` job in `.github/workflows/build.yml` downloads
the compiler artifact to exactly `compiler/cpp/thrift`, which the broken
fallback failed to reach).
## Fix
- Pass `THRIFT=${THRIFT}` through explicitly on the `check-local` recipe
line.
- Correct the off-by-one in the relative-path fallback so standalone
invocations (without `make`) still work.
## Test plan
Reproduced and verified against a real autotools build (bootstrap →
configure → compiler artifact placed at `compiler/cpp/thrift`, mirroring the CI
job exactly):
- [x] Before the fix: `make -C lib/py check-local` silently skips the test
(`WARNING: thrift compiler not found, skipping test`) even though the compiler
is present and working.
- [x] After the fix: `make -C lib/py check-local` runs the test for real
(`OK: All 5 generated Python files compile successfully`).
- [x] Standalone run with no `THRIFT` env and no PATH `thrift`: corrected
fallback path finds the compiler and passes.
- [x] Standalone run with genuinely no compiler available anywhere: graceful
skip behavior is preserved (no regression there).
---
This PR includes AI-assisted changes (Claude Code); see commit trailer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
