Allow Android testbed to take all Python command-line options

Author: mhsmith

Android/android.py

@@ -2,6 +2,7 @@
 
 import asyncio
 import argparse
+import json
 import os
 import platform
 import re
@@ -546,27 +547,22 @@ async def gradle_task(context):
         task_prefix = "connected"
         env["ANDROID_SERIAL"] = context.connected
 
-    if context.command:
-        mode = "-c"
-        module = context.command
-    else:
-        mode = "-m"
-        module = context.module or "test"
+    if not any(arg in context.args for arg in ["-c", "-m"]):
+        context.args[0:0] = ["-m", "test"]
 
     args = [
         gradlew, "--console", "plain", f"{task_prefix}DebugAndroidTest",
     ] + [
-        # Build-time properties
-        f"-Ppython.{name}={value}"
-        for name, value in [
-            ("sitePackages", context.site_packages), ("cwd", context.cwd)
-        ] if value
-    ] + [
-        # Runtime properties
-        f"-Pandroid.testInstrumentationRunnerArguments.python{name}={value}"
+        f"-P{name}={value}"
         for name, value in [
-            ("Mode", mode), ("Module", module), ("Args", join_command(context.args))
-        ] if value
+            ("python.sitePackages", context.site_packages),
+            ("python.cwd", context.cwd),
+            (
+                "android.testInstrumentationRunnerArguments.pythonArgs",
+                json.dumps(context.args),
+            ),
+        ]
+        if value
     ]
     if context.verbose >= 2:
         args.append("--info")
@@ -734,13 +730,19 @@ def ci(context):
     else:
         with TemporaryDirectory(prefix=SCRIPT_NAME) as temp_dir:
             print("::group::Tests")
+
             # Prove the package is self-contained by using it to run the tests.
             shutil.unpack_archive(package_path, temp_dir)
 
-            # Randomization is disabled because order-dependent failures are
-            # much less likely to pass on a rerun in single-process mode.
             launcher_args = ["--managed", "maxVersion", "-v"]
-            test_args = ["--fast-ci", "--single-process", "--no-randomize"]
+            test_args = [
+                # See _add_ci_python_opts in libregrtest/main.py.
+                "-W", "error", "-bb", "-E",
+
+                # Randomization is disabled because order-dependent failures are
+                # much less likely to pass on a rerun in single-process mode.
+                "-m", "test", "--fast-ci", "--single-process", "--no-randomize"
+            ]
             run(
                 ["./android.py", "test", *launcher_args, "--", *test_args],
                 cwd=temp_dir
@@ -825,18 +827,11 @@ def add_parser(*args, **kwargs):
     test.add_argument(
         "--cwd", metavar="DIR", type=abspath,
         help="Directory to copy as the app's working directory.")
-
-    mode_group = test.add_mutually_exclusive_group()
-    mode_group.add_argument(
-        "-c", dest="command", help="Execute the given Python code.")
-    mode_group.add_argument(
-        "-m", dest="module", help="Execute the module with the given name.")
-    test.epilog = (
-        "If neither -c nor -m are passed, the default is '-m test', which will "
-        "run Python's own test suite.")
     test.add_argument(
-        "args", nargs="*", help=f"Arguments to add to sys.argv. "
-        f"Separate them from {SCRIPT_NAME}'s own arguments with `--`.")
+        "args", nargs="*", help=f"Python command-line arguments. "
+        f"Separate them from {SCRIPT_NAME}'s own arguments with `--`. "
+        f"If neither -c nor -m are included, `-m test` will be prepended, "
+        f"which will run Python's own test suite.")
 
     # Package arguments.
     for subcommand in [package, ci]:

Android/testbed/app/src/androidTest/java/org/python/testbed/PythonSuite.kt

@@ -20,7 +20,7 @@ class PythonSuite {
             val status = PythonTestRunner(
                 InstrumentationRegistry.getInstrumentation().targetContext
             ).run(
-                InstrumentationRegistry.getArguments()
+                InstrumentationRegistry.getArguments().getString("pythonArgs")!!,
             )
             assertEquals(0, status)
         } finally {

Android/testbed/app/src/main/c/main_activity.c

@@ -3,6 +3,7 @@
 #include <jni.h>
 #include <pthread.h>
 #include <Python.h>
+#include <signal.h>
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
@@ -15,6 +16,13 @@ static void throw_runtime_exception(JNIEnv *env, const char *message) {
         message);
 }
 
+static void throw_errno(JNIEnv *env, const char *error_prefix) {
+    char error_message[1024];
+    snprintf(error_message, sizeof(error_message),
+             "%s: %s", error_prefix, strerror(errno));
+    throw_runtime_exception(env, error_message);
+}
+
 
 // --- Stdio redirection ------------------------------------------------------
 
@@ -95,10 +103,7 @@ JNIEXPORT void JNICALL Java_org_python_testbed_PythonTestRunner_redirectStdioToL
     for (StreamInfo *si = STREAMS; si->file; si++) {
         char *error_prefix;
         if ((error_prefix = redirect_stream(si))) {
-            char error_message[1024];
-            snprintf(error_message, sizeof(error_message),
-                     "%s: %s", error_prefix, strerror(errno));
-            throw_runtime_exception(env, error_message);
+            throw_errno(env, error_prefix);
             return;
         }
     }
@@ -107,41 +112,86 @@ JNIEXPORT void JNICALL Java_org_python_testbed_PythonTestRunner_redirectStdioToL
 
 // --- Python initialization ---------------------------------------------------
 
-static PyStatus set_config_string(
-    JNIEnv *env, PyConfig *config, wchar_t **config_str, jstring value
-) {
-    const char *value_utf8 = (*env)->GetStringUTFChars(env, value, NULL);
-    PyStatus status = PyConfig_SetBytesString(config, config_str, value_utf8);
-    (*env)->ReleaseStringUTFChars(env, value, value_utf8);
-    return status;
+static char *init_signals() {
+    // Some tests use SIGUSR1, but that's blocked by default in an Android app in
+    // order to make it available to `sigwait` in the Signal Catcher thread.
+    // (https://cs.android.com/android/platform/superproject/+/android14-qpr3-release:art/runtime/signal_catcher.cc).
+    // That thread's functionality is only useful for debugging the JVM, so disabling
+    // it should not weaken the tests.
+    //
+    // There's no safe way of stopping the thread completely (#123982), but simply
+    // unblocking SIGUSR1 is enough to fix most tests.
+    //
+    // However, in tests that generate multiple different signals in quick
+    // succession, it's possible for SIGUSR1 to arrive while the main thread is busy
+    // running the C-level handler for a different signal. In that case, the SIGUSR1
+    // may be sent to the Signal Catcher thread instead, which will generate a log
+    // message containing the text "reacting to signal".
+    //
+    // Such tests may need to be changed in one of the following ways:
+    //   * Use a signal other than SIGUSR1 (e.g. test_stress_delivery_simultaneous in
+    //     test_signal.py).
+    //   * Send the signal to a specific thread rather than the whole process (e.g.
+    //     test_signals in test_threadsignals.py.
+    sigset_t set;
+    if (sigemptyset(&set)) {
+        return "sigemptyset";
+    }
+    if (sigaddset(&set, SIGUSR1)) {
+        return "sigaddset";
+    }
+    if ((errno = pthread_sigmask(SIG_UNBLOCK, &set, NULL))) {
+        return "pthread_sigmask";
+    }
+    return NULL;
 }
 
 static void throw_status(JNIEnv *env, PyStatus status) {
     throw_runtime_exception(env, status.err_msg ? status.err_msg : "");
 }
 
 JNIEXPORT int JNICALL Java_org_python_testbed_PythonTestRunner_runPython(
-    JNIEnv *env, jobject obj, jstring home, jstring runModule
+    JNIEnv *env, jobject obj, jstring home, jarray args
 ) {
+    const char *home_utf8 = (*env)->GetStringUTFChars(env, home, NULL);
+    char cwd[PATH_MAX];
+    snprintf(cwd, sizeof(cwd), "%s/%s", home_utf8, "cwd");
+    if (chdir(cwd)) {
+        throw_errno(env, "chdir");
+        return 1;
+    }
+
+    char *error_prefix;
+    if ((error_prefix = init_signals())) {
+        throw_errno(env, error_prefix);
+        return 1;
+    }
+
     PyConfig config;
     PyStatus status;
-    PyConfig_InitIsolatedConfig(&config);
+    PyConfig_InitPythonConfig(&config);
 
-    status = set_config_string(env, &config, &config.home, home);
-    if (PyStatus_Exception(status)) {
+    jsize argc = (*env)->GetArrayLength(env, args);
+    const char *argv[argc + 1];
+    for (int i = 0; i < argc; i++) {
+        jobject arg = (*env)->GetObjectArrayElement(env, args, i);
+        argv[i] = (*env)->GetStringUTFChars(env, arg, NULL);
+    }
+    argv[argc] = NULL;
+
+    // PyConfig_SetBytesArgv "must be called before other methods, since the
+    // preinitialization configuration depends on command line arguments"
+    if (PyStatus_Exception(status = PyConfig_SetBytesArgv(&config, argc, (char**)argv))) {
         throw_status(env, status);
         return 1;
     }
 
-    status = set_config_string(env, &config, &config.run_module, runModule);
+    status = PyConfig_SetBytesString(&config, &config.home, home_utf8);
     if (PyStatus_Exception(status)) {
         throw_status(env, status);
         return 1;
     }
 
-    // Some tests generate SIGPIPE and SIGXFSZ, which should be ignored.
-    config.install_signal_handlers = 1;
-
     status = Py_InitializeFromConfig(&config);
     if (PyStatus_Exception(status)) {
         throw_status(env, status);

Android/testbed/app/src/main/java/org/python/testbed/MainActivity.kt

@@ -5,6 +5,7 @@ import android.os.*
 import android.system.Os
 import android.widget.TextView
 import androidx.appcompat.app.*
+import org.json.JSONArray
 import java.io.*
 
 
@@ -15,30 +16,25 @@ class MainActivity : AppCompatActivity() {
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         setContentView(R.layout.activity_main)
-        val status = PythonTestRunner(this).run("-m", "test", "-W -uall")
+        val status = PythonTestRunner(this).run("""["-m", "test", "-W", "-uall"]""")
         findViewById<TextView>(R.id.tvHello).text = "Exit status $status"
     }
 }
 
 
 class PythonTestRunner(val context: Context) {
-    fun run(instrumentationArgs: Bundle) = run(
-        instrumentationArgs.getString("pythonMode")!!,
-        instrumentationArgs.getString("pythonModule")!!,
-        instrumentationArgs.getString("pythonArgs") ?: "",
-    )
-
     /** Run Python.
      *
-     * @param mode Either "-c" or "-m".
-     * @param module Python statements for "-c" mode, or a module name for
-     *     "-m" mode.
-     * @param args Arguments to add to sys.argv. Will be parsed by `shlex.split`.
+     * @param args Python command-line, encoded as JSON.
      * @return The Python exit status: zero on success, nonzero on failure. */
-    fun run(mode: String, module: String, args: String) : Int {
-        Os.setenv("PYTHON_MODE", mode, true)
-        Os.setenv("PYTHON_MODULE", module, true)
-        Os.setenv("PYTHON_ARGS", args, true)
+    fun run(args: String) : Int {
+        // We leave argument 0 as an empty string, which is a placeholder for the
+        // executable name in embedded mode.
+        val argsJsonArray = JSONArray(args)
+        val argsStringArray = Array<String>(argsJsonArray.length() + 1) { it -> ""}
+        for (i in 0..<argsJsonArray.length()) {
+            argsStringArray[i + 1] = argsJsonArray.getString(i)
+        }
 
         // Python needs this variable to help it find the temporary directory,
         // but Android only sets it on API level 33 and later.
@@ -47,10 +43,7 @@ class PythonTestRunner(val context: Context) {
         val pythonHome = extractAssets()
         System.loadLibrary("main_activity")
         redirectStdioToLogcat()
-
-        // The main module is in src/main/python. We don't simply call it
-        // "main", as that could clash with third-party test code.
-        return runPython(pythonHome.toString(), "android_testbed_main")
+        return runPython(pythonHome.toString(), argsStringArray)
     }
 
     private fun extractAssets() : File {
@@ -59,6 +52,13 @@ class PythonTestRunner(val context: Context) {
             throw RuntimeException("Failed to delete $pythonHome")
         }
         extractAssetDir("python", context.filesDir)
+
+        // Empty directories are lost in the asset packing/unpacking process.
+        val cwd = File(pythonHome, "cwd")
+        if (!cwd.exists()) {
+            cwd.mkdir()
+        }
+
         return pythonHome
     }
 
@@ -88,5 +88,5 @@ class PythonTestRunner(val context: Context) {
     }
 
     private external fun redirectStdioToLogcat()
-    private external fun runPython(home: String, runModule: String) : Int
+    private external fun runPython(home: String, args: Array<String>) : Int
 }

Android/testbed/app/src/main/python/android_testbed_main.py

@@ -646,15 +646,20 @@ def _add_cross_compile_opts(self, regrtest_opts):
         return (environ, keep_environ)
 
     def _add_ci_python_opts(self, python_opts, keep_environ):
-        # --fast-ci and --slow-ci add options to Python:
-        # "-u -W default -bb -E"
-
-        # Unbuffered stdout and stderr
-        if not sys.stdout.write_through:
+        # --fast-ci and --slow-ci add options to Python.
+        #
+        # Some platforms cannot change options after startup, so if these
+        # options are changed, also update the copies in:
+        #  * cpython/Android/android.py
+        #  * buildmaster-config/master/custom/factories.py
+
+        # Unbuffered stdout and stderr. This isn't helpful on Android, because
+        # it would cause lines to be split into multiple log messages.
+        if not sys.stdout.write_through and sys.platform != "android":
             python_opts.append('-u')
 
         # Add warnings filter 'error'
-        if 'default' not in sys.warnoptions:
+        if 'error' not in sys.warnoptions:
             python_opts.extend(('-W', 'error'))
 
         # Error on bytes/str comparison