[PATCH] D139147: [libc++][Android] Enable libc++ testing on Android

Louis Dionne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 10 10:06:15 PDT 2023

ldionne added inline comments.

Comment at: libcxx/cmake/caches/AndroidNDK.cmake:25-28
+# Clang links libc++ by default, but it doesn't exist yet, which breaks CMake's
+# feature testing, so pass -nostdlib++.
I think what you want here instead is 


like in https://reviews.llvm.org/D150766. And in the future this shouldn't be required in caches, it should just work out of the box.

Comment at: libcxx/cmake/caches/AndroidNDK.cmake:39-40
+set(LIBCXX_TEST_PARAMS "android_libcxx_kind=ndk" CACHE STRING "")
+set(LIBCXXABI_TEST_PARAMS "android_libcxx_kind=ndk" CACHE STRING "")
I would suggest treating those as entirely different builds and giving them their own cache files. There are ways to avoid duplication (e.g. `include(AndroidBase.cmake)`) without adding a ad-hoc knob here.

Comment at: libcxx/test/configs/llvm-libc++-android.cfg.in:15-23
+# Allow overriding the target API level for each test run. The API level used
+# to build the tests should often be higher than that used to build libc++.
+# Android's internal test harness prefers to specify target_api directly to lit,
+# independently of the CMake arguments, which appears in `lit_config.params`,
+# but run-buildbot needs to configure this using LIBCXX_TEST_PARAMS, where it
+# appears as an attribute of the `config` object. Check both places.
+target_api = lit_config.params.get('target_api', getattr(config, 'target_api', None))
Here, you should be able to simply specify `--param target_triple=<WHATEVER>` when running `lit`, and get rid of `target_api` altogether.

Comment at: libcxx/test/libcxx/modules_include.gen.py:35
 // The Android headers don't appear to be compatible with modules yet
This needs to be updated when you rebase, there's two references in this file now.

Comment at: libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp:11-13
+// Android devices frequently don't have enough memory to run this test. Rather
+// than throw std::bad_alloc, exhausting memory tends to trigger the OOM Killer
+// and/or crash the device (killing adb, rebooting it, etc).
Per live review, this comment can be updated.

Comment at: libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp:14
+// and/or crash the device (killing adb, rebooting it, etc).
+// UNSUPPORTED: android
For now, I would prefer that we keep this `LIBCXX-ANDROID-FIXME` so we can have a separate discussion about how to handle low-memory environments in the test suite (more generally).

Comment at: libcxx/utils/ci/buildkite-pipeline.yml:1092
+    - label: "Android 5.0, x86 NDK"
+      command: "libcxx/utils/ci/vendor/android/stop-emulator-after-run.sh libcxx/utils/ci/run-buildbot android-ndk"
+      artifact_paths:
I think we should make the `run-buildbot` script stop the emulator unconditionally. It would be nice to keep the property that all the `command`s in the buildkite pipeline are just direct calls to `run-buildbot` (with a few exceptions).

Comment at: libcxx/utils/ci/buildkite-pipeline.yml:1097
+      env:
+          ANDROID_EMU_IMG: "21-def-x86"
+      agents:
Instead of this, you could run `libcxx/utils/ci/run-buildbot android-ndk-<VERSION>`, similarly to what we do for the macOS backdeployment?

Comment at: libcxx/utils/ci/run-buildbot:55
+ANDROID_EMU_IMG     The name of an Android emulator OS image.
So this goes away.

Comment at: libcxx/utils/ci/run-buildbot:701-704
+    if ! validate_emu_img "${ANDROID_EMU_IMG}"; then
+        echo "error: ANDROID_EMU_IMG must be a valid emulator image." >&2
+        exit 1
+    fi
Not needed anymore.

Comment at: libcxx/utils/ci/run-buildbot:707-714
+    # Compile the builtins archive and libunwind.a and add them to a patched
+    # resource directory, and generate Clang wrapper scripts.
+    "${MONOREPO_ROOT}/libcxx/utils/ci/vendor/android/build-toolchain.sh" \
+        --llvm-root "${MONOREPO_ROOT}" \
+        --output-dir "${BUILD_DIR}/android-toolchain" \
+        --cmake "${CMAKE}" --ninja "${NINJA}" --arch ${ARCH}
+    export CC="${BUILD_DIR}/android-toolchain/bin/clang"
I feel like this is a workaround for not having the equivalent of an `apt.llvm.org` build of the toolchain. We should make this available on the system we're testing on instead (which probably means building it into the docker image).

Comment at: libcxx/utils/ci/vendor/android/Dockerfile.emulator:1
Note to self: review stopped here.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list