[libcxx-commits] [PATCH] D139147: [libc++][Android] Enable libc++ testing using adb on an x86-64 emulator

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

It's awesome to see this taking form! This will be the first time that we have an upstream CI bot running via an emulator, and I know there are many more use cases for this so it's great to break trail.

I have a few comments but this LGTM. I agree with others that it would be nice to add a CI runner in the same patch to test that it all works.



================
Comment at: libcxx/cmake/caches/AndroidNDK.cmake:21
+# symbol conflicts on older versions of Android.
+set(LIBCXX_ABI_NAMESPACE __ndk1 CACHE STRING "")
+set(LIBCXX_ABI_VERSION 1 CACHE STRING "")
----------------
rprichard wrote:
> philnik wrote:
> > Does Android actually use a different ABI namespace, or is this just to avoid name clashes when testing? If it's just fur testing purposes I'd suggest to rename it to something like `__test`.
> Android has a few different builds of libc++. In the "platform libc++" (`/system/lib[64]/libc++.so`), the namespace is the ordinary `__1`. Apps aren't supposed to use the platform libc++, though, and instead they redistribute a `libc++_shared.so` from the Android NDK. `libc++_shared.so` uses an `__ndk1` namespace. Both the filename and the namespace are supposed to help isolate the NDK libc++ from the platform STL.
> 
> With recent Android versions, there are also "APEX modules" that have a copy of libc++.so in them. This copy is very similar to the platform libc++.so, but typically built against an older API level.
> 
> Naming it `__test` for this test configuration seems reasonable.
> 
Ideally, this cache file would reflect the way you actually build for the NDK. The executables in the test suite should reliably use `-nostdlib++` and so on to make sure that we're linking only against the just-built libc++, so I am not sure whether these hoops are necessary. Did you notice that the tests were not being linked/run against the right library?

The same comment holds for the `_static` renaming above.


================
Comment at: libcxx/utils/ci/run-buildbot:632-635
+    echo "+++ Pushing libc++ to the device"
+    adb shell rm -fr /data/local/tmp/libc++
+    adb shell mkdir /data/local/tmp/libc++
+    adb push "${BUILD_DIR}/lib/libc++_shared.so" /data/local/tmp/libc++
----------------
It would be sweet to be able to do this automatically when we startup the test suite. But this is probably fine to improve incrementally, this is already a giant step in the right direction.


================
Comment at: libcxxabi/test/configs/llvm-libc++abi-android-ndk.cfg.in:35
+    lit_config
+)
----------------
Not attached to this line: You should update our platform support matrix to add Android if it's not already there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139147