-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Android] Enable building static stdlib on macOS #4972
Conversation
6c6aea8
to
4f9929a
Compare
I think that we should consider splitting this up a fair amount. Enabling the policy for Doing that will simplify this change and make it easier to digest. |
Absolutely, will do. Thanks, @compnerd! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta comment. Can you split this up?
function(_add_swift_lipo_target sdk target output) | ||
if("${sdk}" STREQUAL "") | ||
message(FATAL_ERROR "sdk is required") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some new utilities called precondition. It makes this a bit cleaner:
precondition("${sdk}" MESSAGE "sdk is required")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! :)
@@ -707,6 +715,14 @@ function(_add_swift_library_single target name) | |||
add_dependencies(${target}_IMPLIB ${${target}_IMPORT_LIBRARY}) | |||
endif() | |||
set_property(TARGET "${target}" PROPERTY NO_SONAME ON) | |||
elseif("${SWIFTLIB_SINGLE_SDK}" STREQUAL "ANDROID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code path is pretty cheesy. Do you think you can refactor it out in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what you mean by "cheesy"? Is something like #5068 what you had in mind?
set(PLIST_INFO_BUILD_VERSION | ||
"${SWIFT_COMPILER_VERSION}") | ||
endif() | ||
if(SWIFTLIB_SINGLE_IS_STDLIB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this linker code somewhere else. My reasoning is that appending -shared to the link_flags on android has nothing to do with plist creation. I would look for where linker flags are added for other platforms and put the linker flag change there.
@@ -156,14 +156,9 @@ set(SWIFTLIB_SOURCES | |||
Zip.swift | |||
) | |||
set(GROUP_INFO_JSON_FILE ${CMAKE_CURRENT_SOURCE_DIR}/GroupInfo.json) | |||
set(swift_core_link_flags "${SWIFT_RUNTIME_SWIFT_LINK_FLAGS}") | |||
set(swift_core_framework_depends) | |||
set(swift_core_private_link_libraries) | |||
set(swift_stdlib_compile_flags "${SWIFT_RUNTIME_SWIFT_COMPILE_FLAGS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for refactoring! = D.
Question. Why didn't you also get rid of swift_stdlib_compile_flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed swift_core_framework_depends
because it was only used in the add_swift_library
call for TARGET_SDKS ALL_APPLE_PLATFORMS
. Instead of setting it here, I just inlined it.
Similarly, swift_core_link_flags
was just SWIFT_RUNTIME_SWIFT_LINK_FLAGS
, except for Apple platforms, where -all_load
was appended to it. So I just used SWIFT_RUNTIME_SWIFT_LINK_FLAGS
for all calls to add_swift_library
, except the call with TARGET_SDKS ALL_APPLE_PLATFORMS
includes an -all_load
.
SwiftObject.mm | ||
SwiftValue.mm | ||
Remangle.cpp | ||
Reflection.mm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code no longer conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's only included in the add_swift_library
call with TARGET_SDKS ALL_APPLE_PLATFORMS
, and not in the add_swift_library
call for all non-Apple ones. When building on Linux, the files are still included in this list variable, but the variable is never used.
4f9929a
to
c931968
Compare
b9a369e
to
54776ac
Compare
e42997f
to
c2774dc
Compare
I rebased these changes on top of:
This can probably be split up further, but I'll wait until those pull requests are reviewed/merged. |
2a79f9d
to
022bdf0
Compare
022bdf0
to
f6838c5
Compare
f6838c5
to
8a5dbea
Compare
@swift-ci Please test |
Build failed |
|
@modocache Yes, Please re-trigger testing |
Build failed |
@swift-ci Please test |
@erg @gottesmm @jrose-apple Friendly ping! It'd be great if I could build off of these changes over the weekend. :) |
endif() | ||
else() | ||
message(FATAL_ERROR "A Darwin or Linux host is required to build the Swift runtime for Android") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing this to an early exit?
if (NOT ("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Linux"))
message(FATAL_ERROR "A Darwin or Linux host is required to build the Swift runtime for Android")
endif ()
You can then set the SWIFT_PRIMARY_VARIANT_SDK_default
and SWIFT_PRIMARY_VARIANT_ARCH_default
unconditionally reducing duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@compnerd Done!
The Swift runtime can be built for Android on Linux host machines using the instructions in `docs/Android.md`. This commit allows a static build of the runtime to be compiled on macOS host machines as well. This work is inspired by @compnerd's work in enabling Windows builds from macOS hosts. It assumes host tools are already built for macOS. It also does not support building SDK overlays yet. I intend to support these in a future commit. For now, the Android runtime may be built by using the following bash script, which simply calls CMake: ```sh set -e set -x HOME_DIR="/Users/bgesiak" APPLE_DIR="${HOME_DIR}/GitHub/apple" BUILD_DIR="${APPLE_DIR}/build/Ninja-Release" CMARK_SRC_DIR="${APPLE_DIR}/cmark" CMARK_BUILD_DIR="${BUILD_DIR}/cmark-macosx-x86_64" LLVM_SRC_DIR="${APPLE_DIR}/llvm" LLVM_BUILD_DIR="${BUILD_DIR}/llvm-macosx-x86_64" SWIFT_BUILD_DIR="${BUILD_DIR}/swift-macosx-x86_64" ANDROID_NDK_DIR="${HOME_DIR}/android-ndk-r12b" LIBICU_SRC_DIR="${APPLE_DIR}/libiconv-libicu-android" LIBICU_BUILD_DIR="${LIBICU_SRC_DIR}/armeabi-v7a" SWIFT_ANDROID_BUILD_DIR="${APPLE_DIR}/build/SwiftAndroid" mkdir -p "${SWIFT_ANDROID_BUILD_DIR}" cmake \ -G "Ninja" \ \ -DCMAKE_C_COMPILER:PATH="${LLVM_BUILD_DIR}/bin/clang" \ -DCMAKE_CXX_COMPILER:PATH="${LLVM_BUILD_DIR}/bin/clang++" \ \ -DSWIFT_PATH_TO_CMARK_SOURCE:PATH="${CMARK_SRC_DIR}" \ -DSWIFT_PATH_TO_CMARK_BUILD:PATH="${CMARK_BUILD_DIR}" \ -DSWIFT_CMARK_LIBRARY_DIR:PATH="${CMARK_BUILD_DIR}/src" \ -DSWIFT_PATH_TO_LLVM_SOURCE:PATH="${LLVM_SRC_DIR}" \ -DSWIFT_PATH_TO_LLVM_BUILD:PATH="${LLVM_BUILD_DIR}" \ -DSWIFT_PATH_TO_CLANG_SOURCE:PATH="${LLVM_SRC_DIR}/tools/clang" \ -DSWIFT_PATH_TO_CLANG_BUILD:PATH="${LLVM_BUILD_DIR}" \ -DSWIFT_NATIVE_LLVM_TOOLS_PATH:STRING="${LLVM_BUILD_DIR}/bin" \ -DSWIFT_NATIVE_SWIFT_TOOLS_PATH:STRING="${SWIFT_BUILD_DIR}/bin" \ \ -DSWIFT_ANALYZE_CODE_COVERAGE:STRING=FALSE \ -DSWIFT_SERIALIZE_STDLIB_UNITTEST:BOOL=FALSE \ -DSWIFT_STDLIB_SIL_DEBUGGING:BOOL=FALSE \ -DSWIFT_CHECK_INCREMENTAL_COMPILATION:BOOL=FALSE \ -DSWIFT_STDLIB_ENABLE_RESILIENCE:BOOL=FALSE \ -DSWIFT_STDLIB_SIL_SERIALIZE_ALL:BOOL=TRUE \ -DSWIFT_BUILD_PERF_TESTSUITE:BOOL=FALSE \ -DSWIFT_BUILD_EXAMPLES:BOOL=FALSE \ -DSWIFT_INCLUDE_TESTS:BOOL=FALSE \ -DSWIFT_EMBED_BITCODE_SECTION:BOOL=FALSE \ -DSWIFT_TOOLS_ENABLE_LTO:STRING= \ -DSWIFT_AST_VERIFIER:BOOL=TRUE \ -DSWIFT_SIL_VERIFY_ALL:BOOL=FALSE \ -DSWIFT_RUNTIME_ENABLE_LEAK_CHECKER:BOOL=FALSE \ \ -DCMAKE_BUILD_TYPE:STRING=Release \ -DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE \ -DSWIFT_STDLIB_BUILD_TYPE:STRING=Release \ -DSWIFT_STDLIB_ASSERTIONS:BOOL=TRUE \ -DSWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER:BOOL=TRUE \ -DSWIFT_HOST_VARIANT=macosx \ -DSWIFT_HOST_VARIANT_SDK=OSX \ -DSWIFT_HOST_VARIANT_ARCH=x86_64 \ -DSWIFT_BUILD_TOOLS:BOOL=FALSE \ -DSWIFT_INCLUDE_TOOLS:BOOL=FALSE \ -DSWIFT_BUILD_REMOTE_MIRROR:BOOL=FALSE \ -DSWIFT_BUILD_DYNAMIC_STDLIB:BOOL=FALSE \ -DSWIFT_BUILD_STATIC_STDLIB:BOOL=TRUE \ -DSWIFT_BUILD_DYNAMIC_SDK_OVERLAY:BOOL=FALSE \ -DSWIFT_BUILD_STATIC_SDK_OVERLAY:BOOL=FALSE \ -DSWIFT_EXEC:STRING="${SWIFT_BUILD_DIR}/bin/swiftc" \ \ -DSWIFT_ENABLE_GOLD_LINKER:BOOL=TRUE \ -DSWIFT_SDKS:STRING=ANDROID \ -DSWIFT_ANDROID_NDK_PATH:STRING="${ANDROID_NDK_DIR}" \ -DSWIFT_ANDROID_NDK_GCC_VERSION:STRING=4.9 \ -DSWIFT_ANDROID_SDK_PATH:STRING="${ANDROID_NDK_DIR}/platforms/android-21/arch-arm" \ -DSWIFT_ANDROID_ICU_UC:STRING="${LIBICU_BUILD_DIR}" \ -DSWIFT_ANDROID_ICU_UC_INCLUDE:STRING="${LIBICU_BUILD_DIR}/icu/source/common" \ -DSWIFT_ANDROID_ICU_I18N:STRING="${LIBICU_BUILD_DIR}" \ -DSWIFT_ANDROID_ICU_I18N_INCLUDE:STRING="${LIBICU_BUILD_DIR}/icu/source/i18n" \ \ "${APPLE_DIR}/swift" cmake --build "${APPLE_DIR}/swift" -- -j8 swift-stdlib-android-armv7 ```
37c946e
to
be5a34e
Compare
@swift-ci Please test |
Build failed |
The DebugInfo test failure appears unrelated. |
@swift-ci please test |
Build failed |
@swift-ci please test os x platform |
Build failed |
@swift-ci please test os x platform |
Build failed |
@swift-ci please test os x platform |
@erg Can you take this one. I need to focus on semantic ARC. Let me dismiss the review. |
The code has changed and I don't have time right now to review = (
Awesome, thanks @erg!! |
great persistent efforts @modocache thanks a lot.. moving in that direction. |
The Swift runtime can be built for Android on Linux host machines using the instructions in
docs/Android.md
. This commit allows it to be built on macOS host machines as well.This work is inspired by @compnerd's work in enabling Windows builds from macOS hosts. It assumes host tools are already built for macOS. It also does not support building the stdlib and SDK overlays yet. I intend to support these in a future commit.
For now, the Android runtime may be built by using the following bash script, which simply calls CMake:
Addresses part of SR-1362, with stdlib and SDK overlay support coming in a future pull request.