From 103f97e0ff0c65fea77d12083cd4d2b2c0aba46f Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Wed, 10 Jul 2024 20:42:35 +0100 Subject: [PATCH 01/34] Update to Android Gradle plugin version 8.4.2 --- Android/testbed/build.gradle.kts | 2 +- Android/testbed/gradle/wrapper/gradle-wrapper.properties | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Android/testbed/build.gradle.kts b/Android/testbed/build.gradle.kts index 53f4a67287fcc5..34884f724412ea 100644 --- a/Android/testbed/build.gradle.kts +++ b/Android/testbed/build.gradle.kts @@ -1,5 +1,5 @@ // Top-level build file where you can add configuration options common to all sub-projects/modules. plugins { - id("com.android.application") version "8.2.2" apply false + id("com.android.application") version "8.4.2" apply false id("org.jetbrains.kotlin.android") version "1.9.22" apply false } \ No newline at end of file diff --git a/Android/testbed/gradle/wrapper/gradle-wrapper.properties b/Android/testbed/gradle/wrapper/gradle-wrapper.properties index 2dc3339a3ef213..57b2f57cc86b51 100644 --- a/Android/testbed/gradle/wrapper/gradle-wrapper.properties +++ b/Android/testbed/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ #Mon Feb 19 20:29:06 GMT 2024 distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.2-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists From fb1b54ab13f9d802ec69a7e1175b9be085257442 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Wed, 10 Jul 2024 20:46:55 +0100 Subject: [PATCH 02/34] Separate Python test runner from MainActivity so it can be run from a JUnit test suite --- Android/testbed/app/build.gradle.kts | 4 +++ .../java/org/python/testbed/PythonSuite.kt | 23 +++++++++++++ .../testbed/app/src/main/c/main_activity.c | 12 +++---- .../java/org/python/testbed/MainActivity.kt | 34 ++++++++++++++----- Android/testbed/app/src/main/python/main.py | 12 ++++--- 5 files changed, 66 insertions(+), 19 deletions(-) create mode 100644 Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 7690d3fd86b2fd..970c879c0a7953 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -38,6 +38,8 @@ android { externalNativeBuild.cmake.arguments( "-DPYTHON_CROSS_DIR=$PYTHON_CROSS_DIR", "-DPYTHON_VERSION=$PYTHON_VERSION") + + testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" } externalNativeBuild.cmake { @@ -61,6 +63,8 @@ dependencies { implementation("androidx.appcompat:appcompat:1.6.1") implementation("com.google.android.material:material:1.11.0") implementation("androidx.constraintlayout:constraintlayout:2.1.4") + androidTestImplementation("androidx.test.ext:junit:1.1.5") + androidTestImplementation("androidx.test:rules:1.5.0") } diff --git a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt new file mode 100644 index 00000000000000..50ecf393dfab6e --- /dev/null +++ b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt @@ -0,0 +1,23 @@ +package org.python.testbed + +import androidx.test.annotation.UiThreadTest +import androidx.test.platform.app.InstrumentationRegistry +import androidx.test.ext.junit.runners.AndroidJUnit4 + +import org.junit.Test +import org.junit.runner.RunWith + +import org.junit.Assert.* + + +@RunWith(AndroidJUnit4::class) +class PythonSuite { + @Test + @UiThreadTest + fun testPython() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + val args = InstrumentationRegistry.getArguments().getString("pythonArgs", "") + val status = PythonTestRunner(context).run(args) + assertEquals(0, status) + } +} \ No newline at end of file diff --git a/Android/testbed/app/src/main/c/main_activity.c b/Android/testbed/app/src/main/c/main_activity.c index 73aba4164d000f..534709048990c6 100644 --- a/Android/testbed/app/src/main/c/main_activity.c +++ b/Android/testbed/app/src/main/c/main_activity.c @@ -84,7 +84,7 @@ static char *redirect_stream(StreamInfo *si) { return 0; } -JNIEXPORT void JNICALL Java_org_python_testbed_MainActivity_redirectStdioToLogcat( +JNIEXPORT void JNICALL Java_org_python_testbed_PythonTestRunner_redirectStdioToLogcat( JNIEnv *env, jobject obj ) { for (StreamInfo *si = STREAMS; si->file; si++) { @@ -115,7 +115,7 @@ static void throw_status(JNIEnv *env, PyStatus status) { throw_runtime_exception(env, status.err_msg ? status.err_msg : ""); } -JNIEXPORT void JNICALL Java_org_python_testbed_MainActivity_runPython( +JNIEXPORT int JNICALL Java_org_python_testbed_PythonTestRunner_runPython( JNIEnv *env, jobject obj, jstring home, jstring runModule ) { PyConfig config; @@ -125,13 +125,13 @@ JNIEXPORT void JNICALL Java_org_python_testbed_MainActivity_runPython( status = set_config_string(env, &config, &config.home, home); if (PyStatus_Exception(status)) { throw_status(env, status); - return; + return 1; } status = set_config_string(env, &config, &config.run_module, runModule); if (PyStatus_Exception(status)) { throw_status(env, status); - return; + return 1; } // Some tests generate SIGPIPE and SIGXFSZ, which should be ignored. @@ -140,8 +140,8 @@ JNIEXPORT void JNICALL Java_org_python_testbed_MainActivity_runPython( status = Py_InitializeFromConfig(&config); if (PyStatus_Exception(status)) { throw_status(env, status); - return; + return 1; } - Py_RunMain(); + return Py_RunMain(); } diff --git a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt index 5a590d5d04e954..8e5166f518127d 100644 --- a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt +++ b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt @@ -1,38 +1,56 @@ package org.python.testbed +import android.content.Context import android.os.* import android.system.Os import android.widget.TextView import androidx.appcompat.app.* import java.io.* + +// Launching the tests from an activity is OK for a quick check, but for +// anything more complicated it'll be more use `android.py test` to launch the +// tests via PythonSuite. class MainActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) setContentView(R.layout.activity_main) + val status = PythonTestRunner(this).run() + findViewById(R.id.tvHello).text = "Exit status $status" + } +} + + +class PythonTestRunner(val context: Context) { + /** @param args Extra arguments for `python -m test`. + * @return The Python exit status: zero if the tests passed, nonzero if + * they failed. */ + fun run(args: String = "") : Int { + Os.setenv("PYTHON_ARGS", args, true) // Python needs this variable to help it find the temporary directory, // but Android only sets it on API level 33 and later. - Os.setenv("TMPDIR", cacheDir.toString(), false) + Os.setenv("TMPDIR", context.cacheDir.toString(), false) val pythonHome = extractAssets() System.loadLibrary("main_activity") redirectStdioToLogcat() - runPython(pythonHome.toString(), "main") - findViewById(R.id.tvHello).text = "Python complete" + + // The main module is in src/main/python/main.py. + return runPython(pythonHome.toString(), "main") } private fun extractAssets() : File { - val pythonHome = File(filesDir, "python") + val pythonHome = File(context.filesDir, "python") if (pythonHome.exists() && !pythonHome.deleteRecursively()) { throw RuntimeException("Failed to delete $pythonHome") } - extractAssetDir("python", filesDir) + extractAssetDir("python", context.filesDir) return pythonHome } private fun extractAssetDir(path: String, targetDir: File) { - val names = assets.list(path) + val names = context.assets.list(path) ?: throw RuntimeException("Failed to list $path") val targetSubdir = File(targetDir, path) if (!targetSubdir.mkdirs()) { @@ -43,7 +61,7 @@ class MainActivity : AppCompatActivity() { val subPath = "$path/$name" val input: InputStream try { - input = assets.open(subPath) + input = context.assets.open(subPath) } catch (e: FileNotFoundException) { extractAssetDir(subPath, targetDir) continue @@ -57,5 +75,5 @@ class MainActivity : AppCompatActivity() { } private external fun redirectStdioToLogcat() - private external fun runPython(home: String, runModule: String) + private external fun runPython(home: String, runModule: String) : Int } \ No newline at end of file diff --git a/Android/testbed/app/src/main/python/main.py b/Android/testbed/app/src/main/python/main.py index a1b6def34ede81..b9fd0f126f22a6 100644 --- a/Android/testbed/app/src/main/python/main.py +++ b/Android/testbed/app/src/main/python/main.py @@ -1,4 +1,6 @@ +import os import runpy +import shlex import signal import sys @@ -8,10 +10,10 @@ # profile save"), so disabling it should not weaken the tests. signal.pthread_sigmask(signal.SIG_UNBLOCK, [signal.SIGUSR1]) -# To run specific tests, or pass any other arguments to the test suite, edit -# this command line. sys.argv[1:] = [ - "--use", "all,-cpu", - "--verbose3", -] + "-uall", # Enable all resources + "-W", # Display test output on failure +] + shlex.split(os.environ["PYTHON_ARGS"]) + +# The test module will call sys.exit to indicate whether the tests passed. runpy.run_module("test") From b99b76942d6add61143180ecaf4bfa16e3bc73db Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Wed, 10 Jul 2024 21:12:52 +0100 Subject: [PATCH 03/34] Add `android.py test` command --- Android/android.py | 190 +++++++++++++++++++++++++-- Android/testbed/app/build.gradle.kts | 14 ++ 2 files changed, 193 insertions(+), 11 deletions(-) diff --git a/Android/android.py b/Android/android.py index 0a1393e61ddb0e..cc59562be3b170 100755 --- a/Android/android.py +++ b/Android/android.py @@ -1,21 +1,45 @@ #!/usr/bin/env python3 import argparse +import atexit import os import re +import shlex import shutil import subprocess import sys import sysconfig from os.path import basename, relpath from pathlib import Path +from subprocess import check_output from tempfile import TemporaryDirectory +from threading import Thread +from time import sleep + SCRIPT_NAME = Path(__file__).name CHECKOUT = Path(__file__).resolve().parent.parent +ANDROID_DIR = CHECKOUT / "Android" +TESTBED_DIR = ANDROID_DIR / "testbed" CROSS_BUILD_DIR = CHECKOUT / "cross-build" +try: + android_home = os.environ['ANDROID_HOME'] +except KeyError: + sys.exit("The ANDROID_HOME environment variable is required.") + +adb = Path( + f"{android_home}/platform-tools/adb" + + (".exe" if os.name == "nt" else "") +) +if not adb.exists(): + sys.exit( + f"{adb} does not exist. Install the Platform Tools package using the " + f"Android SDK manager." + ) + + def delete_if_exists(path): if path.exists(): print(f"Deleting {path} ...") @@ -36,10 +60,13 @@ def subdir(name, *, clean=None): return path -def run(command, *, host=None, **kwargs): - env = os.environ.copy() +def run(command, *, host=None, env=None, **kwargs): + if env is None: + env = os.environ.copy() + original_env = env.copy() + if host: - env_script = CHECKOUT / "Android/android-env.sh" + env_script = ANDROID_DIR / "android-env.sh" env_output = subprocess.run( f"set -eu; " f"HOST={host}; " @@ -60,11 +87,10 @@ def run(command, *, host=None, **kwargs): print(line) env[key] = value - if env == os.environ: + if env == original_env: raise ValueError(f"Found no variables in {env_script.name} output:\n" + env_output) - print(">", " ".join(map(str, command))) try: subprocess.run(command, check=True, env=env, **kwargs) except subprocess.CalledProcessError as e: @@ -173,12 +199,11 @@ def clean_all(context): def setup_testbed(context): ver_long = "8.7.0" ver_short = ver_long.removesuffix(".0") - testbed_dir = CHECKOUT / "Android/testbed" for filename in ["gradlew", "gradlew.bat"]: out_path = download( f"https://raw.githubusercontent.com/gradle/gradle/v{ver_long}/{filename}", - testbed_dir) + TESTBED_DIR) os.chmod(out_path, 0o755) with TemporaryDirectory(prefix=SCRIPT_NAME) as temp_dir: @@ -187,10 +212,140 @@ def setup_testbed(context): f"https://services.gradle.org/distributions/gradle-{ver_short}-bin.zip") outer_jar = f"gradle-{ver_short}/lib/plugins/gradle-wrapper-{ver_short}.jar" run(["unzip", bin_zip, outer_jar]) - run(["unzip", "-o", "-d", f"{testbed_dir}/gradle/wrapper", outer_jar, + run(["unzip", "-o", "-d", f"{TESTBED_DIR}/gradle/wrapper", outer_jar, "gradle-wrapper.jar"]) +def list_devices(): + serials = [] + header_found = False + + lines = check_output([adb, "devices"], text=True).splitlines() + for line in lines: + # Ignore blank lines, and all lines before the header. + line = line.strip() + if line == "List of devices attached": + header_found = True + elif header_found and line: + try: + serial, status = line.split() + except ValueError: + raise ValueError(f"failed to parse {line!r}") + if status == "device": + serials.append(serial) + + if not header_found: + raise ValueError(f"failed to parse {lines}") + return serials + + +def wait_for_new_device(initial_devices): + while True: + new_devices = set(list_devices()).difference(initial_devices) + if len(new_devices) == 0: + sleep(1) + elif len(new_devices) == 1: + return new_devices.pop() + else: + sys.exit(f"Found more than one new device: {new_devices}") + + +def wait_for_uid(): + while True: + lines = check_output( + [adb, "shell", "pm", "list", "packages", "-U", "org.python.testbed"], + text=True + ).splitlines() + + if len(lines) == 0: + sleep(1) + elif len(lines) == 1: + if match := re.search(r"uid:\d+", lines[0]): + return match[1] + else: + raise ValueError(f"failed to parse {lines[0]!r}") + else: + sys.exit(f"Found more than one UID: {lines}") + + +def logcat_thread(context, initial_devices): + serial = context.connected or wait_for_new_device(initial_devices) + + # Because Gradle uninstalls the app after running the tests, its UID should + # be different every time. There's therefore no need to filter the logs by + # timestamp or PID. + uid = wait_for_uid() + + logcat = subprocess.Popen( + [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"], + stdout=subprocess.PIPE, text=True + ) + + # This is a daemon thread, so `finally` won't work. + atexit.register(logcat.kill) + + for line in logcat.stdout: + if match := re.fullmatch(r"(\w)/(\w+): (.*)", line): + level, tag, message = match.groups() + else: + # If the regex doesn't match, this is probably the second or + # subsequent line of a multi-line message. Python won't produce + # such messages, but other components might. + level, tag, message = None, None, line + + stream = ( + sys.stderr + if level in ["E", "F"] # ERROR and FATAL (aka ASSERT) + else sys.stdout + ) + + # We strip the level/tag indicator from Python's stdout and stderr, to + # simplify automated processing of the output, e.g. a buildbot posting a + # failure notice on a GitHub PR. + # + # Non-Python messages from the app are still worth keeping, as they may + # help explain any problems. + stream.write( + message if tag in ["python.stdout", "python.stderr"] else line + ) + + status = logcat.wait() + if status != 0: + sys.exit(f"Logcat exit status {status}") + + +def run_testbed(context): + if not (TESTBED_DIR / "gradlew").exists(): + setup_testbed(context) + + kwargs = dict(cwd=TESTBED_DIR) + + if context.connected: + task_prefix = "connected" + env = os.environ.copy() + env["ANDROID_SERIAL"] = context.connected + kwargs.update(env=env) + elif context.managed: + task_prefix = context.managed + else: + raise ValueError("no device argument found") + + Thread( + target=logcat_thread, args=(context, list_devices()), daemon=True + ).start() + + run( + [ + "./gradlew", + "--console", "plain", + f"{task_prefix}DebugAndroidTest", + "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" + + shlex.join(context.args), + ], + **kwargs + ) + + def main(): parser = argparse.ArgumentParser() subcommands = parser.add_subparsers(dest="subcommand") @@ -206,8 +361,6 @@ def main(): help="Run `make` for Android") subcommands.add_parser( "clean", help="Delete the cross-build directory") - subcommands.add_parser( - "setup-testbed", help="Download the testbed Gradle wrapper") for subcommand in build, configure_build, configure_host: subcommand.add_argument( @@ -222,6 +375,20 @@ def main(): subcommand.add_argument("args", nargs="*", help="Extra arguments to pass to `configure`") + subcommands.add_parser( + "setup-testbed", help="Download the testbed Gradle wrapper") + test = subcommands.add_parser( + "test", help="Run the test suite") + device_group = test.add_mutually_exclusive_group(required=True) + device_group.add_argument( + "--connected", metavar="SERIAL", help="Run on a connected device. " + "Connect it yourself, then get its serial from `adb devices`.") + device_group.add_argument( + "--managed", metavar="NAME", help="Run on a Gradle-managed device. " + "These are defined in `managedDevices` in testbed/app/build.gradle.kts.") + test.add_argument( + "args", nargs="*", help="Extra arguments for `python -m test`") + context = parser.parse_args() dispatch = {"configure-build": configure_build_python, "make-build": make_build_python, @@ -229,7 +396,8 @@ def main(): "make-host": make_host_python, "build": build_all, "clean": clean_all, - "setup-testbed": setup_testbed} + "setup-testbed": setup_testbed, + "test": run_testbed} dispatch[context.subcommand](context) diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 970c879c0a7953..72a199d576d6ba 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -57,6 +57,20 @@ android { kotlinOptions { jvmTarget = "1.8" } + + testOptions { + managedDevices { + localDevices { + // In the future we may add a "minSdk" device, but managed + // devices have a minimum API level of 27. + create("targetSdk") { + device = "Small Phone" + apiLevel = defaultConfig.targetSdk!! + systemImageSource = "aosp-atd" + } + } + } + } } dependencies { From 6f07a395bb58c6f358dc71e80f80f05be0d16b9c Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 23 Jul 2024 22:07:54 +0100 Subject: [PATCH 04/34] --connected mode working --- Android/android.py | 262 +++++++++++++++++++++++++++++---------------- 1 file changed, 172 insertions(+), 90 deletions(-) diff --git a/Android/android.py b/Android/android.py index cc59562be3b170..4e929edf054f8f 100755 --- a/Android/android.py +++ b/Android/android.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 +import asyncio import argparse -import atexit import os import re import shlex @@ -9,12 +9,11 @@ import subprocess import sys import sysconfig +from asyncio import wait_for +from contextlib import asynccontextmanager from os.path import basename, relpath from pathlib import Path -from subprocess import check_output from tempfile import TemporaryDirectory -from threading import Thread -from time import sleep SCRIPT_NAME = Path(__file__).name @@ -216,11 +215,58 @@ def setup_testbed(context): "gradle-wrapper.jar"]) -def list_devices(): +# Work around a bug involving sys.exit and TaskGroups +# (https://github.com/python/cpython/issues/101515). +def exit(*args): + raise MySystemExit(*args) + + +class MySystemExit(Exception): + pass + + +# The `test` subcommand runs all subprocesses through this context manager so +# that no matter what happens, they can always be cancelled from another task, +# and they will always be cleaned up on exit. +@asynccontextmanager +async def async_process(*args, **kwargs): + process = await asyncio.create_subprocess_exec(*args, **kwargs) + try: + yield process + finally: + if process.returncode is None: + process.kill() + + # Even after killing the process we must still wait for it, + # otherwise we'll get "Exception ignored in __del__" messages. + await wait_for(process.wait(), timeout=1) + + +async def async_check_output(*args, **kwargs): + async with async_process( + *args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs + ) as process: + stdout, stderr = await process.communicate() + if process.returncode == 0: + return stdout.decode("UTF-8", "backslashreplace") + else: + sys.stdout.buffer.write(stdout) + sys.stdout.flush() + sys.stderr.buffer.write(stderr) + sys.stderr.flush() + exit( + f"Command {args} returned non-zero exit status " + f"{process.returncode}" + ) + + +# Return a list of the serial numbers of connected devices. Emulators will have +# serials of the form "emulator-5678". +async def list_devices(): serials = [] header_found = False - lines = check_output([adb, "devices"], text=True).splitlines() + lines = (await async_check_output(adb, "devices")).splitlines() for line in lines: # Ignore blank lines, and all lines before the header. line = line.strip() @@ -239,111 +285,141 @@ def list_devices(): return serials -def wait_for_new_device(initial_devices): +async def find_device(context, initial_devices): + if context.connected: + return context.connected + while True: - new_devices = set(list_devices()).difference(initial_devices) + new_devices = set(await list_devices()).difference(initial_devices) if len(new_devices) == 0: - sleep(1) + await asyncio.sleep(1) elif len(new_devices) == 1: - return new_devices.pop() + device = new_devices.pop() + print(f"Found device: {device}") + return device else: - sys.exit(f"Found more than one new device: {new_devices}") + exit(f"Found more than one new device: {new_devices}") -def wait_for_uid(): +async def find_uid(serial): + testbed_package = "org.python.testbed" while True: - lines = check_output( - [adb, "shell", "pm", "list", "packages", "-U", "org.python.testbed"], - text=True - ).splitlines() - - if len(lines) == 0: - sleep(1) - elif len(lines) == 1: - if match := re.search(r"uid:\d+", lines[0]): - return match[1] - else: + lines = (await async_check_output( + adb, "-s", serial, "shell", + "pm", "list", "packages", "-U", testbed_package + )).splitlines() + + # The unit test package org.python.testbed.test will also be returned. + for line in lines: + match = re.fullmatch(r"package:(\S+) uid:(\d+)", lines[0]) + if not match: raise ValueError(f"failed to parse {lines[0]!r}") - else: - sys.exit(f"Found more than one UID: {lines}") + package, uid = match.groups() + if package == testbed_package: + print(f"Found UID: {uid}") + return uid + await asyncio.sleep(1) -def logcat_thread(context, initial_devices): - serial = context.connected or wait_for_new_device(initial_devices) + +async def logcat_task(context, initial_devices): + serial = await find_device(context, initial_devices) # Because Gradle uninstalls the app after running the tests, its UID should # be different every time. There's therefore no need to filter the logs by # timestamp or PID. - uid = wait_for_uid() + uid = await find_uid(serial) + async with async_process( + adb, "-s", serial, "logcat", + "--uid", uid, + "--format", "tag", + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + ) as process: + while line := (await process.stdout.readline()).decode( + "UTF-8", "backslashreplace" + ): + if match := re.fullmatch(r"([A-Z])/(.*)", line, re.DOTALL): + level, message = match.groups() + else: + # If the regex doesn't match, this is probably the second or + # subsequent line of a multi-line message. Python won't produce + # such messages, but other components might. + level, message = None, line + + # Put high-level messages on stderr so they're highlighted in the + # buildbot logs. This will include Python's own stderr. + stream = ( + sys.stderr + if level in ["E", "F"] # ERROR and FATAL (aka ASSERT) + else sys.stdout + ) + + # To simplify automated processing of the output, e.g. a buildbot + # posting a failure notice on a GitHub PR, we strip the level and + # tag indicators from Python's stdout and stderr. + for prefix in ["python.stdout: ", "python.stderr: "]: + if message.startswith(prefix): + stream.write(message.removeprefix(prefix)) + break + else: + if context.verbose: + # Non-Python messages add a lot of noise, but they may + # sometimes help explain a failure. + stream.write(line) - logcat = subprocess.Popen( - [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"], - stdout=subprocess.PIPE, text=True - ) + # If the device disconnects while logcat is running, the status will be 0. + status = await wait_for(process.wait(), timeout=1) + if status != 0: + exit(f"Logcat exit status {status}") - # This is a daemon thread, so `finally` won't work. - atexit.register(logcat.kill) - for line in logcat.stdout: - if match := re.fullmatch(r"(\w)/(\w+): (.*)", line): - level, tag, message = match.groups() +async def gradle_task(context): + env = os.environ.copy() + if context.connected: + task_prefix = "connected" + env["ANDROID_SERIAL"] = context.connected + else: + task_prefix = context.managed + + async with async_process( + "./gradlew", + "--console", "plain", + f"{task_prefix}DebugAndroidTest", + "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" + + shlex.join(context.args), + cwd=TESTBED_DIR, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + ) as process: + while line := await process.stdout.readline(): + if context.verbose: + sys.stdout.buffer.write(line) + sys.stdout.flush() + + status = await wait_for(process.wait(), timeout=1) + if status == 0: + exit(0) else: - # If the regex doesn't match, this is probably the second or - # subsequent line of a multi-line message. Python won't produce - # such messages, but other components might. - level, tag, message = None, None, line - - stream = ( - sys.stderr - if level in ["E", "F"] # ERROR and FATAL (aka ASSERT) - else sys.stdout - ) - - # We strip the level/tag indicator from Python's stdout and stderr, to - # simplify automated processing of the output, e.g. a buildbot posting a - # failure notice on a GitHub PR. - # - # Non-Python messages from the app are still worth keeping, as they may - # help explain any problems. - stream.write( - message if tag in ["python.stdout", "python.stderr"] else line - ) - - status = logcat.wait() - if status != 0: - sys.exit(f"Logcat exit status {status}") - - -def run_testbed(context): + exit(f"Gradle exit status {status}") + + +async def run_testbed(context): if not (TESTBED_DIR / "gradlew").exists(): setup_testbed(context) - kwargs = dict(cwd=TESTBED_DIR) + # In --managed mode, Gradle will create a device with an unpredictable name. + # So we save a list of the running devices before starting Gradle, and + # find_device then waits for a new device to appear. + initial_devices = (await list_devices()) if context.managed else None - if context.connected: - task_prefix = "connected" - env = os.environ.copy() - env["ANDROID_SERIAL"] = context.connected - kwargs.update(env=env) - elif context.managed: - task_prefix = context.managed - else: - raise ValueError("no device argument found") - - Thread( - target=logcat_thread, args=(context, list_devices()), daemon=True - ).start() - - run( - [ - "./gradlew", - "--console", "plain", - f"{task_prefix}DebugAndroidTest", - "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" - + shlex.join(context.args), - ], - **kwargs - ) + try: + async with asyncio.TaskGroup() as tg: + tg.create_task(logcat_task(context, initial_devices)) + tg.create_task(gradle_task(context)) + except* MySystemExit as e: + raise SystemExit(*e.exceptions[0].args) from None def main(): @@ -379,6 +455,9 @@ def main(): "setup-testbed", help="Download the testbed Gradle wrapper") test = subcommands.add_parser( "test", help="Run the test suite") + test.add_argument( + "-v", "--verbose", action="store_true", + help="Show Gradle output, and non-Python logcat messages") device_group = test.add_mutually_exclusive_group(required=True) device_group.add_argument( "--connected", metavar="SERIAL", help="Run on a connected device. " @@ -398,7 +477,10 @@ def main(): "clean": clean_all, "setup-testbed": setup_testbed, "test": run_testbed} - dispatch[context.subcommand](context) + + result = dispatch[context.subcommand](context) + if asyncio.iscoroutine(result): + asyncio.run(result) if __name__ == "__main__": From c71f3cfd03eb1094fca954dd970f04c6ffb547a3 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 23 Jul 2024 22:36:01 +0100 Subject: [PATCH 05/34] --managed mode working, and add instructions to README --- Android/README.md | 41 ++++++++++++++++++++++++-------------- Android/android.py | 49 ++++++++++++++++++++++++++++------------------ 2 files changed, 56 insertions(+), 34 deletions(-) diff --git a/Android/README.md b/Android/README.md index f5f463ca116589..5b6c95a5390933 100644 --- a/Android/README.md +++ b/Android/README.md @@ -80,18 +80,29 @@ call. For example, if you want a pydebug build that also caches the results from ## Testing -To run the Python test suite on Android: - -* Install Android Studio, if you don't already have it. -* Follow the instructions in the previous section to build all supported - architectures. -* Run `./android.py setup-testbed` to download the Gradle wrapper. -* Open the `testbed` directory in Android Studio. -* In the *Device Manager* dock, connect a device or start an emulator. - Then select it from the drop-down list in the toolbar. -* Click the "Run" button in the toolbar. -* The testbed app displays nothing on screen while running. To see its output, - open the [Logcat window](https://developer.android.com/studio/debug/logcat). - -To run specific tests, or pass any other arguments to the test suite, edit the -command line in testbed/app/src/main/python/main.py. +Before running the test suite, follow the instructions in the previous section +to build for all supported architectures. + +The test suite can be run in two modes: + +* In `--connected` mode, it's run on a device or emulator you have already + connected to the build machine. For example: + + ```sh + ./android.py test --connected emulator-5554 + ``` + +* In `--managed` mode, it's run on a Gradle-managed device. These are defined in + `managedDevices` in testbed/app/build.gradle.kts. This mode is slower, but + more reproducible. For example: + + ```sh + ./android.py test --managed targetSdk + ``` + +By default, the only messages the script will show are Python's own stdout and +stderr. Add the `-v` option to also show Gradle output, and non-Python logcat +messages. + +Any other arguments on the `android.py test` command line will be passed through +to `python -m test`. Use `--` to separate them from android.py's own options. diff --git a/Android/android.py b/Android/android.py index 4e929edf054f8f..ba54d8c33a63cd 100755 --- a/Android/android.py +++ b/Android/android.py @@ -285,20 +285,33 @@ async def list_devices(): return serials +# Before boot is completed, `pm list` will fail with the error "Can't find +# service: package". +async def boot_completed(serial): + return (await async_check_output( + adb, "-s", serial, "shell", "getprop", "sys.boot_completed" + )).strip() == "1" + + async def find_device(context, initial_devices): - if context.connected: - return context.connected + if context.managed: + serial = None + while serial is None: + new_devices = set(await list_devices()).difference(initial_devices) + if len(new_devices) == 0: + await asyncio.sleep(1) + elif len(new_devices) == 1: + serial = new_devices.pop() + else: + exit(f"Found more than one new device: {new_devices}") + else: + serial = context.connected - while True: - new_devices = set(await list_devices()).difference(initial_devices) - if len(new_devices) == 0: - await asyncio.sleep(1) - elif len(new_devices) == 1: - device = new_devices.pop() - print(f"Found device: {device}") - return device - else: - exit(f"Found more than one new device: {new_devices}") + print(f"Serial: {serial}") + while not await boot_completed(serial): + await asyncio.sleep(1) + print("Boot completed") + return serial async def find_uid(serial): @@ -316,7 +329,7 @@ async def find_uid(serial): raise ValueError(f"failed to parse {lines[0]!r}") package, uid = match.groups() if package == testbed_package: - print(f"Found UID: {uid}") + print(f"UID: {uid}") return uid await asyncio.sleep(1) @@ -330,9 +343,7 @@ async def logcat_task(context, initial_devices): # timestamp or PID. uid = await find_uid(serial) async with async_process( - adb, "-s", serial, "logcat", - "--uid", uid, - "--format", "tag", + adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag", stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: @@ -376,11 +387,11 @@ async def logcat_task(context, initial_devices): async def gradle_task(context): env = os.environ.copy() - if context.connected: + if context.managed: + task_prefix = context.managed + else: task_prefix = "connected" env["ANDROID_SERIAL"] = context.connected - else: - task_prefix = context.managed async with async_process( "./gradlew", From cd27b0fe96c8761ca36a87146262830b251fab02 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Wed, 24 Jul 2024 00:36:44 +0100 Subject: [PATCH 06/34] Clarifications --- Android/README.md | 8 ++++---- .../app/src/main/java/org/python/testbed/MainActivity.kt | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Android/README.md b/Android/README.md index 5b6c95a5390933..a741ff73a7d2ce 100644 --- a/Android/README.md +++ b/Android/README.md @@ -85,16 +85,16 @@ to build for all supported architectures. The test suite can be run in two modes: -* In `--connected` mode, it's run on a device or emulator you have already +* In `--connected` mode, it runs on a device or emulator you have already connected to the build machine. For example: ```sh ./android.py test --connected emulator-5554 ``` -* In `--managed` mode, it's run on a Gradle-managed device. These are defined in - `managedDevices` in testbed/app/build.gradle.kts. This mode is slower, but - more reproducible. For example: +* In `--managed` mode, it uses a temporary emulator defined in the + `managedDevices` section of testbed/app/build.gradle.kts. This mode is slower, + but more reproducible. For example: ```sh ./android.py test --managed targetSdk diff --git a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt index 8e5166f518127d..c9663f87f01619 100644 --- a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt +++ b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt @@ -9,8 +9,8 @@ import java.io.* // Launching the tests from an activity is OK for a quick check, but for -// anything more complicated it'll be more use `android.py test` to launch the -// tests via PythonSuite. +// anything more complicated it'll be more convenient to use `android.py test` +// to launch the tests via PythonSuite. class MainActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) From 6bbdfda366f77e4cbb304dc45a63cc17527798da Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Wed, 24 Jul 2024 00:54:13 +0100 Subject: [PATCH 07/34] Restore build command logging, and only require adb when running tests --- Android/android.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Android/android.py b/Android/android.py index ba54d8c33a63cd..86288f7bc301ee 100755 --- a/Android/android.py +++ b/Android/android.py @@ -32,11 +32,6 @@ f"{android_home}/platform-tools/adb" + (".exe" if os.name == "nt" else "") ) -if not adb.exists(): - sys.exit( - f"{adb} does not exist. Install the Platform Tools package using the " - f"Android SDK manager." - ) def delete_if_exists(path): @@ -90,6 +85,7 @@ def run(command, *, host=None, env=None, **kwargs): raise ValueError(f"Found no variables in {env_script.name} output:\n" + env_output) + print(">", " ".join(map(str, command))) try: subprocess.run(command, check=True, env=env, **kwargs) except subprocess.CalledProcessError as e: @@ -417,6 +413,12 @@ async def gradle_task(context): async def run_testbed(context): + if not adb.exists(): + sys.exit( + f"{adb} does not exist. Install the Platform Tools package using " + f"the Android SDK manager." + ) + if not (TESTBED_DIR / "gradlew").exists(): setup_testbed(context) From e73868974a4cbbb0268e62246c08d1f6e9212437 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 30 Jul 2024 20:32:05 +0100 Subject: [PATCH 08/34] Make testbed include only those ABIs which have been built --- Android/android.py | 25 +++++++++++++++++++------ Android/testbed/app/build.gradle.kts | 9 ++++++++- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/Android/android.py b/Android/android.py index 0a1393e61ddb0e..a78b15c9c4e58c 100755 --- a/Android/android.py +++ b/Android/android.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import argparse +from glob import glob import os import re import shutil @@ -16,16 +17,21 @@ CROSS_BUILD_DIR = CHECKOUT / "cross-build" -def delete_if_exists(path): - if path.exists(): +def delete_glob(pattern): + # Path.glob doesn't accept non-relative patterns. + for path in glob(str(pattern)): + path = Path(path) print(f"Deleting {path} ...") - shutil.rmtree(path) + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path) + else: + path.unlink() def subdir(name, *, clean=None): path = CROSS_BUILD_DIR / name if clean: - delete_if_exists(path) + delete_glob(path) if not path.exists(): if clean is None: sys.exit( @@ -150,10 +156,17 @@ def configure_host_python(context): def make_host_python(context): + # The CFLAGS and LDFLAGS set in android-env include the prefix dir, so + # delete any previously-installed Python libs and include files to prevent + # them being used during the build. host_dir = subdir(context.host) + prefix_dir = host_dir / "prefix" + delete_glob(f"{prefix_dir}/include/python*") + delete_glob(f"{prefix_dir}/lib/libpython*") + os.chdir(host_dir / "build") run(["make", "-j", str(os.cpu_count())], host=context.host) - run(["make", "install", f"prefix={host_dir}/prefix"], host=context.host) + run(["make", "install", f"prefix={prefix_dir}"], host=context.host) def build_all(context): @@ -164,7 +177,7 @@ def build_all(context): def clean_all(context): - delete_if_exists(CROSS_BUILD_DIR) + delete_glob(CROSS_BUILD_DIR) # To avoid distributing compiled artifacts without corresponding source code, diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 7690d3fd86b2fd..7320b21e98bbd1 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -7,10 +7,17 @@ plugins { val PYTHON_DIR = File(projectDir, "../../..").canonicalPath val PYTHON_CROSS_DIR = "$PYTHON_DIR/cross-build" + val ABIS = mapOf( "arm64-v8a" to "aarch64-linux-android", "x86_64" to "x86_64-linux-android", -) +).filter { File("$PYTHON_CROSS_DIR/${it.value}").exists() } +if (ABIS.isEmpty()) { + throw GradleException( + "No Android ABIs found in $PYTHON_CROSS_DIR: see Android/README.md " + + "for building instructions." + ) +} val PYTHON_VERSION = File("$PYTHON_DIR/Include/patchlevel.h").useLines { for (line in it) { From b5198743525a7d698d460b116902cbb5d6c94472 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 1 Aug 2024 00:17:15 +0100 Subject: [PATCH 09/34] Fix Windows issues --- Android/android.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Android/android.py b/Android/android.py index db7c0876908324..07dddb7fcf2c0e 100755 --- a/Android/android.py +++ b/Android/android.py @@ -215,13 +215,13 @@ def setup_testbed(context): os.chmod(out_path, 0o755) with TemporaryDirectory(prefix=SCRIPT_NAME) as temp_dir: - os.chdir(temp_dir) bin_zip = download( - f"https://services.gradle.org/distributions/gradle-{ver_short}-bin.zip") + f"https://services.gradle.org/distributions/gradle-{ver_short}-bin.zip", + temp_dir) outer_jar = f"gradle-{ver_short}/lib/plugins/gradle-wrapper-{ver_short}.jar" - run(["unzip", bin_zip, outer_jar]) - run(["unzip", "-o", "-d", f"{TESTBED_DIR}/gradle/wrapper", outer_jar, - "gradle-wrapper.jar"]) + run(["unzip", "-d", temp_dir, bin_zip, outer_jar]) + run(["unzip", "-o", "-d", f"{TESTBED_DIR}/gradle/wrapper", + f"{temp_dir}/{outer_jar}", "gradle-wrapper.jar"]) # Work around a bug involving sys.exit and TaskGroups @@ -403,7 +403,7 @@ async def gradle_task(context): env["ANDROID_SERIAL"] = context.connected async with async_process( - "./gradlew", + f"{TESTBED_DIR}/gradlew" + (".bat" if os.name == "nt" else ""), "--console", "plain", f"{task_prefix}DebugAndroidTest", "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" From 32f79d7a8074f32fdb7a7f2790a34b9a3476414c Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 8 Aug 2024 22:22:12 +0100 Subject: [PATCH 10/34] Clarify documentation; remove default `python -m test` arguments --- Android/README.md | 19 ++++++++++++------- Android/testbed/app/build.gradle.kts | 15 +++++++++++---- Android/testbed/app/src/main/python/main.py | 5 +---- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/Android/README.md b/Android/README.md index a741ff73a7d2ce..3508b0533284cd 100644 --- a/Android/README.md +++ b/Android/README.md @@ -25,7 +25,7 @@ you don't already have the SDK, here's how to install it: The `android.py` script also requires the following commands to be on the `PATH`: * `curl` -* `java` +* `java` (or set the `JAVA_HOME` environment variable) * `tar` * `unzip` @@ -81,23 +81,28 @@ call. For example, if you want a pydebug build that also caches the results from ## Testing Before running the test suite, follow the instructions in the previous section -to build for all supported architectures. +to build the architecture you want to test. -The test suite can be run in two modes: +The test script can be run in two modes: * In `--connected` mode, it runs on a device or emulator you have already - connected to the build machine. For example: + connected to the build machine. List the available devices with + `$ANDROID_HOME/platform-tools/adb devices -l`, then pass a device ID to the + script like this: ```sh ./android.py test --connected emulator-5554 ``` -* In `--managed` mode, it uses a temporary emulator defined in the +* In `--managed` mode, it uses a temporary headless emulator defined in the `managedDevices` section of testbed/app/build.gradle.kts. This mode is slower, - but more reproducible. For example: + but more reproducible. + + We currently define two devices: `minVersion` and `maxVersion`, corresponding + to our minimum and maximum supported Android versions. For example: ```sh - ./android.py test --managed targetSdk + ./android.py test --managed maxVersion ``` By default, the only messages the script will show are Python's own stdout and diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index c75eccd73daaca..845057b5e786e2 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -1,4 +1,5 @@ import com.android.build.api.variant.* +import kotlin.math.max plugins { id("com.android.application") @@ -68,12 +69,18 @@ android { testOptions { managedDevices { localDevices { - // In the future we may add a "minSdk" device, but managed - // devices have a minimum API level of 27. - create("targetSdk") { + create("minVersion") { device = "Small Phone" - apiLevel = defaultConfig.targetSdk!! systemImageSource = "aosp-atd" + + // Managed devices have a minimum API level of 27. + apiLevel = max(27, defaultConfig.minSdk!!) + } + + create("maxVersion") { + device = "Small Phone" + systemImageSource = "aosp-atd" + apiLevel = defaultConfig.targetSdk!! } } } diff --git a/Android/testbed/app/src/main/python/main.py b/Android/testbed/app/src/main/python/main.py index b9fd0f126f22a6..c7314b500bf821 100644 --- a/Android/testbed/app/src/main/python/main.py +++ b/Android/testbed/app/src/main/python/main.py @@ -10,10 +10,7 @@ # profile save"), so disabling it should not weaken the tests. signal.pthread_sigmask(signal.SIG_UNBLOCK, [signal.SIGUSR1]) -sys.argv[1:] = [ - "-uall", # Enable all resources - "-W", # Display test output on failure -] + shlex.split(os.environ["PYTHON_ARGS"]) +sys.argv[1:] = shlex.split(os.environ["PYTHON_ARGS"]) # The test module will call sys.exit to indicate whether the tests passed. runpy.run_module("test") From 6471512ee146e36cf3ecf77832955fbc7e9cf3b8 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 8 Aug 2024 22:44:29 +0100 Subject: [PATCH 11/34] Link to page about test options; add newlines at end of files --- Android/README.md | 6 +++++- .../src/androidTest/java/org/python/testbed/PythonSuite.kt | 2 +- .../app/src/main/java/org/python/testbed/MainActivity.kt | 2 +- Android/testbed/build.gradle.kts | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Android/README.md b/Android/README.md index 3508b0533284cd..cb774f7684825a 100644 --- a/Android/README.md +++ b/Android/README.md @@ -110,4 +110,8 @@ stderr. Add the `-v` option to also show Gradle output, and non-Python logcat messages. Any other arguments on the `android.py test` command line will be passed through -to `python -m test`. Use `--` to separate them from android.py's own options. +to `python -m test` – use `--` to separate them from android.py's own options. +See the [Python Developer's +Guide](https://devguide.python.org/testing/run-write-tests/) for common options +– most of them will work on Android, except for those that involve subprocesses, +such as `-j`. diff --git a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt index 50ecf393dfab6e..5afb043eacb772 100644 --- a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt +++ b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt @@ -20,4 +20,4 @@ class PythonSuite { val status = PythonTestRunner(context).run(args) assertEquals(0, status) } -} \ No newline at end of file +} diff --git a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt index c9663f87f01619..846e3ccb9ddab8 100644 --- a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt +++ b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt @@ -76,4 +76,4 @@ class PythonTestRunner(val context: Context) { private external fun redirectStdioToLogcat() private external fun runPython(home: String, runModule: String) : Int -} \ No newline at end of file +} diff --git a/Android/testbed/build.gradle.kts b/Android/testbed/build.gradle.kts index 34884f724412ea..2dad1501c2422f 100644 --- a/Android/testbed/build.gradle.kts +++ b/Android/testbed/build.gradle.kts @@ -2,4 +2,4 @@ plugins { id("com.android.application") version "8.4.2" apply false id("org.jetbrains.kotlin.android") version "1.9.22" apply false -} \ No newline at end of file +} From 5c3967ef2170bc1c2bc1a2ebd16ced95bc1cee5b Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 8 Aug 2024 23:23:15 +0100 Subject: [PATCH 12/34] Add `android.py build-testbed` command --- Android/android.py | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/Android/android.py b/Android/android.py index 07dddb7fcf2c0e..f00019c4726256 100755 --- a/Android/android.py +++ b/Android/android.py @@ -34,6 +34,11 @@ + (".exe" if os.name == "nt" else "") ) +gradlew = Path( + f"{TESTBED_DIR}/gradlew" + + (".bat" if os.name == "nt" else "") +) + def delete_glob(pattern): # Path.glob doesn't accept non-relative patterns. @@ -224,6 +229,21 @@ def setup_testbed(context): f"{temp_dir}/{outer_jar}", "gradle-wrapper.jar"]) +def setup_testbed_if_needed(context): + if not gradlew.exists(): + setup_testbed(context) + + +# run_testbed will build the app automatically, but it hides the Gradle output +# by default, so it's useful to have this as a separate command for the buildbot. +def build_testbed(context): + setup_testbed_if_needed(context) + run( + [gradlew, "--console", "plain", "packageDebug", "packageDebugAndroidTest"], + cwd=TESTBED_DIR, + ) + + # Work around a bug involving sys.exit and TaskGroups # (https://github.com/python/cpython/issues/101515). def exit(*args): @@ -403,8 +423,7 @@ async def gradle_task(context): env["ANDROID_SERIAL"] = context.connected async with async_process( - f"{TESTBED_DIR}/gradlew" + (".bat" if os.name == "nt" else ""), - "--console", "plain", + gradlew, "--console", "plain", f"{task_prefix}DebugAndroidTest", "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" + shlex.join(context.args), @@ -432,8 +451,7 @@ async def run_testbed(context): f"the Android SDK manager." ) - if not (TESTBED_DIR / "gradlew").exists(): - setup_testbed(context) + setup_testbed_if_needed(context) # In --managed mode, Gradle will create a device with an unpredictable name. # So we save a list of the running devices before starting Gradle, and @@ -479,6 +497,8 @@ def main(): subcommands.add_parser( "setup-testbed", help="Download the testbed Gradle wrapper") + subcommands.add_parser( + "build-testbed", help="Build the testbed app") test = subcommands.add_parser( "test", help="Run the test suite") test.add_argument( @@ -502,6 +522,7 @@ def main(): "build": build_all, "clean": clean_all, "setup-testbed": setup_testbed, + "build-testbed": build_testbed, "test": run_testbed} result = dispatch[context.subcommand](context) From cb60cc44d01350fa1fa84b4fd7538e76c4455b74 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sun, 11 Aug 2024 20:02:54 +0100 Subject: [PATCH 13/34] If test script fails before logcat starts, show the Gradle output even in non-verbose mose --- Android/android.py | 86 ++++++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/Android/android.py b/Android/android.py index f00019c4726256..89c2cd438be9d5 100755 --- a/Android/android.py +++ b/Android/android.py @@ -22,6 +22,7 @@ ANDROID_DIR = CHECKOUT / "Android" TESTBED_DIR = ANDROID_DIR / "testbed" CROSS_BUILD_DIR = CHECKOUT / "cross-build" +DECODE_ARGS = ("UTF-8", "backslashreplace") try: @@ -39,6 +40,8 @@ + (".bat" if os.name == "nt" else "") ) +logcat_started = False + def delete_glob(pattern): # Path.glob doesn't accept non-relative patterns. @@ -271,21 +274,28 @@ async def async_process(*args, **kwargs): await wait_for(process.wait(), timeout=1) +def exit_status_error(args, status, *, prefix=""): + if prefix and not prefix.endswith("\n"): + prefix += "\n" + + # Format the command so it can be copied into a shell. + args_joined = " ".join(shlex.quote(str(arg)) for arg in args) + exit( + f"{prefix}Command '{args_joined}' returned exit status {status}" + ) + + async def async_check_output(*args, **kwargs): async with async_process( *args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs ) as process: stdout, stderr = await process.communicate() if process.returncode == 0: - return stdout.decode("UTF-8", "backslashreplace") + return stdout.decode(*DECODE_ARGS) else: - sys.stdout.buffer.write(stdout) - sys.stdout.flush() - sys.stderr.buffer.write(stderr) - sys.stderr.flush() - exit( - f"Command {args} returned non-zero exit status " - f"{process.returncode}" + exit_status_error( + args, process.returncode, + prefix=(stdout + stderr).decode(*DECODE_ARGS) ) @@ -371,14 +381,15 @@ async def logcat_task(context, initial_devices): # be different every time. There's therefore no need to filter the logs by # timestamp or PID. uid = await find_uid(serial) + + args = [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"] async with async_process( - adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag", - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: - while line := (await process.stdout.readline()).decode( - "UTF-8", "backslashreplace" - ): + while line := (await process.stdout.readline()).decode(*DECODE_ARGS): + global logcat_started + logcat_started = True + if match := re.fullmatch(r"([A-Z])/(.*)", line, re.DOTALL): level, message = match.groups() else: @@ -411,7 +422,7 @@ async def logcat_task(context, initial_devices): # If the device disconnects while logcat is running, the status will be 0. status = await wait_for(process.wait(), timeout=1) if status != 0: - exit(f"Logcat exit status {status}") + exit_status_error(args, status) async def gradle_task(context): @@ -422,26 +433,35 @@ async def gradle_task(context): task_prefix = "connected" env["ANDROID_SERIAL"] = context.connected - async with async_process( - gradlew, "--console", "plain", - f"{task_prefix}DebugAndroidTest", + args = [ + gradlew, "--console", "plain", f"{task_prefix}DebugAndroidTest", "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" + shlex.join(context.args), - cwd=TESTBED_DIR, - env=env, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - ) as process: - while line := await process.stdout.readline(): - if context.verbose: - sys.stdout.buffer.write(line) - sys.stdout.flush() - - status = await wait_for(process.wait(), timeout=1) - if status == 0: - exit(0) - else: - exit(f"Gradle exit status {status}") + ] + gradle_output = [] + try: + async with async_process( + *args, cwd=TESTBED_DIR, env=env, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + ) as process: + while line := await process.stdout.readline(): + if context.verbose: + sys.stdout.buffer.write(line) + sys.stdout.flush() + else: + gradle_output.append(line) + + status = await wait_for(process.wait(), timeout=1) + if status == 0: + exit(0) + else: + exit_status_error(args, status) + finally: + # If we failed before logcat started, then the user probably wants to + # see the Gradle output even in non-verbose mode. + if gradle_output and not logcat_started: + sys.stdout.buffer.write(b"".join(gradle_output)) + sys.stdout.flush() async def run_testbed(context): From b075842eb6369ce7292a94e5114980fdeb093093 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sun, 11 Aug 2024 20:24:30 +0100 Subject: [PATCH 14/34] Fix logcat error messages being hidden --- Android/android.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Android/android.py b/Android/android.py index 89c2cd438be9d5..661ba69345a4e0 100755 --- a/Android/android.py +++ b/Android/android.py @@ -383,6 +383,7 @@ async def logcat_task(context, initial_devices): uid = await find_uid(serial) args = [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"] + hidden_output = [] async with async_process( *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: @@ -418,11 +419,13 @@ async def logcat_task(context, initial_devices): # Non-Python messages add a lot of noise, but they may # sometimes help explain a failure. stream.write(line) + else: + hidden_output.append(line) # If the device disconnects while logcat is running, the status will be 0. status = await wait_for(process.wait(), timeout=1) if status != 0: - exit_status_error(args, status) + exit_status_error(args, status, prefix="".join(hidden_output)) async def gradle_task(context): @@ -438,18 +441,17 @@ async def gradle_task(context): "-Pandroid.testInstrumentationRunnerArguments.pythonArgs=" + shlex.join(context.args), ] - gradle_output = [] + hidden_output = [] try: async with async_process( *args, cwd=TESTBED_DIR, env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: - while line := await process.stdout.readline(): + while line := (await process.stdout.readline()).decode(*DECODE_ARGS): if context.verbose: - sys.stdout.buffer.write(line) - sys.stdout.flush() + sys.stdout.write(line) else: - gradle_output.append(line) + hidden_output.append(line) status = await wait_for(process.wait(), timeout=1) if status == 0: @@ -459,9 +461,8 @@ async def gradle_task(context): finally: # If we failed before logcat started, then the user probably wants to # see the Gradle output even in non-verbose mode. - if gradle_output and not logcat_started: - sys.stdout.buffer.write(b"".join(gradle_output)) - sys.stdout.flush() + if hidden_output and not logcat_started: + sys.stdout.write("".join(hidden_output)) async def run_testbed(context): From 2266e828ab932a295207ab50d801aa95da5a1625 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sun, 11 Aug 2024 21:18:34 +0100 Subject: [PATCH 15/34] Improve logging, add timeouts --- Android/android.py | 47 ++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/Android/android.py b/Android/android.py index 661ba69345a4e0..040434f253e1c9 100755 --- a/Android/android.py +++ b/Android/android.py @@ -324,36 +324,42 @@ async def list_devices(): return serials -# Before boot is completed, `pm list` will fail with the error "Can't find -# service: package". -async def boot_completed(serial): - return (await async_check_output( - adb, "-s", serial, "shell", "getprop", "sys.boot_completed" - )).strip() == "1" - - async def find_device(context, initial_devices): if context.managed: - serial = None - while serial is None: + print("Waiting for managed device - this may take several minutes") + while True: new_devices = set(await list_devices()).difference(initial_devices) if len(new_devices) == 0: await asyncio.sleep(1) elif len(new_devices) == 1: serial = new_devices.pop() + print(f"Serial: {serial}") + return serial else: exit(f"Found more than one new device: {new_devices}") else: - serial = context.connected + return context.connected - print(f"Serial: {serial}") - while not await boot_completed(serial): - await asyncio.sleep(1) - print("Boot completed") - return serial + +# Before boot is completed, `pm list` will fail with the error "Can't find +# service: package". +async def boot_completed(serial): + # No need to show a message if the device is already booted. + shown_message = False + while True: + if (await async_check_output( + adb, "-s", serial, "shell", "getprop", "sys.boot_completed" + )).strip() == "1": + return + else: + if not shown_message: + print("Waiting for boot") + shown_message = True + await asyncio.sleep(1) async def find_uid(serial): + print("Waiting for app to start - this may take several minutes") testbed_package = "org.python.testbed" while True: lines = (await async_check_output( @@ -375,12 +381,17 @@ async def find_uid(serial): async def logcat_task(context, initial_devices): - serial = await find_device(context, initial_devices) + # Gradle may need to do some large downloads of libraries and emulator + # images. This will happen during find_device in --managed mode, or find_uid + # in --connected mode. + startup_timeout = 600 + serial = await wait_for(find_device(context, initial_devices), startup_timeout) + await wait_for(boot_completed(serial), startup_timeout) # Because Gradle uninstalls the app after running the tests, its UID should # be different every time. There's therefore no need to filter the logs by # timestamp or PID. - uid = await find_uid(serial) + uid = await wait_for(find_uid(serial), startup_timeout) args = [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"] hidden_output = [] From 397d20b5d0f6f5c87af34aec5383f83a8c878a1c Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sun, 11 Aug 2024 21:56:59 +0100 Subject: [PATCH 16/34] Make the app use the same NDK version as the Python build --- Android/android-env.sh | 2 +- Android/testbed/app/build.gradle.kts | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/Android/android-env.sh b/Android/android-env.sh index 545d559d93ab36..93372e3fe1c7ee 100644 --- a/Android/android-env.sh +++ b/Android/android-env.sh @@ -28,7 +28,7 @@ ndk_version=26.2.11394342 ndk=$ANDROID_HOME/ndk/$ndk_version if ! [ -e $ndk ]; then - log "Installing NDK: this may take several minutes" + log "Installing NDK - this may take several minutes" yes | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager "ndk;$ndk_version" fi diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 845057b5e786e2..f02c7c6f42563b 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -6,13 +6,13 @@ plugins { id("org.jetbrains.kotlin.android") } -val PYTHON_DIR = File(projectDir, "../../..").canonicalPath +val PYTHON_DIR = file("../../..").canonicalPath val PYTHON_CROSS_DIR = "$PYTHON_DIR/cross-build" val ABIS = mapOf( "arm64-v8a" to "aarch64-linux-android", "x86_64" to "x86_64-linux-android", -).filter { File("$PYTHON_CROSS_DIR/${it.value}").exists() } +).filter { file("$PYTHON_CROSS_DIR/${it.value}").exists() } if (ABIS.isEmpty()) { throw GradleException( "No Android ABIs found in $PYTHON_CROSS_DIR: see Android/README.md " + @@ -20,7 +20,7 @@ if (ABIS.isEmpty()) { ) } -val PYTHON_VERSION = File("$PYTHON_DIR/Include/patchlevel.h").useLines { +val PYTHON_VERSION = file("$PYTHON_DIR/Include/patchlevel.h").useLines { for (line in it) { val match = """#define PY_VERSION\s+"(\d+\.\d+)""".toRegex().find(line) if (match != null) { @@ -30,6 +30,16 @@ val PYTHON_VERSION = File("$PYTHON_DIR/Include/patchlevel.h").useLines { throw GradleException("Failed to find Python version") } +android.ndkVersion = file("../../android-env.sh").useLines { + for (line in it) { + val match = """ndk_version=(\S+)""".toRegex().find(line) + if (match != null) { + return@useLines match.groupValues[1] + } + } + throw GradleException("Failed to find NDK version") +} + android { namespace = "org.python.testbed" From 8e80dd2acdeab677dada66504300939e083057cb Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sun, 11 Aug 2024 22:22:14 +0100 Subject: [PATCH 17/34] Install platform-tools automatically --- Android/android.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Android/android.py b/Android/android.py index 040434f253e1c9..f175d7310ae669 100755 --- a/Android/android.py +++ b/Android/android.py @@ -30,6 +30,11 @@ except KeyError: sys.exit("The ANDROID_HOME environment variable is required.") +sdkmanager = Path( + f"{android_home}/cmdline-tools/latest/bin/sdkmanager" + + (".bat" if os.name == "nt" else "") +) + adb = Path( f"{android_home}/platform-tools/adb" + (".exe" if os.name == "nt" else "") @@ -478,10 +483,9 @@ async def gradle_task(context): async def run_testbed(context): if not adb.exists(): - sys.exit( - f"{adb} does not exist. Install the Platform Tools package using " - f"the Android SDK manager." - ) + print("Installing Platform-Tools") + # Input "y" to accept licenses. + run([sdkmanager, "platform-tools"], text=True, input="y\n" * 100) setup_testbed_if_needed(context) From 709061e4b9c2924158ec20d2c9f4a021de58cf1d Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 01:19:46 +0100 Subject: [PATCH 18/34] In --connected mode, uninstall app before running Gradle --- Android/android.py | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/Android/android.py b/Android/android.py index f175d7310ae669..bbbb66d52ebfe2 100755 --- a/Android/android.py +++ b/Android/android.py @@ -14,6 +14,7 @@ from contextlib import asynccontextmanager from os.path import basename, relpath from pathlib import Path +from subprocess import DEVNULL from tempfile import TemporaryDirectory @@ -22,6 +23,8 @@ ANDROID_DIR = CHECKOUT / "Android" TESTBED_DIR = ANDROID_DIR / "testbed" CROSS_BUILD_DIR = CHECKOUT / "cross-build" + +APP_ID = "org.python.testbed" DECODE_ARGS = ("UTF-8", "backslashreplace") @@ -73,7 +76,8 @@ def subdir(name, *, clean=None): return path -def run(command, *, host=None, env=None, **kwargs): +def run(command, *, host=None, env=None, log=True, **kwargs): + kwargs.setdefault("check", True) if env is None: env = os.environ.copy() original_env = env.copy() @@ -104,9 +108,10 @@ def run(command, *, host=None, env=None, **kwargs): raise ValueError(f"Found no variables in {env_script.name} output:\n" + env_output) - print(">", " ".join(map(str, command))) + if log: + print(">", " ".join(map(str, command))) try: - subprocess.run(command, check=True, env=env, **kwargs) + subprocess.run(command, env=env, **kwargs) except subprocess.CalledProcessError as e: sys.exit(e) @@ -286,7 +291,7 @@ def exit_status_error(args, status, *, prefix=""): # Format the command so it can be copied into a shell. args_joined = " ".join(shlex.quote(str(arg)) for arg in args) exit( - f"{prefix}Command '{args_joined}' returned exit status {status}" + f"{prefix}Command '{args_joined}' returned non-zero exit status {status}" ) @@ -365,20 +370,18 @@ async def boot_completed(serial): async def find_uid(serial): print("Waiting for app to start - this may take several minutes") - testbed_package = "org.python.testbed" while True: lines = (await async_check_output( - adb, "-s", serial, "shell", - "pm", "list", "packages", "-U", testbed_package + adb, "-s", serial, "shell", "pm", "list", "packages", "-U", APP_ID )).splitlines() - # The unit test package org.python.testbed.test will also be returned. + # The unit test package {APP_ID}.test will also be returned. for line in lines: match = re.fullmatch(r"package:(\S+) uid:(\d+)", lines[0]) if not match: raise ValueError(f"failed to parse {lines[0]!r}") package, uid = match.groups() - if package == testbed_package: + if package == APP_ID: print(f"UID: {uid}") return uid @@ -489,10 +492,21 @@ async def run_testbed(context): setup_testbed_if_needed(context) - # In --managed mode, Gradle will create a device with an unpredictable name. - # So we save a list of the running devices before starting Gradle, and - # find_device then waits for a new device to appear. - initial_devices = (await list_devices()) if context.managed else None + if context.managed: + # In this mode, Gradle will create a device with an unpredictable name. + # So we save a list of the running devices before starting Gradle, and + # find_device then waits for a new device to appear. + initial_devices = await list_devices() + else: + # Gradle normally uninstalls the app after the tests finish, but let's + # make certain, otherwise we might show logs from a previous run. This + # is unnecessary in --managed mode, because Gradle creates a new + # emulator every time. + run( + [adb, "-s", context.connected, "uninstall", APP_ID], + log=False, stdout=DEVNULL, stderr=DEVNULL, check=False + ) + initial_devices = None try: async with asyncio.TaskGroup() as tg: From 756bc1318dfd84faaa3d77edb7732d1401def577 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 01:39:19 +0100 Subject: [PATCH 19/34] Handle adb logcat returning failure when device disconnects --- Android/android.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Android/android.py b/Android/android.py index bbbb66d52ebfe2..b5ea8bffd49b18 100755 --- a/Android/android.py +++ b/Android/android.py @@ -407,9 +407,6 @@ async def logcat_task(context, initial_devices): *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: while line := (await process.stdout.readline()).decode(*DECODE_ARGS): - global logcat_started - logcat_started = True - if match := re.fullmatch(r"([A-Z])/(.*)", line, re.DOTALL): level, message = match.groups() else: @@ -431,6 +428,8 @@ async def logcat_task(context, initial_devices): # tag indicators from Python's stdout and stderr. for prefix in ["python.stdout: ", "python.stderr: "]: if message.startswith(prefix): + global logcat_started + logcat_started = True stream.write(message.removeprefix(prefix)) break else: @@ -441,9 +440,12 @@ async def logcat_task(context, initial_devices): else: hidden_output.append(line) - # If the device disconnects while logcat is running, the status will be 0. + # If the device disconnects while logcat is running, which always + # happens in --managed mode, some versions of adb return failure. + # Distinguish this from a logcat startup error by checking whether we've + # received a message from Python yet. status = await wait_for(process.wait(), timeout=1) - if status != 0: + if status != 0 and not logcat_started: exit_status_error(args, status, prefix="".join(hidden_output)) From 6b2ed6c28716be286f57413fc93ef9d667b4c0e5 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 02:45:43 +0100 Subject: [PATCH 20/34] Filter logs by PID rather than UID --- Android/android.py | 41 +++++++++---------- Android/testbed/app/build.gradle.kts | 7 +++- .../java/org/python/testbed/PythonSuite.kt | 20 +++++++-- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/Android/android.py b/Android/android.py index b5ea8bffd49b18..d8e6b7644348aa 100755 --- a/Android/android.py +++ b/Android/android.py @@ -368,40 +368,39 @@ async def boot_completed(serial): await asyncio.sleep(1) -async def find_uid(serial): +# An older version of this script in #121595 filtered the logs by UID instead, +# which is more reliable since the PID is shorter-lived. But logcat can't filter +# by UID until API level 31. +async def find_pid(serial): print("Waiting for app to start - this may take several minutes") while True: - lines = (await async_check_output( - adb, "-s", serial, "shell", "pm", "list", "packages", "-U", APP_ID - )).splitlines() + try: + pid = (await async_check_output( + adb, "-s", serial, "shell", "pidof", "-s", APP_ID + )).strip() + except MySystemExit: + pid = None - # The unit test package {APP_ID}.test will also be returned. - for line in lines: - match = re.fullmatch(r"package:(\S+) uid:(\d+)", lines[0]) - if not match: - raise ValueError(f"failed to parse {lines[0]!r}") - package, uid = match.groups() - if package == APP_ID: - print(f"UID: {uid}") - return uid + # Exit status is unreliable: some devices (e.g. Nexus 4) return 0 even + # when no process was found. + if pid: + print(f"PID: {pid}") + return pid - await asyncio.sleep(1) + # Loop fairly rapidly to avoid missing a short-lived process. + await asyncio.sleep(0.2) async def logcat_task(context, initial_devices): # Gradle may need to do some large downloads of libraries and emulator - # images. This will happen during find_device in --managed mode, or find_uid + # images. This will happen during find_device in --managed mode, or find_pid # in --connected mode. startup_timeout = 600 serial = await wait_for(find_device(context, initial_devices), startup_timeout) await wait_for(boot_completed(serial), startup_timeout) + pid = await wait_for(find_pid(serial), startup_timeout) - # Because Gradle uninstalls the app after running the tests, its UID should - # be different every time. There's therefore no need to filter the logs by - # timestamp or PID. - uid = await wait_for(find_uid(serial), startup_timeout) - - args = [adb, "-s", serial, "logcat", "--uid", uid, "--format", "tag"] + args = [adb, "-s", serial, "logcat", "--pid", pid, "--format", "tag"] hidden_output = [] async with async_process( *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index f02c7c6f42563b..1ab40c0416e68a 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -81,16 +81,19 @@ android { localDevices { create("minVersion") { device = "Small Phone" - systemImageSource = "aosp-atd" // Managed devices have a minimum API level of 27. apiLevel = max(27, defaultConfig.minSdk!!) + + // ATD devices are smaller and faster, but have a minimum + // API level of 30. + systemImageSource = if (apiLevel >= 30) "aosp-atd" else "aosp" } create("maxVersion") { device = "Small Phone" - systemImageSource = "aosp-atd" apiLevel = defaultConfig.targetSdk!! + systemImageSource = "aosp-atd" } } } diff --git a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt index 5afb043eacb772..f62ccefb49dbca 100644 --- a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt +++ b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt @@ -15,9 +15,21 @@ class PythonSuite { @Test @UiThreadTest fun testPython() { - val context = InstrumentationRegistry.getInstrumentation().targetContext - val args = InstrumentationRegistry.getArguments().getString("pythonArgs", "") - val status = PythonTestRunner(context).run(args) - assertEquals(0, status) + val start = System.currentTimeMillis() + try { + val context = + InstrumentationRegistry.getInstrumentation().targetContext + val args = + InstrumentationRegistry.getArguments().getString("pythonArgs", "") + val status = PythonTestRunner(context).run(args) + assertEquals(0, status) + } finally { + // Make sure the process lives long enough for the test script to + // detect it. + val delay = 2000 - (System.currentTimeMillis() - start) + if (delay > 0) { + Thread.sleep(delay) + } + } } } From 05a26cc03fee4c788358791b80e4deb7e633118a Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 14:55:27 +0100 Subject: [PATCH 21/34] Try to terminate subprocesses with SIGTERM before sending SIGKILL --- Android/android.py | 96 +++++++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 39 deletions(-) diff --git a/Android/android.py b/Android/android.py index d8e6b7644348aa..70bc3b78b7c50b 100755 --- a/Android/android.py +++ b/Android/android.py @@ -14,7 +14,7 @@ from contextlib import asynccontextmanager from os.path import basename, relpath from pathlib import Path -from subprocess import DEVNULL +from subprocess import CalledProcessError, DEVNULL from tempfile import TemporaryDirectory @@ -110,10 +110,7 @@ def run(command, *, host=None, env=None, log=True, **kwargs): if log: print(">", " ".join(map(str, command))) - try: - subprocess.run(command, env=env, **kwargs) - except subprocess.CalledProcessError as e: - sys.exit(e) + return subprocess.run(command, env=env, **kwargs) def build_python_path(): @@ -277,22 +274,22 @@ async def async_process(*args, **kwargs): yield process finally: if process.returncode is None: - process.kill() - - # Even after killing the process we must still wait for it, - # otherwise we'll get "Exception ignored in __del__" messages. - await wait_for(process.wait(), timeout=1) - - -def exit_status_error(args, status, *, prefix=""): - if prefix and not prefix.endswith("\n"): - prefix += "\n" + # Allow a reasonably long time for Gradle to clean itself up, + # because we don't want stale emulators left behind. + timeout = 10 + process.terminate() + try: + await wait_for(process.wait(), timeout) + except TimeoutError: + print( + f"Command {args} did not terminate after {timeout} seconds " + f" - sending SIGKILL" + ) + process.kill() - # Format the command so it can be copied into a shell. - args_joined = " ".join(shlex.quote(str(arg)) for arg in args) - exit( - f"{prefix}Command '{args_joined}' returned non-zero exit status {status}" - ) + # Even after killing the process we must still wait for it, + # otherwise we'll get the warning "Exception ignored in __del__". + await wait_for(process.wait(), timeout=1) async def async_check_output(*args, **kwargs): @@ -303,9 +300,9 @@ async def async_check_output(*args, **kwargs): if process.returncode == 0: return stdout.decode(*DECODE_ARGS) else: - exit_status_error( - args, process.returncode, - prefix=(stdout + stderr).decode(*DECODE_ARGS) + raise CalledProcessError( + process.returncode, args, + stdout.decode(*DECODE_ARGS), stderr.decode(*DECODE_ARGS) ) @@ -378,14 +375,17 @@ async def find_pid(serial): pid = (await async_check_output( adb, "-s", serial, "shell", "pidof", "-s", APP_ID )).strip() - except MySystemExit: - pid = None - - # Exit status is unreliable: some devices (e.g. Nexus 4) return 0 even - # when no process was found. - if pid: - print(f"PID: {pid}") - return pid + except CalledProcessError as e: + # If the app isn't running yet, pidof gives no output, so if there + # is output, there must have been some other error. + if e.stdout or e.stderr: + raise + else: + # Some older devices (e.g. Nexus 4) return zero even when no process + # was found, so check whether we actually got any output. + if pid: + print(f"PID: {pid}") + return pid # Loop fairly rapidly to avoid missing a short-lived process. await asyncio.sleep(0.2) @@ -440,12 +440,12 @@ async def logcat_task(context, initial_devices): hidden_output.append(line) # If the device disconnects while logcat is running, which always - # happens in --managed mode, some versions of adb return failure. + # happens in --managed mode, some versions of adb return non-zero. # Distinguish this from a logcat startup error by checking whether we've # received a message from Python yet. status = await wait_for(process.wait(), timeout=1) if status != 0 and not logcat_started: - exit_status_error(args, status, prefix="".join(hidden_output)) + raise CalledProcessError(status, args, "".join(hidden_output)) async def gradle_task(context): @@ -477,10 +477,10 @@ async def gradle_task(context): if status == 0: exit(0) else: - exit_status_error(args, status) + raise CalledProcessError(status, args) finally: - # If we failed before logcat started, then the user probably wants to - # see the Gradle output even in non-verbose mode. + # If logcat never started, then something has gone badly wrong, so the + # user probably wants to see the Gradle output even in non-verbose mode. if hidden_output and not logcat_started: sys.stdout.write("".join(hidden_output)) @@ -515,6 +515,9 @@ async def run_testbed(context): tg.create_task(gradle_task(context)) except* MySystemExit as e: raise SystemExit(*e.exceptions[0].args) from None + except* CalledProcessError as e: + # Extract it from the ExceptionGroup so it can be handled by `main`. + raise e.exceptions[0] def main(): @@ -576,9 +579,24 @@ def main(): "build-testbed": build_testbed, "test": run_testbed} - result = dispatch[context.subcommand](context) - if asyncio.iscoroutine(result): - asyncio.run(result) + try: + result = dispatch[context.subcommand](context) + if asyncio.iscoroutine(result): + asyncio.run(result) + except CalledProcessError as e: + for stream_name in ["stdout", "stderr"]: + content = getattr(e, stream_name) + stream = getattr(sys, stream_name) + if content: + stream.write(content) + if not content.endswith("\n"): + stream.write("\n") + + # Format the command so it can be copied into a shell. + args_joined = " ".join(shlex.quote(str(arg)) for arg in e.cmd) + sys.exit( + f"Command '{args_joined}' returned exit status {e.returncode}" + ) if __name__ == "__main__": From a0dd03b0a24c717b002df9479ed6dac325830b87 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 17:46:44 +0100 Subject: [PATCH 22/34] Handle SIGTERM the same way as SIGINT --- Android/android.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/Android/android.py b/Android/android.py index 70bc3b78b7c50b..d0df429e90df7f 100755 --- a/Android/android.py +++ b/Android/android.py @@ -7,6 +7,7 @@ import re import shlex import shutil +import signal import subprocess import sys import sysconfig @@ -520,7 +521,16 @@ async def run_testbed(context): raise e.exceptions[0] -def main(): +# Handle SIGTERM the same way as SIGINT. This ensures that if we're terminated +# by the buildbot worker, we'll make an attempt to clean up our subprocesses. +def install_signal_handler(): + def signal_handler(*args): + os.kill(os.getpid(), signal.SIGINT) + + signal.signal(signal.SIGTERM, signal_handler) + + +def parse_args(): parser = argparse.ArgumentParser() subcommands = parser.add_subparsers(dest="subcommand") build = subcommands.add_parser("build", help="Build everything") @@ -568,7 +578,12 @@ def main(): test.add_argument( "args", nargs="*", help="Extra arguments for `python -m test`") - context = parser.parse_args() + return parser.parse_args() + + +def main(): + install_signal_handler() + context = parse_args() dispatch = {"configure-build": configure_build_python, "make-build": make_build_python, "configure-host": configure_host_python, From 2d53549e49ee83897ed464404b3a83f5fc241ddd Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 18:48:02 +0100 Subject: [PATCH 23/34] Fix race condition with pre-run uninstall --- Android/android.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/Android/android.py b/Android/android.py index d0df429e90df7f..12f81e695ef5e2 100755 --- a/Android/android.py +++ b/Android/android.py @@ -17,6 +17,7 @@ from pathlib import Path from subprocess import CalledProcessError, DEVNULL from tempfile import TemporaryDirectory +from time import sleep SCRIPT_NAME = Path(__file__).name @@ -504,10 +505,21 @@ async def run_testbed(context): # make certain, otherwise we might show logs from a previous run. This # is unnecessary in --managed mode, because Gradle creates a new # emulator every time. - run( - [adb, "-s", context.connected, "uninstall", APP_ID], - log=False, stdout=DEVNULL, stderr=DEVNULL, check=False - ) + removed = False + for package in [f"{APP_ID}.test", APP_ID]: + status = run( + [adb, "-s", context.connected, "uninstall", package], + log=False, stdout=DEVNULL, stderr=DEVNULL, check=False + ).returncode + if status == 0: + removed = True + + # There appears to be a race condition where if you uninstall and then + # immediately reinstall, the next run may fail with no output. + if removed: + print("Waiting for uninstall of old version") + sleep(10) # 5 seconds is not enough. + initial_devices = None try: From c9d5bf59b4180ce0ea48691c8496de8d6a46487e Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 19:33:24 +0100 Subject: [PATCH 24/34] Fix race condition in a more efficient way --- Android/android.py | 32 +++++++++++++------------------ Android/testbed/gradle.properties | 7 ++++++- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/Android/android.py b/Android/android.py index 12f81e695ef5e2..6aa799a1357a9d 100755 --- a/Android/android.py +++ b/Android/android.py @@ -450,6 +450,10 @@ async def logcat_task(context, initial_devices): raise CalledProcessError(status, args, "".join(hidden_output)) +def stop_app(serial): + run([adb, "-s", serial, "shell", "am", "force-stop", APP_ID], log=False) + + async def gradle_task(context): env = os.environ.copy() if context.managed: @@ -486,6 +490,10 @@ async def gradle_task(context): if hidden_output and not logcat_started: sys.stdout.write("".join(hidden_output)) + # Gradle does not stop the tests when interrupted. + if context.connected: + stop_app(context.connected) + async def run_testbed(context): if not adb.exists(): @@ -501,25 +509,11 @@ async def run_testbed(context): # find_device then waits for a new device to appear. initial_devices = await list_devices() else: - # Gradle normally uninstalls the app after the tests finish, but let's - # make certain, otherwise we might show logs from a previous run. This - # is unnecessary in --managed mode, because Gradle creates a new - # emulator every time. - removed = False - for package in [f"{APP_ID}.test", APP_ID]: - status = run( - [adb, "-s", context.connected, "uninstall", package], - log=False, stdout=DEVNULL, stderr=DEVNULL, check=False - ).returncode - if status == 0: - removed = True - - # There appears to be a race condition where if you uninstall and then - # immediately reinstall, the next run may fail with no output. - if removed: - print("Waiting for uninstall of old version") - sleep(10) # 5 seconds is not enough. - + # In case the previous shutdown was unclean, make sure the app isn't + # running, otherwise we might show logs from a previous run. This is + # unnecessary in --managed mode, because Gradle creates a new emulator + # every time. + stop_app(context.connected) initial_devices = None try: diff --git a/Android/testbed/gradle.properties b/Android/testbed/gradle.properties index 3c5031eb7d63f7..e9f345c8c26250 100644 --- a/Android/testbed/gradle.properties +++ b/Android/testbed/gradle.properties @@ -20,4 +20,9 @@ kotlin.code.style=official # Enables namespacing of each library's R class so that its R class includes only the # resources declared in the library itself and none from the library's dependencies, # thereby reducing the size of the R class for that library -android.nonTransitiveRClass=true \ No newline at end of file +android.nonTransitiveRClass=true + +# By default, the app will be uninstalled after the tests finish (apparently +# after 10 seconds in case of an unclean shutdown). We disable this, because +# when using android.py it can conflict with the installation of the next run. +android.injected.androidTest.leaveApksInstalledAfterRun=true From 706a1dab7fcc67217321a4098e92c24265e486e1 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 12 Aug 2024 22:38:03 +0100 Subject: [PATCH 25/34] Make testbed pick up edits to pure-Python files in the Lib directory --- Android/README.md | 5 +++ Android/testbed/app/build.gradle.kts | 31 +++++++++++-------- .../java/org/python/testbed/MainActivity.kt | 2 +- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/Android/README.md b/Android/README.md index cb774f7684825a..874456363febaa 100644 --- a/Android/README.md +++ b/Android/README.md @@ -115,3 +115,8 @@ See the [Python Developer's Guide](https://devguide.python.org/testing/run-write-tests/) for common options – most of them will work on Android, except for those that involve subprocesses, such as `-j`. + +Every time you run `android.py test`, changes in pure-Python files in the +repository's `Lib` directory will be picked up immediately. Changes in C files, +and architecture-specific files such as sysconfigdata, will not take effect +until you re-run `android.py make-host` or `build`. diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 1ab40c0416e68a..321f39325f4eb6 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -112,29 +112,34 @@ dependencies { // Create some custom tasks to copy Python and its standard library from // elsewhere in the repository. androidComponents.onVariants { variant -> + val pyPlusVer = "python$PYTHON_VERSION" generateTask(variant, variant.sources.assets!!) { into("python") { - for (triplet in ABIS.values) { - for (subDir in listOf("include", "lib")) { - into(subDir) { - from("$PYTHON_CROSS_DIR/$triplet/prefix/$subDir") - include("python$PYTHON_VERSION/**") - duplicatesStrategy = DuplicatesStrategy.EXCLUDE - } + into("include/$pyPlusVer") { + for (triplet in ABIS.values) { + from("$PYTHON_CROSS_DIR/$triplet/prefix/include/$pyPlusVer") } + duplicatesStrategy = DuplicatesStrategy.EXCLUDE } - into("lib/python$PYTHON_VERSION") { - // Uncomment this to pick up edits from the source directory - // without having to rerun `make install`. - // from("$PYTHON_DIR/Lib") - // duplicatesStrategy = DuplicatesStrategy.INCLUDE + + into("lib/$pyPlusVer") { + // To aid debugging, the source directory takes priority. + from("$PYTHON_DIR/Lib") + + // The cross-build directory provides ABI-specific files such as + // sysconfigdata. + for (triplet in ABIS.values) { + from("$PYTHON_CROSS_DIR/$triplet/prefix/lib/$pyPlusVer") + } into("site-packages") { from("$projectDir/src/main/python") } + + duplicatesStrategy = DuplicatesStrategy.EXCLUDE + exclude("**/__pycache__") } } - exclude("**/__pycache__") } generateTask(variant, variant.sources.jniLibs!!) { diff --git a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt index 846e3ccb9ddab8..c4bf6cbe83d8cd 100644 --- a/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt +++ b/Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt @@ -15,7 +15,7 @@ class MainActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) setContentView(R.layout.activity_main) - val status = PythonTestRunner(this).run() + val status = PythonTestRunner(this).run("-W -uall") findViewById(R.id.tvHello).text = "Exit status $status" } } From cf15b99ccb1e03b8cbd0c01319a8fa0a19c680e9 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 14:48:44 +0100 Subject: [PATCH 26/34] Remove `boot_completed`, which is unnecessary since we're no longer running `pm list`. --- Android/android.py | 36 ++++++------------- .../java/org/python/testbed/PythonSuite.kt | 2 +- 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/Android/android.py b/Android/android.py index 6aa799a1357a9d..ead5f5aa238694 100755 --- a/Android/android.py +++ b/Android/android.py @@ -15,9 +15,8 @@ from contextlib import asynccontextmanager from os.path import basename, relpath from pathlib import Path -from subprocess import CalledProcessError, DEVNULL +from subprocess import CalledProcessError from tempfile import TemporaryDirectory -from time import sleep SCRIPT_NAME = Path(__file__).name @@ -350,26 +349,13 @@ async def find_device(context, initial_devices): return context.connected -# Before boot is completed, `pm list` will fail with the error "Can't find -# service: package". -async def boot_completed(serial): - # No need to show a message if the device is already booted. - shown_message = False - while True: - if (await async_check_output( - adb, "-s", serial, "shell", "getprop", "sys.boot_completed" - )).strip() == "1": - return - else: - if not shown_message: - print("Waiting for boot") - shown_message = True - await asyncio.sleep(1) - - -# An older version of this script in #121595 filtered the logs by UID instead, -# which is more reliable since the PID is shorter-lived. But logcat can't filter -# by UID until API level 31. +# An older version of this script in #121595 filtered the logs by UID instead. +# But logcat can't filter by UID until API level 31. If we ever switch back to +# filtering by UID, we'll also have to filter by time so we only show messages +# produced after the initial call to `stop_app`. +# +# We're more likely to miss the PID because it's shorter-lived, so there's a +# workaround in PythonSuite.kt to stop it being *too* short-lived. async def find_pid(serial): print("Waiting for app to start - this may take several minutes") while True: @@ -378,7 +364,7 @@ async def find_pid(serial): adb, "-s", serial, "shell", "pidof", "-s", APP_ID )).strip() except CalledProcessError as e: - # If the app isn't running yet, pidof gives no output, so if there + # If the app isn't running yet, pidof gives no output. So if there # is output, there must have been some other error. if e.stdout or e.stderr: raise @@ -399,7 +385,6 @@ async def logcat_task(context, initial_devices): # in --connected mode. startup_timeout = 600 serial = await wait_for(find_device(context, initial_devices), startup_timeout) - await wait_for(boot_completed(serial), startup_timeout) pid = await wait_for(find_pid(serial), startup_timeout) args = [adb, "-s", serial, "logcat", "--pid", pid, "--format", "tag"] @@ -582,7 +567,8 @@ def parse_args(): "--managed", metavar="NAME", help="Run on a Gradle-managed device. " "These are defined in `managedDevices` in testbed/app/build.gradle.kts.") test.add_argument( - "args", nargs="*", help="Extra arguments for `python -m test`") + "args", nargs="*", help=f"Arguments for `python -m test`. " + f"Separate them from {SCRIPT_NAME}'s own arguments with `--`.") return parser.parse_args() diff --git a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt index f62ccefb49dbca..0e888ab71d87da 100644 --- a/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt +++ b/Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt @@ -25,7 +25,7 @@ class PythonSuite { assertEquals(0, status) } finally { // Make sure the process lives long enough for the test script to - // detect it. + // detect it (see `find_pid` in android.py). val delay = 2000 - (System.currentTimeMillis() - start) if (delay > 0) { Thread.sleep(delay) From c56373a20196950dc30cdb1e2064f175fbb1ab94 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 16:05:00 +0100 Subject: [PATCH 27/34] Automatically accept SDK licenses, and log Gradle package installations even in non-verbose mode --- Android/android.py | 56 +++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/Android/android.py b/Android/android.py index ead5f5aa238694..42292ef82de008 100755 --- a/Android/android.py +++ b/Android/android.py @@ -30,15 +30,10 @@ try: - android_home = os.environ['ANDROID_HOME'] + android_home = Path(os.environ['ANDROID_HOME']) except KeyError: sys.exit("The ANDROID_HOME environment variable is required.") -sdkmanager = Path( - f"{android_home}/cmdline-tools/latest/bin/sdkmanager" - + (".bat" if os.name == "nt" else "") -) - adb = Path( f"{android_home}/platform-tools/adb" + (".exe" if os.name == "nt" else "") @@ -217,10 +212,34 @@ def clean_all(context): delete_glob(CROSS_BUILD_DIR) +def setup_sdk(): + sdkmanager = android_home / ( + "cmdline-tools/latest/bin/sdkmanager" + + (".bat" if os.name == "nt" else "") + ) + + # Gradle will fail if it needs to install an SDK package whose license + # hasn't been accepted, so pre-accept all licenses. + if not all((android_home / "licenses" / path).exists() for path in [ + "android-sdk-arm-dbt-license", "android-sdk-license" + ]): + run([sdkmanager, "--licenses"], text=True, input="y\n" * 100) + + # Gradle may install this automatically, but we can't rely on that because + # we need to run adb within the logcat task. + if not adb.exists(): + run([sdkmanager, "platform-tools"]) + + # To avoid distributing compiled artifacts without corresponding source code, # the Gradle wrapper is not included in the CPython repository. Instead, we # extract it from the Gradle release. -def setup_testbed(context): +def setup_testbed(): + if all((TESTBED_DIR / path).exists() for path in [ + "gradlew", "gradlew.bat", "gradle/wrapper/gradle-wrapper.jar", + ]): + return + ver_long = "8.7.0" ver_short = ver_long.removesuffix(".0") @@ -240,15 +259,11 @@ def setup_testbed(context): f"{temp_dir}/{outer_jar}", "gradle-wrapper.jar"]) -def setup_testbed_if_needed(context): - if not gradlew.exists(): - setup_testbed(context) - - # run_testbed will build the app automatically, but it hides the Gradle output # by default, so it's useful to have this as a separate command for the buildbot. def build_testbed(context): - setup_testbed_if_needed(context) + setup_sdk() + setup_testbed() run( [gradlew, "--console", "plain", "packageDebug", "packageDebugAndroidTest"], cwd=TESTBED_DIR, @@ -459,7 +474,9 @@ async def gradle_task(context): stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) as process: while line := (await process.stdout.readline()).decode(*DECODE_ARGS): - if context.verbose: + # Gradle may take several minutes to install SDK packages, so + # it's worth showing those messages even in non-verbose mode. + if context.verbose or line.startswith('Preparing "Install'): sys.stdout.write(line) else: hidden_output.append(line) @@ -481,12 +498,8 @@ async def gradle_task(context): async def run_testbed(context): - if not adb.exists(): - print("Installing Platform-Tools") - # Input "y" to accept licenses. - run([sdkmanager, "platform-tools"], text=True, input="y\n" * 100) - - setup_testbed_if_needed(context) + setup_sdk() + setup_testbed() if context.managed: # In this mode, Gradle will create a device with an unpredictable name. @@ -550,8 +563,6 @@ def parse_args(): subcommand.add_argument("args", nargs="*", help="Extra arguments to pass to `configure`") - subcommands.add_parser( - "setup-testbed", help="Download the testbed Gradle wrapper") subcommands.add_parser( "build-testbed", help="Build the testbed app") test = subcommands.add_parser( @@ -582,7 +593,6 @@ def main(): "make-host": make_host_python, "build": build_all, "clean": clean_all, - "setup-testbed": setup_testbed, "build-testbed": build_testbed, "test": run_testbed} From 1e89e58b254c533002091637d8777b3dcaca8db1 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 16:25:10 +0100 Subject: [PATCH 28/34] Stop Gradle test tasks being skipped as up to date --- Android/testbed/app/build.gradle.kts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Android/testbed/app/build.gradle.kts b/Android/testbed/app/build.gradle.kts index 321f39325f4eb6..7e0bef58ed88eb 100644 --- a/Android/testbed/app/build.gradle.kts +++ b/Android/testbed/app/build.gradle.kts @@ -96,6 +96,16 @@ android { systemImageSource = "aosp-atd" } } + + // If the previous test run succeeded and nothing has changed, + // Gradle thinks there's no need to run it again. Override that. + afterEvaluate { + (localDevices.names + listOf("connected")).forEach { + tasks.named("${it}DebugAndroidTest") { + outputs.upToDateWhen { false } + } + } + } } } } From 91f356b38bf8ced6764ad493eefd7b229fcb087d Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 17:58:18 +0100 Subject: [PATCH 29/34] Fix CalledProcessError formatting --- Android/android.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Android/android.py b/Android/android.py index 42292ef82de008..0f802147b902ae 100755 --- a/Android/android.py +++ b/Android/android.py @@ -609,10 +609,14 @@ def main(): if not content.endswith("\n"): stream.write("\n") - # Format the command so it can be copied into a shell. - args_joined = " ".join(shlex.quote(str(arg)) for arg in e.cmd) + # Format the command so it can be copied into a shell. shlex uses single + # quotes, so we surround the whole command with double quotes. + args_joined = ( + e.cmd if isinstance(e.cmd, str) + else " ".join(shlex.quote(str(arg)) for arg in e.cmd) + ) sys.exit( - f"Command '{args_joined}' returned exit status {e.returncode}" + f'Command "{args_joined}" returned exit status {e.returncode}' ) From bdaad24cf6c122fbf71717576bfa10e81b1b7047 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 19:32:29 +0100 Subject: [PATCH 30/34] Add note about testing on Windows --- Android/README.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Android/README.md b/Android/README.md index 874456363febaa..0b78f05d382d0c 100644 --- a/Android/README.md +++ b/Android/README.md @@ -80,10 +80,13 @@ call. For example, if you want a pydebug build that also caches the results from ## Testing -Before running the test suite, follow the instructions in the previous section -to build the architecture you want to test. +The tests can be run on Linux, macOS, or Windows, although on Windows you'll +have to build the `cross-build/HOST` subdirectory on one of the other platforms +and copy it over. -The test script can be run in two modes: +Before running the test suite, follow the instructions in the previous section +to build the architecture you want to test. Then run the test script in one of +the following modes: * In `--connected` mode, it runs on a device or emulator you have already connected to the build machine. List the available devices with From ca2bae2cc32775c99ae0b6b2b74412af0a73c539 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 19:36:12 +0100 Subject: [PATCH 31/34] Implement `fileno` method on stdout and stderr --- Lib/_android_support.py | 19 ++++++++++++++----- Lib/test/test_android.py | 13 ++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Lib/_android_support.py b/Lib/_android_support.py index d5d13ec6a48e14..066e54708fb75c 100644 --- a/Lib/_android_support.py +++ b/Lib/_android_support.py @@ -31,15 +31,17 @@ def init_streams(android_log_write, stdout_prio, stderr_prio): logcat = Logcat(android_log_write) sys.stdout = TextLogStream( - stdout_prio, "python.stdout", errors=sys.stdout.errors) + stdout_prio, "python.stdout", sys.stdout.fileno(), + errors=sys.stdout.errors) sys.stderr = TextLogStream( - stderr_prio, "python.stderr", errors=sys.stderr.errors) + stderr_prio, "python.stderr", sys.stderr.fileno(), + errors=sys.stderr.errors) class TextLogStream(io.TextIOWrapper): - def __init__(self, prio, tag, **kwargs): + def __init__(self, prio, tag, fileno=None, **kwargs): kwargs.setdefault("encoding", "UTF-8") - super().__init__(BinaryLogStream(prio, tag), **kwargs) + super().__init__(BinaryLogStream(prio, tag, fileno), **kwargs) self._lock = RLock() self._pending_bytes = [] self._pending_bytes_count = 0 @@ -98,9 +100,10 @@ def line_buffering(self): class BinaryLogStream(io.RawIOBase): - def __init__(self, prio, tag): + def __init__(self, prio, tag, fileno=None): self.prio = prio self.tag = tag + self._fileno = fileno def __repr__(self): return f"" @@ -122,6 +125,12 @@ def write(self, b): logcat.write(self.prio, self.tag, b) return len(b) + # This is needed by the test suite --timeout option, which uses faulthandler. + def fileno(self): + if self._fileno is None: + raise io.UnsupportedOperation("fileno") + return self._fileno + # When a large volume of data is written to logcat at once, e.g. when a test # module fails in --verbose3 mode, there's a risk of overflowing logcat's own diff --git a/Lib/test/test_android.py b/Lib/test/test_android.py index 82035061bb6fdd..09fa21cea877c2 100644 --- a/Lib/test/test_android.py +++ b/Lib/test/test_android.py @@ -19,6 +19,9 @@ api_level = platform.android_ver().api_level +# (name, level, fileno) +STREAM_INFO = [("stdout", "I", 1), ("stderr", "W", 2)] + # Test redirection of stdout and stderr to the Android log. @unittest.skipIf( @@ -94,19 +97,21 @@ def stream_context(self, stream_name, level): stack = ExitStack() stack.enter_context(self.subTest(stream_name)) stream = getattr(sys, stream_name) + native_stream = getattr(sys, f"__{stream_name}__") if isinstance(stream, io.StringIO): stack.enter_context( patch( f"sys.{stream_name}", TextLogStream( - prio, f"python.{stream_name}", errors="backslashreplace" + prio, f"python.{stream_name}", native_stream.fileno(), + errors="backslashreplace" ), ) ) return stack def test_str(self): - for stream_name, level in [("stdout", "I"), ("stderr", "W")]: + for stream_name, level, fileno in STREAM_INFO: with self.stream_context(stream_name, level): stream = getattr(sys, stream_name) tag = f"python.{stream_name}" @@ -114,6 +119,7 @@ def test_str(self): self.assertIs(stream.writable(), True) self.assertIs(stream.readable(), False) + self.assertEqual(stream.fileno(), fileno) self.assertEqual("UTF-8", stream.encoding) self.assertIs(stream.line_buffering, True) self.assertIs(stream.write_through, False) @@ -257,13 +263,14 @@ def __str__(self): write("\n", [s * 51]) # 0 bytes in, 510 bytes out def test_bytes(self): - for stream_name, level in [("stdout", "I"), ("stderr", "W")]: + for stream_name, level, fileno in STREAM_INFO: with self.stream_context(stream_name, level): stream = getattr(sys, stream_name).buffer tag = f"python.{stream_name}" self.assertEqual(f"", repr(stream)) self.assertIs(stream.writable(), True) self.assertIs(stream.readable(), False) + self.assertEqual(stream.fileno(), fileno) def write(b, lines=None, *, write_len=None): if write_len is None: From 7a3d674f7174cd13b159be2e2af48b705d0869b6 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Tue, 13 Aug 2024 23:18:13 +0100 Subject: [PATCH 32/34] Correct comment --- Lib/test/libregrtest/cmdline.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Lib/test/libregrtest/cmdline.py b/Lib/test/libregrtest/cmdline.py index 2ff4715e82a41b..8bef04cba81138 100644 --- a/Lib/test/libregrtest/cmdline.py +++ b/Lib/test/libregrtest/cmdline.py @@ -428,9 +428,7 @@ def _parse_args(args, **kwargs): # Continuous Integration (CI): common options for fast/slow CI modes if ns.slow_ci or ns.fast_ci: # Similar to options: - # - # -j0 --randomize --fail-env-changed --fail-rerun --rerun - # --slowest --verbose3 + # -j0 --randomize --fail-env-changed --rerun --slowest --verbose3 if ns.use_mp is None: ns.use_mp = 0 ns.randomize = True From 602fbdda23066ca1d4b6ef85ba0dfcd5cf755c8e Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 15 Aug 2024 21:49:50 +0100 Subject: [PATCH 33/34] Add note about RAM requirements --- Android/README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Android/README.md b/Android/README.md index 0b78f05d382d0c..bae9150ef057ac 100644 --- a/Android/README.md +++ b/Android/README.md @@ -84,6 +84,14 @@ The tests can be run on Linux, macOS, or Windows, although on Windows you'll have to build the `cross-build/HOST` subdirectory on one of the other platforms and copy it over. +The test suite can usually be run on a device with 2 GB of RAM, though for some +configurations or test orders you may need to increase this. As of Android +Studio Koala, 2 GB is the default for all emulators, although the user interface +may indicate otherwise. The effective setting is `hw.ramSize` in +~/.android/avd/*.avd/hardware-qemu.ini, whereas Android Studio displays the +value from config.ini. Changing the value in Android Studio will update both of +these files. + Before running the test suite, follow the instructions in the previous section to build the architecture you want to test. Then run the test script in one of the following modes: From 305d7867b0733ff0d5b31dcbeb7774ecadffdde8 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 15 Aug 2024 22:40:05 +0100 Subject: [PATCH 34/34] Handle transient failure of pidof --- Android/android.py | 50 +++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/Android/android.py b/Android/android.py index 0f802147b902ae..b5403b5d2a4a5e 100755 --- a/Android/android.py +++ b/Android/android.py @@ -373,6 +373,7 @@ async def find_device(context, initial_devices): # workaround in PythonSuite.kt to stop it being *too* short-lived. async def find_pid(serial): print("Waiting for app to start - this may take several minutes") + shown_error = False while True: try: pid = (await async_check_output( @@ -380,9 +381,13 @@ async def find_pid(serial): )).strip() except CalledProcessError as e: # If the app isn't running yet, pidof gives no output. So if there - # is output, there must have been some other error. - if e.stdout or e.stderr: - raise + # is output, there must have been some other error. However, this + # sometimes happens transiently, especially when running a managed + # emulator for the first time, so don't make it fatal. + if (e.stdout or e.stderr) and not shown_error: + print_called_process_error(e) + print("This may be transient, so continuing to wait") + shown_error = True else: # Some older devices (e.g. Nexus 4) return zero even when no process # was found, so check whether we actually got any output. @@ -601,23 +606,28 @@ def main(): if asyncio.iscoroutine(result): asyncio.run(result) except CalledProcessError as e: - for stream_name in ["stdout", "stderr"]: - content = getattr(e, stream_name) - stream = getattr(sys, stream_name) - if content: - stream.write(content) - if not content.endswith("\n"): - stream.write("\n") - - # Format the command so it can be copied into a shell. shlex uses single - # quotes, so we surround the whole command with double quotes. - args_joined = ( - e.cmd if isinstance(e.cmd, str) - else " ".join(shlex.quote(str(arg)) for arg in e.cmd) - ) - sys.exit( - f'Command "{args_joined}" returned exit status {e.returncode}' - ) + print_called_process_error(e) + sys.exit(1) + + +def print_called_process_error(e): + for stream_name in ["stdout", "stderr"]: + content = getattr(e, stream_name) + stream = getattr(sys, stream_name) + if content: + stream.write(content) + if not content.endswith("\n"): + stream.write("\n") + + # Format the command so it can be copied into a shell. shlex uses single + # quotes, so we surround the whole command with double quotes. + args_joined = ( + e.cmd if isinstance(e.cmd, str) + else " ".join(shlex.quote(str(arg)) for arg in e.cmd) + ) + print( + f'Command "{args_joined}" returned exit status {e.returncode}' + ) if __name__ == "__main__":