Specify linker for Android armv7 target. #7777
Conversation
CMakeLists.txt
Outdated
| @@ -813,6 +813,9 @@ if(swift_build_android AND NOT "${SWIFT_ANDROID_NDK_PATH}" STREQUAL "") | |||
| endif() | |||
| set(SWIFT_ANDROID_PREBUILT_PATH | |||
| "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}") | |||
| get_filename_component(CMAKE_LINKER_NAME "${CMAKE_LINKER}" NAME) | |||
There was a problem hiding this comment.
Why derive this from CMAKE_LINKER? Why not specify the complete path to the cmake invocation?
There was a problem hiding this comment.
I thought it was better to do this rather than checking for SWIFT_USE_GOLD_LINKER, but I agree it is misleading. I can replace it for the check on the macro (ld and ld.gold are the only two linkers in the ndk toolchain anyway) or add a comment.
gonzalolarralde
force-pushed
the
master
branch
2 times, most recently
from
4512271 to
a66e857
Compare
|
@compnerd rebased it and added a comment for the linker selection part. Let me know if looks ok now. Thanks! |
CMakeLists.txt
Outdated
| # Resolve the correct linker based on the file name of CMAKE_LINKER (being 'ld' or 'ld.gold' the options) | ||
| get_filename_component(CMAKE_LINKER_NAME "${CMAKE_LINKER}" NAME) | ||
| set(SWIFT_SDK_ANDROID_ARCH_armv7_LINKER | ||
| "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}/bin/arm-linux-androideabi-${CMAKE_LINKER_NAME}") |
There was a problem hiding this comment.
Shouldnt this use the filename component rather than CMAKE_LINKER_NAME?
There was a problem hiding this comment.
@compnerd the filename component is being saved in CMAKE_LINKER_NAME but I can set the variable name to SWIFT_ANDROID_LINKER_NAME.
There was a problem hiding this comment.
I think that the renamed variable choice is significantly better.
There was a problem hiding this comment.
@compnerd fixed :) sorry about that.
CMakeLists.txt
Outdated
| "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}") | ||
|
|
||
| # Resolve the correct linker based on the file name of CMAKE_LINKER (being 'ld' or 'ld.gold' the options) | ||
| get_filename_component(CMAKE_LINKER_NAME "${CMAKE_LINKER}" NAME) |
There was a problem hiding this comment.
Doesn't this clobber CMAKE_LINKER_NAME? I think your parameters are swapped.
There was a problem hiding this comment.
Correct, it does. The result is being saved in CMAKE_LINKER_NAME (this is not a CMAKE var, so I can change it to SWIFT_ANDROID_LINKER_NAME). The params of get_filename_component are <DEST_VAR> <FILE_NAME> <COMPONENT>.
* `prefix` should be `sdk` in runtime cmake list file * typo on variable existence checking
|
Currently Swift for Android does not build on Ubuntu 16.04.2 LTS. I cherry-picked this PR, and now Swift for Android builds successfully. Should definitely consider giving this PR some love. :) cc @erg |
| "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}") | ||
|
|
||
| # Resolve the correct linker based on the file name of CMAKE_LINKER (being 'ld' or 'ld.gold' the options) | ||
| get_filename_component(SWIFT_ANDROID_LINKER_NAME "${CMAKE_LINKER}" NAME) |
There was a problem hiding this comment.
This is a bad idea in the long run. Since this enables building for Android, I think this is okay to commit, but we shouldn't use CMAKE_LINKER per se. I think that detecting the linker here should be separate since the valid linkers are ld, ld.gold, ld.lld, ld.ld64, link. At least two of those do not support ELF.
|
@swift-ci please smoke test and merge |
|
@swift-ci please smoke test and merge |
prefixshould besdkin runtime cmake list file