Repository navigation
gh-140239: Check for statx on Android - #140395
vstinner merged 1 commit into
Conversation
Sorry, something went wrong.
Android has Linux's statx, but MACHDEP is "android" on Android, so configure doesn't check for statx on Android. Base the check for statx on ac_sys_system instead, which is "Linux-android" on Android, "Linux" on other Linux distributions, and "AIX" on AIX (which has an incompatible function named statx).
|
I didn't knew MACHDEP is not Linux in Android. Sorry for that. Thanks for all the details. Will be more thorough in the future., |
Sorry, something went wrong.
|
@ayappanec I appreciate that you're keeping up with the main branch and reporting issues promptly. Don't worry about this. Given the maybe-also-wrong check right below it, it took me about 30 minutes to figure this out. |
Sorry, something went wrong.
|
!buildbot Android |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @vstinner for commit 0a8f9cc 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F140395%2Fmerge The command will test the builders whose names match following regular expression: Android The builders matched are:
|
Sorry, something went wrong.
mhsmith
left a comment
There was a problem hiding this comment.
- If the Android buildbots still exist and use API level 30 or higher (#123014 indicates they "will" use 34), please run the Android buildbots to make sure the tests pass.
The buildbots may run on API level 34, but they build against API level 21 or 24, so they still won't detect the function. However, I've tested this PR manually by setting the ANDROID_API_LEVEL environment variable to 30, and it builds and passes the statx tests.
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
I merged your PR, thanks. |
Sorry, something went wrong.
configure says checking for statx... yes. But I don't see test_os in run tests.
configure says checking for statx... no. |
Sorry, something went wrong.
|
As expected, the AMD64 build says statx... yes for the "build Python" for Linux, but no for the "host Python" for Android. And it looks like test_os appears normally at line 390 of the test log. |
Sorry, something went wrong.
|
Oh ok, I looked at the wrong line. Thanks for correcting me :-) |
Sorry, something went wrong.
Android has Linux's statx, but MACHDEP is "android" on Android, so configure doesn't check for statx on Android. Base the check for statx on ac_sys_system instead, which is "Linux-android" on Android, "Linux" on other Linux distributions, and "AIX" on AIX (which has an incompatible function named statx).
#140249 inadvertently disabled checking for statx on Android because MACHDEP is "android" on Android. You can verify this by looking at the logs from the Android runner from #140249. This is a cross-compile. In the "Configure build Python" logs, MACHDEP is linux and configure checks for statx; in the "Configure host Python" logs, MACHDEP is android and configure doesn't check for statx (would be just after checking for writev).
Base the check for statx on ac_sys_system instead, which is Linux-android on Android, Linux on other Linux distributions, and AIX on AIX (which has an incompatible function named statx that prompted #140249).
Before merging:
cc @ayappanec as author of #140249
cc @vstinner as reviewer of statx-related PRs
cc @mhsmith as Android buildbot operator and Android maintainer
(There's a "$MACHDEP" != linux check just below the code this PR changes, which does not exclude Android as may have been intended. Android introduced lchmod in API level 36. lchmod delegates to fchmodat with AT_SYMLINK_NOFOLLOW, and fchmodat has code that should work on non-symlinks and give the correct errno for symlinks, so I think the behavior is acceptable, but we aren't testing it. That can be a separate issue/PR if it's a problem.)