SEZ9 commented on PR #12740: URL: https://github.com/apache/seatunnel/pull/12740#issuecomment-6206403113
@karthik-0306 thanks for the follow-up — confirmed, `6acc237f` includes the JAVA_HOME error changes (attempted value plus fix/unset guidance for both the missing-binary and too-old-JDK paths) and the new too-old test, so my two asks are addressed. Your Windows run on Python 3.9 and 3.13 (28 passed: 21 detector + 7 existing SDK) plus CryoThrust's macOS check with real Java 8/11/17 gives me enough confidence in the detection helper across platforms; I don't need you to run macOS or Linux manually. Also agree with leaving the `isfile` check as is — the `OSError` branch already turns a non-executable `JAVA_HOME/bin/java` into a clear `JavaDetectionError`, so no need to add `os.access`. Two small things left before merge: 1. The `Build` check on `6acc237f` reportedly has one failed job, `Run / engine-v2-it (11, ubuntu-latest)`. Since this PR only touches Python files under `tools/seatunnel-python-sdk`, that looks unrelated to the diff, but please re-run just that job from your fork's Actions run so we get a green `Build`. No code change expected. 2. Could you confirm whether the new detector tests actually execute in CI (i.e. on a Linux runner), or only locally? If they are not wired into CI yet, let me know and we can decide whether that belongs in this PR or the follow-up for the launcher. Once `Build` is green I'll do the final pass for merge. <!-- streview-comment:1779 --> -- 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]
