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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D139147#4386090 <https://reviews.llvm.org/D139147#4386090>, @rprichard wrote:

> I was wondering whether I can land this patch adding Android testing to buildkite-pipeline.yml while the Android libc++ tests are still failing, or if the buildbot isn't set up yet. e.g. I wouldn't want non-Android-related LLVM patches to be affected. (Maybe I should land all the Android-related work first, and only then add the entry to the yml file.)

I think the way to go is marking all the failing tests as `XFAIL` and go from there. This avoids regressions and makes it easier to verify that a patch actually does what it claims to do.

> I'm also wondering how the buildbot is restarted when the ldionne/libcxx-builder Docker image changes.
>
> In D139147#4384537 <https://reviews.llvm.org/D139147#4384537>, @philnik wrote:
>
>> In D139147#4382747 <https://reviews.llvm.org/D139147#4382747>, @rprichard wrote:
>>
>>> I still haven't added anything to libcxx/utils/ci/buildkite-pipeline.yml. It's not clear to me whether that should be part of this patch, because there are several things that must happen first before CI tests will work:
>>>
>>> - I need to create a new docker image to add the Android stuff to it, and host it somewhere.
>>> - I need to setup a special buildbot on a VM somewhere that uses the new docker image.
>>
>> You don't have to use a Docker image for a CI bot. Although it would be really nice, this is not a requirement, and if you want to do this it can still be done later.
>
> This patch has a Dockerfile.android that produces a libcxx-builder-android image -- I just need to publish it somewhere. I think we want to keep the Android stuff out of the main ldionne/libcxx-builder Docker image because it's big? When I measured it back in December, adding Android support would increase the size from 4.7GB to 12.5GB. The Android SDK is large (6.8G), and I also need to install the `openjdk-11-jdk` Ubuntu package (400M). The buildbot could download the SDK when it starts, but then a local run of `run-buildbot android-ndk` would be slow.

That is indeed quite a lot of extra data. Maybe we could just have a second image `ldionne/libcxx-builder-android`. Then everybody who has to can access and update it without problems.

>>> - There are a bunch of patches I have uploaded to Phabricator currently that are required to get tests running cleanly.
>>
>> Any tests that fail can be marked `LIBCXX-ANDROID-FIXME` for now. Or do you mean something more general?
>
> It's not that many patches, but Android isn't a properly supported target yet in libc++, so maybe that discouraged reviewing?

I can't speak for everybody, but I personally am much more reluctant to accept a patch that isn't for a supported platform. Having a CI that tells you that the patch doesn't break anything major is quite helpful, especially since you don't have to assume that the person tested it properly.

> Some of the patches are small but required to make many tests work, e.g. D137130 <https://reviews.llvm.org/D137130> and D137131 <https://reviews.llvm.org/D137131> in particular. D139497 <https://reviews.llvm.org/D139497> is needed to get three tests passing. D137129 <https://reviews.llvm.org/D137129> is also simple and fixes one test.
>
>>> - There are a few other fixes for tests that I don't have uploaded, because I'm not quite sure what we want to do (need to disable certain filesystem tests, and there are a couple of demangler problems involving floating-point).
>>
>> Anything that doesn't work can be marked `LIBCXX-ANDROID-FIXME` for now. If it turns out these tests can't work on Android it can still be updated later.
>
> That's probably a good way to disable some of the tests, like test_demangle.pass.cpp and copy_file_large.pass.cpp, which fail on some devices.
>
> Filesystem tests are a problem still:
>
> - Security policy (SELinux...) on Android blocks the `shell` (default non-root) user from creating/using FIFOs, domain sockets, or hard links. The tests can't be run as root because some tests verify that APIs fail from lack of permissions. There are already targets that lack FIFOs and domain sockets, so I have a WIP patch extending that to `__ANDROID__`. That patch also disables a few places that create hard links. I had wondered if there should be a more principled way of indicating (lack of) support for special kinds of files in the test suite. (Also: I suppose the tests could sometimes run as root, and drop privileges as needed? Probably not worth doing.)
> - The L (API 21) non-emulator devices I test with don't allow symlinks for the `shell` user, so all(?) the filesystem tests fail (`static_test_env()` assert fails). The tests succeed on an L emulator. The ability to create symlinks was present in K and is restored in M. Maybe use some sort of "has symlinks" feature to disable all the filesystem tests? Or maybe the test suite can be refactored so the non-symlink tests run. From Android's POV, this also doesn't seem worth it because the filesystem code is sufficiently tested (on the emulator, or on newer devices).

If the tests behave differently on different platforms it's also an option to mark then as `// UNSUPPORTED: LIBCXX-ANDROID-FIXME`. The only difference is that we don't know whether a problem has been fixed somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139147