[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/utils/ci/vendor/android/Dockerfile.emulator.base:2
+#===----------------------------------------------------------------------===##
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
Is it necessary to have a `.base` and a non-base Dockerfile? If so, let's leave a comment to explain why. Otherwise, consider merging both files.


================
Comment at: libcxx/utils/ci/vendor/android/Dockerfile.emulator.base:22
+
+RUN curl -sL https://dl.google.com/android/repository/commandlinetools-linux-9477386_latest.zip -o cmdline-tools.zip && \
+    mkdir -p ${ANDROID_HOME} && \
----------------
Can we use a URL that points to the latest version so that we get a new version of the command-line tools when we rebuild that docker image?

Or do we want to be intentional about the version we're using here so that we only bump it when we really want to? If so, what is going to be the process for bumping it up? Is that something that you or other Android folks can commit to doing on a regular basis? Otherwise, this will be forgotten forever and will never be updated.


================
Comment at: libcxx/utils/ci/vendor/android/build-toolchain.sh:1
+#!/usr/bin/env bash
+#===----------------------------------------------------------------------===##
----------------
Note to self: This would also go away if we downloaded a pre-built toolchain instead.


================
Comment at: libcxx/utils/ci/vendor/android/emulator-wait-for-ready.sh:23
+#
+# XXX: Consider waiting for `adb shell getprop dev.bootcomplete 2>/dev/null
+# | grep 1 >/dev/null` as well. It adds ~4 seconds to 21-def-x86 and ~15 seconds
----------------



================
Comment at: libcxx/utils/ci/vendor/android/start-emulator.sh:27
+
+# Start the container.
+docker run --name libcxx-ci-android-emulator --detach --device /dev/kvm \
----------------
This assumes that the docker image named `$(docker_image_of_emu_img ${EMU_IMG})` is somehow available on whatever's running the CI. We should probably instead publish the emulator's Docker image somewhere (maybe the same place we publish our other docker images) and use that.


================
Comment at: libunwind/cmake/caches/Android.cmake:1
+set(LLVM_ENABLE_RUNTIMES "libunwind" CACHE STRING "")
+set(CMAKE_BUILD_TYPE "Release" CACHE STRING "")
----------------
This would go away if we downloaded a pre-built toolchain and used that toolchain's libunwind instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139147