gh-137242: Allow Android testbed to take all Python command-line options #138805
Conversation
mhsmith
added
skip news
needs backport to 3.13
|
|
||
| # Add warnings filter 'error' | ||
| if 'default' not in sys.warnoptions: | ||
| if 'error' not in sys.warnoptions: |
There was a problem hiding this comment.
Is this intentional? It seems like a significant behavior change.
There was a problem hiding this comment.
This fixes what I'm pretty sure was an oversight in #128973, which made warnings into errors in --fast-ci and --slow-ci.
But I don't think this change will make any difference to the other platforms, because sys.warnoptions represents the Python command line, not the actual warnings state. The other platforms aren't passing any warning options on the command line, so this if evaluates to true either way.
|
|
||
| # Unbuffered stdout and stderr. This isn't helpful on Android, because | ||
| # it would cause lines to be split into multiple log messages. | ||
| if not sys.stdout.write_through and sys.platform != "android": |
There was a problem hiding this comment.
This doesn't feel like the right place to check for this. If _add_ci_python_opts() does anything to modify the environment, then L716 below won't trigger, and an attempt will be made to run the test suite as a subprocess. It seems to me that we should be doing a higher-level check as to whether a subprocess test execution is possible at all, and avoiding the issue there.
In the #138176, when I added --fast/slow-ci support, I also used --dont-add-python-opts as a way to prevent environment modification (and thus the subprocess); that seems closer to the intent here.
There was a problem hiding this comment.
A higher-level check to avoid the subprocess is a good idea, since it'll give a more understandable warning on Android, and avoid the instant crash when you try to start a subprocess on iOS.
However, I think --dont-add-python-opts is risky, because that'll lead to the mobile CI and buildbots using different options to the other platforms without warning. For example, when #128973 added -W error 6 months ago, it had no effect on the Android and iOS buildbots because they weren't using --slow-ci yet. When the Android buildbot started using --slow-ci, its status immediately turned orange because of the Python options mismatch. --dont-add-python-opts would have concealed that issue, possibly leading to bugs going unnoticed.
| # Some platforms cannot change options after startup, so if these | ||
| # options are changed, also update the copies in: | ||
| # * cpython/Android/android.py | ||
| # * buildmaster-config/master/custom/factories.py |
There was a problem hiding this comment.
To confirm - is this an interim comment, or a long term plan? I thought the intention was to get all the options encoded into the android.py script so that "run CI checks" encoded all the defaults, rather than relying on multiple points of configuration.
There was a problem hiding this comment.
The idea is that _add_ci_python_opts checks the expected options, and then android.py and buildmaster-config actually specify those options. The duplication is unfortunate, but I can't see any way of avoiding it.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
#138649 and python/buildmaster-config#628 switched GitHub Actions and the buildbots over to using
--fast-ciand--slow-cion Android. However, this has introduced a warning, which has turned the buildbots orange:Android runs the tests embedded in a larger app, so it's not possible to change the Python options by calling
execv. Instead, this PR adds the ability to pass all Python command-line options to the Android testbed, so the options can be set to the expected values from the start.This may also be useful in cibuildwheel, as it'll allow the testbed to accept a wider range of Python test commands (pypa/cibuildwheel#2590).