[android] Put in fixes for librt and armv7-a #554
Conversation
| find_package(LibRT) | ||
| if(NOT CMAKE_SYSTEM_NAME STREQUAL Android) | ||
| find_package(LibRT) | ||
| endif() |
There was a problem hiding this comment.
This is fine to do, but really unfortunate. CMake already already can deal with librt properly on various targets, but we don't use CMake properly :-(.
There was a problem hiding this comment.
Yes, you could use CMAKE_FIND_ROOT_PATH, like I do when cross-compiling the corelibs, if there is an librt.so to find, but there is none on Android- it's just part of libc.so- so this is the right change for Android.
| if(NOT BUILD_SHARED_LIBS AND CMAKE_SYSTEM_NAME STREQUAL Android AND | ||
| CMAKE_SYSTEM_PROCESSOR STREQUAL armv7-a) | ||
| target_link_options(${name} PRIVATE "LINKER:--allow-multiple-definition") | ||
| endif() |
There was a problem hiding this comment.
I think this needs a huge comment. This is indicative of an issue - the multiple definitions shouldnt be there. The warnings indicate that there is a mix up between libgcc and libunwind, which is a really bad thing as it will crash at runtime in a difficult to diagnose manner (the internal layouts of the unwind context is not compatible/guaranteed).
There was a problem hiding this comment.
Yeah, I don't know exactly what's going on here, their doc doesn't clear it up for me. All I know is that applying this workaround flag gets the test executables to build, and I tried moving a handful to Android and they ran fine. I will add a comment.
Android doesn't have a separate librt, it's just part of libc. Also, the static build wasn't working for armv7-a, because the test executables wouldn't link with the multiple definition errors listed in android/ndk#176, so use the workaround given there.
|
Ping, if someone can run the CI, we can get this in. |
|
@benlangmuir, mind running the CI on this? |
|
@swift-ci please test |
|
A final CI run and this can be merged. |
|
@swift-ci test |
|
CI failed because of unrelated lldb compilation error. |
|
@swift-ci please test |
|
Passed CI, can someone merge? |
|
I just ran into the build failing when cross-compiling for Android AArch64 too, because it tries to link against the system libRT.so on Fedora Core x86_64 too. @benlangmuir, mind merging? |
|
Someone who works on this project should review/merge. I'm not the right person to look at this. It looks like @compnerd was looking at it earlier. |
|
Alright, @MadCoder, mind reviewing? |
|
Ping @drexin, can you get this simple pull in? |
[android] Put in fixes for librt and armv7-a Signed-off-by: Rokhini Prabhu <[email protected]>
Android doesn't have a separate librt, it's just part of libc. Also, the static build wasn't working for armv7-a, because the test executables wouldn't link with the multiple definition errors listed in android/ndk#176, so use the workaround given there.
I didn't have any problems when cross-compiling for 64-bit arches, but hit the first one because I happen to have an i686 libc package installed in Ubuntu and the armv7 build would try to use librt.so from that. The second libunwind issue has been around for awhile, and I worked around it before by not building the tests for libdispatch, but this gets them to build and run.
@compnerd, let me know what you think.