-
Notifications
You must be signed in to change notification settings - Fork 291
Update Android test-command handling #2590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This PR is ready for review. |
cibuildwheel/platforms/android.py
Outdated
test_args = shlex.split(test_command) | ||
if test_args[:2] in [["python", "-c"], ["python", "-m"]]: | ||
test_args[:3] = [test_args[1], test_args[2], "--"] | ||
if test_args[0] == "python" and any(arg in test_args for arg in ["-c", "-m"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python also supports merged short args, like python3 -Om calendar
. Maybe a leading python
should be stripped regardless of what follows? Also, what about python3
?
Also, I think this would read nicely with pattern matching:
in_test_args = shlex.split(test_command)
match in_test_args:
case ["python" | "python3", *test_args]:
pass
case ["pytest", *_]:
# We transform some commands into the `python -m` form, but this is deprecated.
msg = (
f"Test command {test_command!r} is not supported on Android. "
"cibuildwheel will try to execute it as if it started with 'python -m'. "
"If this works, all you need to do is add that to your test command."
)
log.warning(msg)
test_args = ["-m", *in_test_args]
case _:
msg = (
f"Test command {test_command!r} is not supported on Android. "
f"Command must begin with 'python' and contain '-m' or '-c'."
)
raise errors.FatalError(msg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python also supports merged short args, like
python3 -Om calendar
.
That's a good point, but the arguments are processed by the cpython test script, which, if it doesn't find a literal -m
or -c
, will insert -m test
at the start of the command. This would be very confusing for cibuildwheel users.
The script could be improved to detect merged short args, but then this PR would be blocked for 2 months until the next Python release.
This PR also fixes a regression in cibuildwheel 3.2.1: it updated Android in build-platforms.toml to Python 3.14.0, which has the new version of the test script, but didn't include the cibuildwheel side of the changes. This will break Android testing on Python 3.14. To make sure this doesn’t happen again, I’ve updated test_expected_wheels
so it not only builds on all Python versions, but also tests on them.
Also, what about
python3
?
I've updated the code to accept that.
…n 3.14, and add cross venv tests
To avoid merge conflicts, I've also included a fix for #2611. @freakboy3742: |
In python/cpython#138805 I'm updating the Android testbed so it can take all Python command line options, not just
-c
and-m
. However, this will be slightly backward incompatible for cibuildwheel, because the-c
or-m
must be moved after the--
separator on the testbed command line. So this PR will need to remain a draft until the next releases of Python 3.13 and 3.14, which are currently scheduled for October 7.