[libcxx-commits] [PATCH] D139147: [libc++][Android] Enable libc++ testing on Android

[Louis Dionne via Phabricator via libcxx-commits]

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++.
+set(CMAKE_EXE_LINKER_FLAGS "-nostdlib++" CACHE STRING "")
+set(CMAKE_SHARED_LINKER_FLAGS "-nostdlib++" CACHE STRING "")
----------------
I think what you want here instead is 

```
CMAKE_C_COMPILER_WORKS = True
CMAKE_CXX_COMPILER_WORKS = True
```

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
-// XFAIL{BLOCKLIT}: LIBCXX-ANDROID-FIXME
+// UNSUPPORTED{BLOCKLIT}: LIBCXX-ANDROID-FIXME
 
----------------
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.
 EOF
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139147/new/

https://reviews.llvm.org/D139147