[android] Add more changes to build the compiler #69538

finagolfin · 2023-10-31T15:58:19Z

I just built the Oct. 14 trunk source snapshot natively in the Termux app on Android and cross-compiled the Oct. 27 trunk snapshot from linux to Android using these changes, finagolfin/termux-packages@374754b06.

@drodriguez, please review.

finagolfin · 2023-10-31T16:02:44Z

CMakeLists.txt

    # 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.


Pure bridging allows me to work around #60272 for Android.

finagolfin · 2023-10-31T16:04:28Z

CMakeLists.txt

@@ -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))


CMAKE_SYSTEM_NAME is not set by build-script when cross-compiling, this repo uses SWIFT_HOST_VARIANT_SDK instead.

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:

if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows" OR "${SWIFT_HOST_VARIANT_SDK}" STREQUAL "ANDROID" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8))

Most of the time CMAKE_SYSTEM_NAME is set by CMake. Scripts only need to provide a value explicitly when cross-compiling

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 set CMAKE_SYSTEM_NAME anywhere in build-script.

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.

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 replaces CMAKE_SYSTEM_NAME as the host the compiler should be built for, at least in this repo.

There's at least one check for CMAKE_SYSTEM_NAME = Android. Isn't that something that can be used?

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.

build-script is just the way the Darwin/Linux CI uses to build Swift, but Windows CI builds, in particular, do not use build-script (it is impossible). Other people use unified builds with LLVM and other ways of kick starting the build and try to avoid build-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.

build-script is just the way the Darwin/Linux CI uses to build Swift, but Windows CI builds, in particular, do not use build-script (it is impossible). Other people use unified builds with LLVM and other ways of kick starting the build and try to avoid build-script.

I'm well aware, as I have merged changes to those Windows batch files in this repo before.

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.

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 right SWIFT_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. 😉

Can I ask you please to not change this?

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.

One can provide different values to CMAKE_SYSTEM_NAME and SWIFT_HOST_VARIANT_SDK.

Sure, and then this compiler build will not work.

The reason that "override" you mention is there is because build-script passes in SWIFT_HOST_VARIANT_SDK to this CMake build, as I linked above. It does not use CMAKE_SYSTEM_NAME. For those who insist on using CMAKE_SYSTEM_NAME instead outside of build-script, the translation to SWIFT_HOST_VARIANT_SDK that I linked you above is used.

Now, I can see how the use of both CMAKE_SYSTEM_NAME and SWIFT_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 of SWIFT_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 a CMAKE_SYSTEM_NAME when cross-compiling, so it is safer to check SWIFT_HOST_VARIANT_SDK on this line then. Without build-script, one can do all kinds of nonsense, as you noted, but if you set CMAKE_SYSTEM_NAME alone, the translation above ensures that SWIFT_HOST_VARIANT_SDK is properly defined, so again it is safer to check SWIFT_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.

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.

@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.

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.

Consider the builds for the macOS and the fat runtimes on Apple platforms. These can be different (at least currently).

Every Darwin OS sets SWIFT_HOST_VARIANT_SDK as I'm checking here- see the next 100 lines after that link too- none set CMAKE_SYSTEM_NAME.

I think that we should prefer the CMake handling for the cross-compilation mechanism

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.

and is something that might be a possibility in the future.

It hasn't happened in five years, I don't think it will happen anytime soon.

We shouldn't make this harder for such a future refactoring.

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 set CMAKE_SYSTEM_NAME differently when cross-compiling, which is every platform using build-script, ie almost every Swift platform.

As I emphasized from my first pull, you can't check either CMAKE_SYSTEM_NAME or SWIFT_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 in CMAKE_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 Darwinfor 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 with CMAKE_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 or SWIFT_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, using SWIFT_HOST_VARIANT_SDK heavily but also using CMAKE_SYSTEM_NAME for a minority of checks, I'm switching this to the more well-supported SWIFT_HOST_VARIANT_SDK, which is the only one supported by build-script. As I detailed above, checking SWIFT_HOST_VARIANT_SDK here also works for platforms like Windows that only set CMAKE_SYSTEM_NAME, as the Windows CI run for this pull passing demonstrated.

finagolfin · 2023-10-31T16:06:35Z

CMakeLists.txt

@@ -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")


Android does not need this override, for example, I use the CROSSCOMPILE mode when cross-compiling the compiler from linux to Android.

finagolfin · 2023-10-31T16:09:19Z

SwiftCompilerSources/Sources/Basic/Utils.swift

+  public typealias FILEPointer = OpaquePointer
+#else
+  public typealias FILEPointer = UnsafeMutablePointer<FILE>
+#endif


I took this from my similar pull for swift-tools-support-core, which Max later used for Musl also.

finagolfin · 2023-10-31T16:13:55Z

cmake/modules/AddSwift.cmake

@@ -607,6 +607,9 @@ function(_add_swift_runtime_link_flags target relpath_to_lib_dir bootstrapping)
        endif()
      endif()
    endif()
+    if(SWIFT_USE_LINKER STREQUAL "lld")
+      target_link_options(${target} PRIVATE "SHELL:-Xlinker -z -Xlinker nostart-stop-gc")


This is needed since lld 13 on ELF platforms a couple years ago, #60406, which linking the new SwiftSyntax library here triggers.

finagolfin · 2023-10-31T16:21:31Z

lib/Demangling/Errors.cpp

@@ -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__)


Pushing these compilation errors to the system log is not useful, certainly not on Termux.

utils/swift_build_support/swift_build_support/products/earlyswiftdriver.py

eeckstein · 2023-11-02T14:34:22Z

CMakeLists.txt

@@ -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}")


drodriguez · 2023-11-02T16:20:33Z

CMakeLists.txt

@@ -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))


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:

if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows" OR "${SWIFT_HOST_VARIANT_SDK}" STREQUAL "ANDROID" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8))

drodriguez · 2023-11-02T16:33:21Z

lib/Driver/UnixToolChains.cpp

+#if defined(__TERMUX__)
+  bool android_rpath = true;
+#else
+  bool android_rpath = !T.isAndroid();
+#endif


This change is modifying more than it seems to: any Swift compiler build for Termux will use true, even when cross-compiling to pure Android.

Yep, that is what I mean for it to do. Since this is only the default, one can always supply -no-toolchain-stdlib-rpath and override this default on Termux if wanted.

finagolfin · 2023-11-03T14:39:37Z

@bnbarham, calling in a CI run. 😄

bnbarham · 2023-11-03T21:28:45Z

@swift-ci please test

bnbarham · 2023-11-03T21:29:03Z

@swift-ci please build toolchain

finagolfin · 2023-11-04T06:07:49Z

Alright, linux and mac CI passed. Windows CI is broken right now, but both the CI and toolchain build got to the compiler validation suite before failing, showing this change didn't break building the compiler and I checked the Windows CI log to make sure it is still choosing the pure C++ bridging mode instead. Mac toolchain build failure appears spurious.

@drodriguez, if you would just approve, I'll make sure this passes the CI in the coming week.

finagolfin · 2023-11-06T16:59:14Z

Rebased and simply renamed a variable.

bnbarham · 2023-11-06T17:05:35Z

@swift-ci please test

finagolfin · 2023-11-07T08:41:20Z

Linux and mac CI passed again, Windows CI failed at source checkout.

finagolfin · 2023-11-07T16:19:32Z

@egorzhdan, please schedule a Windows CI run, should pass now.

finagolfin · 2023-11-10T11:18:16Z

@al45tair, please run the Windows CI on this.

glessard · 2023-11-10T19:56:51Z

@swift-ci please test windows platform

finagolfin · 2023-11-11T11:46:24Z

@drodriguez, ready for review, passed all CI. It works fine on the Windows CI too, with pure bridging still set there, so changing that CMake variable you were worried about had no effect.

finagolfin · 2023-11-28T04:54:40Z

@drodriguez, this passed all CI, let me know what you think.

finagolfin · 2023-12-06T17:14:55Z

@bnbarham, can you run CI again and review? I think Daniel is too busy to get to this.

Other than the single lld change, which benefits all ELF platforms using lld, this pull only affects the Termux app for Android, including the CMake variable change discussed above which worked fine on the Windows CI before break, so should be fine to merge.

bnbarham · 2023-12-06T17:28:11Z

@swift-ci please test

finagolfin · 2023-12-12T05:51:23Z

@bnbarham, would you review and merge? I've addressed all the concerns about the single CMake variable change, including showing it works fine on the Windows CI, and the rest of the pull other than the lld change only affects building the compiler for the Termux Android app, so should be fine.

finagolfin · 2023-12-15T12:23:39Z

@al45tair, would you review this small pull?

bnbarham · 2023-12-15T18:57:17Z

As far as I can tell, the main issue here was with the change that involves Windows. Given it passes there, I'll merge for now. If any issues come up, we can revert.

…Termux This is the equivalent of the already-merged C++ Driver change in swiftlang/swift#69538.

finagolfin requested review from a team, artemcm and eeckstein as code owners October 31, 2023 15:58

finagolfin commented Oct 31, 2023

View reviewed changes

finagolfin force-pushed the android branch from c01fc84 to 0ad575a Compare October 31, 2023 16:16

finagolfin commented Oct 31, 2023

View reviewed changes

utils/swift_build_support/swift_build_support/products/earlyswiftdriver.py Outdated Show resolved Hide resolved

finagolfin force-pushed the android branch from 0ad575a to 657d331 Compare November 2, 2023 12:48

eeckstein reviewed Nov 2, 2023

View reviewed changes

drodriguez reviewed Nov 2, 2023

View reviewed changes

[android] Add more changes to build the compiler

bf137cb

finagolfin force-pushed the android branch from 657d331 to bf137cb Compare November 6, 2023 16:25

bnbarham merged commit 406d533 into swiftlang:main Dec 15, 2023
5 checks passed

finagolfin deleted the android branch December 15, 2023 19:20

finagolfin added a commit to finagolfin/swift-driver that referenced this pull request Mar 13, 2024

Android: add the stdlib RPATH when natively building on Android with …

faea1fc

…Termux This is the equivalent of the already-merged C++ Driver change in swiftlang/swift#69538.

finagolfin mentioned this pull request Mar 13, 2024

Android: add the stdlib RPATH when natively building on Android with Termux swiftlang/swift-driver#1561

Merged

artemcm pushed a commit to swiftlang/swift-driver that referenced this pull request May 24, 2024

Android: add the stdlib RPATH when natively building on Android with …

1e7ed77

…Termux This is the equivalent of the already-merged C++ Driver change in swiftlang/swift#69538.

[android] Add more changes to build the compiler #69538

[android] Add more changes to build the compiler #69538

Conversation

finagolfin commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Nov 3, 2023

bnbarham commented Nov 3, 2023

bnbarham commented Nov 3, 2023

finagolfin commented Nov 4, 2023

finagolfin commented Nov 6, 2023

bnbarham commented Nov 6, 2023

finagolfin commented Nov 7, 2023

finagolfin commented Nov 7, 2023

finagolfin commented Nov 10, 2023

glessard commented Nov 10, 2023

finagolfin commented Nov 11, 2023

finagolfin commented Nov 28, 2023

finagolfin commented Dec 6, 2023

bnbarham commented Dec 6, 2023

finagolfin commented Dec 12, 2023

finagolfin commented Dec 15, 2023

bnbarham commented Dec 15, 2023

finagolfin commented Oct 31, 2023 •

edited

Loading

finagolfin Dec 7, 2023 •

edited

Loading