-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[android] Add more changes to build the compiler #69538
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -405,9 +405,9 @@ set(SWIFT_STDLIB_MSVC_RUNTIME_LIBRARY | |
|
||
|
||
if(BRIDGING_MODE STREQUAL "DEFAULT" OR NOT BRIDGING_MODE) | ||
if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8)) | ||
if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${SWIFT_HOST_VARIANT_SDK}" MATCHES "WINDOWS|ANDROID" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8)) | ||
# In debug builds, to workaround a problem with LLDB's `po` command (rdar://115770255). | ||
# On windows to workaround a build problem. | ||
# On Windows and Android, to workaround a build problem. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pure bridging allows me to work around #60272 for Android. |
||
# If the host Swift version is less than 5.8, use pure mode to workaround a C++ interop compiler crash. | ||
set(BRIDGING_MODE "PURE") | ||
else() | ||
|
@@ -864,7 +864,7 @@ endif() | |
|
||
if(SWIFT_BUILD_SWIFT_SYNTAX) | ||
# Only "HOSTTOOLS" is supported in Linux when Swift parser integration is enabled. | ||
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD" AND NOT BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS") | ||
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|OPENBSD|FREEBSD" AND NOT BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Android does not need this override, for example, I use the |
||
message(WARNING "Force setting BOOTSTRAPPING=HOSTTOOLS because Swift parser integration is enabled") | ||
set(BOOTSTRAPPING_MODE "HOSTTOOLS") | ||
endif() | ||
|
@@ -1194,6 +1194,7 @@ if(SWIFT_INCLUDE_TOOLS) | |
message(STATUS " Assertions: ${LLVM_ENABLE_ASSERTIONS}") | ||
message(STATUS " LTO: ${SWIFT_TOOLS_ENABLE_LTO}") | ||
message(STATUS " Bootstrapping: ${BOOTSTRAPPING_MODE}") | ||
message(STATUS " C++ Bridging: ${BRIDGING_MODE}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
message(STATUS " Swift parser: ${SWIFT_BUILD_SWIFT_SYNTAX}") | ||
message(STATUS "") | ||
else() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,8 +54,14 @@ public extension NoReflectionChildren { | |
|
||
public var standardError = CFileStream(fp: stderr) | ||
|
||
#if os(Android) || canImport(Musl) | ||
public typealias FILEPointer = OpaquePointer | ||
#else | ||
public typealias FILEPointer = UnsafeMutablePointer<FILE> | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took this from my similar pull for |
||
|
||
public struct CFileStream: TextOutputStream { | ||
var fp: UnsafeMutablePointer<FILE> | ||
var fp: FILEPointer | ||
|
||
public func write(_ string: String) { | ||
fputs(string, fp) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -607,6 +607,9 @@ function(_add_swift_runtime_link_flags target relpath_to_lib_dir bootstrapping) | |
endif() | ||
endif() | ||
endif() | ||
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD" AND SWIFT_USE_LINKER STREQUAL "lld") | ||
target_link_options(${target} PRIVATE "SHELL:-Xlinker -z -Xlinker nostart-stop-gc") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed since lld 13 on ELF platforms a couple years ago, #60406, which linking the new SwiftSyntax library here triggers. |
||
endif() | ||
endif() | ||
|
||
set_property(TARGET ${target} PROPERTY BUILD_WITH_INSTALL_RPATH YES) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,7 @@ static void reportNow(uint32_t flags, const char *message) { | |
#endif | ||
#if SWIFT_STDLIB_HAS_ASL | ||
asl_log(nullptr, nullptr, ASL_LEVEL_ERR, "%s", message); | ||
#elif defined(__ANDROID__) | ||
#elif defined(__ANDROID__) && !defined(__TERMUX__) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushing these compilation errors to the system log is not useful, certainly not on Termux. |
||
__android_log_print(ANDROID_LOG_FATAL, "SwiftDemangle", "%s", message); | ||
#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.
CMAKE_SYSTEM_NAME
is not set bybuild-script
when cross-compiling, this repo usesSWIFT_HOST_VARIANT_SDK
instead.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.
Most of the time
CMAKE_SYSTEM_NAME
is set by CMake. Scripts only need to provide a value explicitly when cross-compiling (see https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html).I do not think the change here is correct. Before it was checking if the compiler was being build for Windows, and after the changes it is checking if the machine building the compiler is Windows (or Android). This is changing behaviour for some cross-compiling scenarios.
There's at least one check for
CMAKE_SYSTEM_NAME = Android
. Isn't that something that can be used?I will recommend recovering the previous line, and adding the match for
ANDROID
in a different clause to not modify the previously working code: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.
Yep, that's the point I'm making. Suppose you want to cross-compile the Swift compiler from linux to Windows: the current check using
CMAKE_SYSTEM_NAME
would not work, because we do not setCMAKE_SYSTEM_NAME
anywhere inbuild-script
.No, that's not how this repo works. This repo only sets
SWIFT_HOST_VARIANT_SDK
and uses it extensively to implement its own cross-compilation system, without using CMake's built-in cross-compilation support that you refer to.SWIFT_HOST_VARIANT_SDK
does not refer to "the machine building the compiler," but replacesCMAKE_SYSTEM_NAME
as the host the compiler should be built for, at least in this repo.It can be used when natively compiling, because as you noted, the native host can be inferred by CMake then. However, the few places that use
CMAKE_SYSTEM_NAME
in this repo all break when cross-compiling, for the reasons given above.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.
build-script
is just the way the Darwin/Linux CI uses to build Swift, but Windows CI builds, in particular, do not usebuild-script
(it is impossible). Other people use unified builds with LLVM and other ways of kick starting the build and try to avoidbuild-script
.I would strongly recommend not changing anything related to other systems like Windows, specially when it is really easy to avoid it, like in this case.
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'm well aware, as I have merged changes to those Windows batch files in this repo before.
Trust me, I know what I'm doing in this repo. Even those other build setups that use
CMAKE_SYSTEM_NAME
have to translate that to the rightSWIFT_HOST_VARIANT_SDK
for this repo, or this repo's build does not work.I had checked to make sure that translation was done above this line before changing this. I know what I'm doing by now. 😉
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.
No, I'm confident in how this compiler build works. If you are not, please take a look at my links to the source to see what I'm talking about.
Sure, and then this compiler build will not work.
The reason that "override" you mention is there is because
build-script
passes inSWIFT_HOST_VARIANT_SDK
to this CMake build, as I linked above. It does not useCMAKE_SYSTEM_NAME
. For those who insist on usingCMAKE_SYSTEM_NAME
instead outside ofbuild-script
, the translation toSWIFT_HOST_VARIANT_SDK
that I linked you above is used.Now, I can see how the use of both
CMAKE_SYSTEM_NAME
andSWIFT_HOST_VARIANT_SDK
in this repo can be confusing, as I remarked that both were being used in this repo from my first pull almost five years ago, though most heavy usage is ofSWIFT_HOST_VARIANT_SDK
alone.That inconsistent usage happens to work when natively compiling because CMake can easily infer the native host and set
CMAKE_SYSTEM_NAME
, but it can break when cross-compiling if you don't set both variables correctly.Now, let's look at the two scenarios, with
build-script
or without.build-script
does not set aCMAKE_SYSTEM_NAME
when cross-compiling, so it is safer to checkSWIFT_HOST_VARIANT_SDK
on this line then. Withoutbuild-script
, one can do all kinds of nonsense, as you noted, but if you setCMAKE_SYSTEM_NAME
alone, the translation above ensures thatSWIFT_HOST_VARIANT_SDK
is properly defined, so again it is safer to checkSWIFT_HOST_VARIANT_SDK
here.I have worked through this logic multiple times, and I'm telling you, this is the right check for this repo.
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.
Not going to insist more on this topic. I am sorry we cannot agree in a solution that works for both cases. If nobody else have problems with this and I am the only one having this problem, feel free to merge.
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.
@drodriguez, I'm telling you, this does work for both cases. If you just look elsewhere in the same file, there are other
SWIFT_HOST_VARIANT_SDK
checks just like this, indicating at least that both styles are used.If you are worried about breaking something, please just run the CI now, which as we discussed, runs both with and without
build-script
. That will show that this doesn't break anything.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 think that @drodriguez has a valid point here. Consider the builds for the macOS and the fat runtimes on Apple platforms. These can be different (at least currently). I think that we should prefer the CMake handling for the cross-compilation mechanism and is something that might be a possibility in the future. We shouldn't make this harder for such a future refactoring.
Uh oh!
There was an error while loading. Please reload this page.
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.
Every Darwin OS sets
SWIFT_HOST_VARIANT_SDK
as I'm checking here- see the next 100 lines after that link too- none setCMAKE_SYSTEM_NAME
.I know: you, me, and Daniel have had this argument since my first pull building the compiler for Android almost five years ago. The fact remains that after all these years, most of this main compiler/stdlib repo primarily uses
SWIFT_HOST_VARIANT_SDK
and with good reason, as we discussed before and here.It hasn't happened in five years, I don't think it will happen anytime soon.
It's a simple search-and-replace, it doesn't make it hard at all. On the contrary, checking
CMAKE_SYSTEM_NAME
as you did here breaks cross-compilation for every platform that doesn't setCMAKE_SYSTEM_NAME
differently when cross-compiling, which is every platform usingbuild-script
, ie almost every Swift platform.As I emphasized from my first pull, you can't check either
CMAKE_SYSTEM_NAME
orSWIFT_HOST_VARIANT_SDK
randomly throughout this compiler codebase, because it will happen to work when natively compiling the compiler but breaks cross-compiling the Swift compiler and stdlib. Sneaking inCMAKE_SYSTEM_NAME
checks like this piecemeal actually breaks things, it is not helping matters.Since I'm probably the only person consistly cross-compiling the Swift compiler from one OS to another (CMake simply uses
Darwin
for all Apple platforms, so that wouldn't be considered cross-OS by CMake, I guess), as the Termux package CI cross-compiles from linux x86_64 for Android, I'm well aware of the problems withCMAKE_SYSTEM_NAME
, which is why I removed its use for this check.Now, all that said, I said from my first pull that I don't care whether
CMAKE_SYSTEM_NAME
orSWIFT_HOST_VARIANT_SDK
is used in this repo, but one has to be chosen and used consistently and exclusively. Since this repo is currently a muddled mess, usingSWIFT_HOST_VARIANT_SDK
heavily but also usingCMAKE_SYSTEM_NAME
for a minority of checks, I'm switching this to the more well-supportedSWIFT_HOST_VARIANT_SDK
, which is the only one supported bybuild-script
. As I detailed above, checkingSWIFT_HOST_VARIANT_SDK
here also works for platforms like Windows that only setCMAKE_SYSTEM_NAME
, as the Windows CI run for this pull passing demonstrated.