Revert "Revert "[android] Update to NDK 23""

[compnerd]

Reverts #39791

[compnerd]

@buttaface - CMake documentation also indicates that for cross-compiling to android, you should set CMAKE_SYSTEM_NAME to Android (and the same should apply for natively building as well). See https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-android. Is there a reason that wouldn't work?

[finagolfin]

Yes, this Swift compiler repo largely doesn't conform to CMake cross-compilation. Instead, it uses its own host variable SWIFT_HOST_VARIANT_SDK, which is always passed in by build-script, and if someone doesn't pass in SWIFT_HOST_VARIANT_SDK but passes in CMAKE_SYSTEM_NAME instead, it uses CMAKE_SYSTEM_NAME to set SWIFT_HOST_VARIANT_SDK and then uses the latter from then on. There are a few places where CMAKE_SYSTEM_NAME is incorrectly used also, but it is mostly SWIFT_HOST_VARIANT_SDK.

That is why I suggested that you pass in SWIFT_HOST_VARIANT_SDK on the Windows CI also, rather than depending on the translation in that second link.

Alternatives would be to reorder this linker configuration after the translation or vice versa, but I don't know if such reordering would break other things.

[compnerd]

The changes needed to remove the use of the variable are beyond the authority that I have to enact. The current mixture of build, host, and target content means that a third variable is required. This will be required as long as the standard library is part of the build unfortunately. However, the tools should be conforming to the CMake cross-compilation options, and places which are not are a bug. In fact, @gottesmm has fixed a few of these before, and I have as well. If you have cases currently which do not conform to that, please let me know, and we can fix those. But, we really do want to ensure that we get this correct so that we can take advantage of the LLVM build.

The reason that the SWIFT_HOST_VARIANT_SDK is set to the value of CMAKE_SYSTEM_NAME is because it assumes that you want to build the stdlib for the same host as the tools.

There are cases where SWIFT_HOST_SDK_VARIANT was previously used incorrectly and those have been fixed which is why the Windows builds do not set them - they are building without the stdlib.

As to your idea of sinking the SWIFT_USE_LINKER further down, I don't think that moving it is a problem per se, as in it shouldn't break anything. However, the reason that it is currently early is because it places all the options in a single location so that you can scan through them rather than have to hunt through the entire file. Given that the other platforms are using the correct CMAKE_SYSTEM_NAME variable, I wonder if there is anything to be gained from making Android different here.

[finagolfin]

The changes needed to remove the use of the variable are beyond the authority that I have to enact.

Not sure what you're referring to, I didn't mention removing any variable.

However, the tools should be conforming to the CMake cross-compilation options, and places which are not are a bug.

No, this Swift compiler repo explicitly rejects using traditional CMake cross-compilation with CMAKE_SYSTEM_NAME and only uses SWIFT_HOST_VARIANT_SDK instead. That is not a bug, that is the explicit design from the beginning.

Consider this CMake file that sets the build flags for the host tools: it checks SWIFT_HOST_VARIANT_SDK dozens of times and CMAKE_SYSTEM_NAME only once.

the reason that it is currently early is because it places all the options in a single location so that you can scan through them rather than have to hunt through the entire file.

Yes, I was leery of moving it for that reason too.

Given that the other platforms are using the correct CMAKE_SYSTEM_NAME variable, I wonder if there is anything to be gained from making Android different here.

I don't think you are too familiar with the current CMake config for this repo: by using SWIFT_HOST_VARIANT_SDK for Android, I am following the standard practice for this repo. By not passing in SWIFT_HOST_VARIANT_SDK on the Windows CI, you are the one doing something different, see the linked file for example.

[finagolfin]

Btw, there is work underway to switch to CMake toolchain files, which use CMAKE_SYSTEM_NAME instead, but it is currently only used to build the separate cmark repo. I think the plan is to switch this compiler repo over eventually, but that will take months if not a year.

[drexin]

I agree with @compnerd. SWIFT_HOST_VARIANT_SDK is not even set before https://github.com/apple/swift/blob/main/CMakeLists.txt#L718-L719. Passing in SWIFT_HOST_VARIANT_ARCH is optional - as this block clearly shows: https://github.com/apple/swift/blob/main/CMakeLists.txt#L684-L716 - but CMAKE_SYSTEM_NAME has to be set properly. Regardless of what is being done today in build-script, the cmake is what's important and we are certainly aiming to move towards using the standard cmake facilities in the future, so it would be great to not make changes that are incompatible with that goal.

[finagolfin]

I agree with @compnerd.

It is not clear what you agree with. He has made multiple statements, some of them deeply wrong, while you bring up some different points of when variables are set.

SWIFT_HOST_VARIANT_SDK is not even set before https://github.com/apple/swift/blob/main/CMakeLists.txt#L718-L719.

Yes, but as I noted above, it is always passed in by build-script, so it is always set long before that. That is why this pull passed the CI on both macOS and Linux. The only reason it fails on the Windows CI is that Saleem is not following the standard set on those other OS's by not passing in SWIFT_HOST_VARIANT_SDK himself, when not using build-script on Windows.

Passing in SWIFT_HOST_VARIANT_ARCH is optional - as this block clearly shows: https://github.com/apple/swift/blob/main/CMakeLists.txt#L684-L716 - but CMAKE_SYSTEM_NAME has to be set properly.

No, build-script always sets SWIFT_HOST_VARIANT_ARCH too, so that block is always bypassed. As for CMAKE_SYSTEM_NAME, CMake always sets that variable to the build host, which you can override when cross-compiling, but this repo largely doesn't use it.

Regardless of what is being done today in build-script, the cmake is what's important and we are certainly aiming to move towards using the standard cmake facilities in the future, so it would be great to not make changes that are incompatible with that goal.

That may be the plan, but it is not how this repo works today. The standard is set by how build-script works today, I don't see why the Windows CI should work differently. Since this issue is easily fixed by Saleem simply setting SWIFT_HOST_VARIANT_SDK to Windows on the Windows CI, as he does for dozens of other variables to match what build-script does, the easiest way to fix this is for him to add SWIFT_HOST_VARIANT_SDK too.

As discussed on that earlier pull where he reverted this, the fundamental problem is that there are two ways to cross-compile the Swift host tools, which is what I do for Android, either by setting CMAKE_SYSTEM_NAME or by setting SWIFT_HOST_VARIANT_SDK. Since the current build-script only sets SWIFT_HOST_VARIANT_SDK when cross-compiling this Swift compiler repo- for example, it doesn't set CMAKE_SYSTEM_NAME when cross-compiling the host tools for Mac arm64, as I keep repeatedly pointing out in my links- and this repo's CMake config uses SWIFT_HOST_VARIANT_SDK almost everywhere internally, it would be better if Saleem also set SWIFT_HOST_VARIANT_SDK on the Windows CI.

What you are "aiming to move towards" is irrelevant, it is not how this repo is built today. If this repo ever moves to using CMAKE_SYSTEM_NAME instead, we will have to change literally a hundred or more uses of SWIFT_HOST_VARIANT_SDK in this repo: this one extra change on the Windows CI is not going to make a difference.

[finagolfin]

As I mentioned earlier, an alternative would be to move this linker config after SWIFT_HOST_VARIANT_SDK is set by default:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c780c0e544f..fae6e6e06ce 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -175,21 +175,6 @@ set(SWIFT_COMPILER_VERSION "" CACHE STRING
 set(CLANG_COMPILER_VERSION "" CACHE STRING
     "The internal version of the Clang compiler")
 
-# Which default linker to use. Prefer LLVM_USE_LINKER if it set, otherwise use
-# our own defaults. This should only be possible in a unified (not stand alone)
-# build environment.
-if(LLVM_USE_LINKER)
-  set(SWIFT_USE_LINKER_default "${LLVM_USE_LINKER}")
-elseif(CMAKE_SYSTEM_NAME STREQUAL Windows AND NOT CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
-  set(SWIFT_USE_LINKER_default "lld")
-elseif(CMAKE_SYSTEM_NAME STREQUAL Darwin)
-  set(SWIFT_USE_LINKER_default "")
-else()
-  set(SWIFT_USE_LINKER_default "gold")
-endif()
-set(SWIFT_USE_LINKER ${SWIFT_USE_LINKER_default} CACHE STRING
-    "Build Swift with a non-default linker")
-
 option(SWIFT_DISABLE_DEAD_STRIPPING
       "Turn off Darwin-specific dead stripping for Swift host tools." FALSE)
 
@@ -720,6 +705,23 @@ set(SWIFT_HOST_VARIANT_SDK "${SWIFT_HOST_VARIANT_SDK_default}" CACHE STRING
 set(SWIFT_HOST_VARIANT_ARCH "${SWIFT_HOST_VARIANT_ARCH_default}" CACHE STRING
     "Deployment arch for Swift host tools (the compiler).")
 
+# Which default linker to use. Prefer LLVM_USE_LINKER if it set, otherwise use
+# our own defaults. This should only be possible in a unified (not stand alone)
+# build environment.
+if(LLVM_USE_LINKER)
+  set(SWIFT_USE_LINKER_default "${LLVM_USE_LINKER}")
+elseif(${SWIFT_HOST_VARIANT_SDK} STREQUAL ANDROID)
+  set(SWIFT_USE_LINKER_default "lld")
+elseif(CMAKE_SYSTEM_NAME STREQUAL Windows AND NOT CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
+  set(SWIFT_USE_LINKER_default "lld")
+elseif(CMAKE_SYSTEM_NAME STREQUAL Darwin)
+  set(SWIFT_USE_LINKER_default "")
+else()
+  set(SWIFT_USE_LINKER_default "gold")
+endif()
+set(SWIFT_USE_LINKER ${SWIFT_USE_LINKER_default} CACHE STRING
+    "Build Swift with a non-default linker")
+
 #
 # Enable additional warnings.
 #

Just like setting SWIFT_HOST_VARIANT_SDK on the Windows CI, I think this will also fix your Windows CI issue, so feel free to apply this patch with my reverted pull instead.

[drexin]

Again, what build-script does, or does not pass in is completely irrelevant for how the cmake works. The cmake allows SWIFT_HOST_VARIANT_SDK to be passed in, but does not require it. Your PR, as it stands, would change that and require SWIFT_HOST_VARIANT_SDK to always be passed in.

I think a feasible solution would be to pass in LLVM_USE_LINKER for the Android build. Would that work for you?

[finagolfin]

Again, what build-script does, or does not pass in is completely irrelevant for how the cmake works.

No, that is the "standard" for what this repo's CMake config expects, at least on the platforms where it is mostly used, Darwin and linux. I don't think it's a good idea for the Windows platform to go off in a different direction.

The cmake allows SWIFT_HOST_VARIANT_SDK to be passed in, but does not require it.

It absolutely requires that it be set since it largely uses SWIFT_HOST_VARIANT_SDK internally. It does provide a fallback where you could pass in CMAKE_SYSTEM_NAME and then translates that to SWIFT_HOST_VARIANT_SDK, as I already pointed out, but given that build-script doesn't pass in CMAKE_SYSTEM_NAME, it's not a good idea for Windows or other platforms to do that. I have been pointing this out for two and a half years now, but nobody ever listens.

Your PR, as it stands, would change that and require SWIFT_HOST_VARIANT_SDK to always be passed in.

Which is not a problem since linux, Android, and all Darwin platforms like macOS and iOS pass SWIFT_HOST_VARIANT_SDK in. Currently, only Saleem's Windows CI doesn't.

I think a feasible solution would be to pass in LLVM_USE_LINKER for the Android build. Would that work for you?

You're suggesting I add that to build-script instead? That's another possibility, but I'd prefer that it's documented here instead.

I have not heard a single reason why Saleem won't just add this variable to his Windows CI, as he does for dozens of other variables when configuring the CMake build for Windows. If that's difficult for some still unstated reason, the patch above changing the order should work too.

[drexin]

Again, what build-script does, or does not pass in is completely irrelevant for how the cmake works.

No, that is the "standard" for what this repo's CMake config expects, at least on the platforms where it is mostly used, Darwin and linux. I don't think it's a good idea for the Windows platform to go off in a different direction.

What the cmake config expects is purely defined in the cmake and not by how it is being invoked. The cmake build sets the variable at a later point and even clearly states that it falls back to CMAKE_SYSTEM_NAME if it's not passed in. This should make it obvious that the cmake build does not expect it to be passed in and there is no reason to change that.

The cmake allows SWIFT_HOST_VARIANT_SDK to be passed in, but does not require it.

It absolutely requires that it be set since it largely uses SWIFT_HOST_VARIANT_SDK internally. It does provide a fallback where you could pass in CMAKE_SYSTEM_NAME and then translates that to SWIFT_HOST_VARIANT_SDK, as I already pointed out, but given that build-script doesn't pass in CMAKE_SYSTEM_NAME, it's not a good idea for Windows or other platforms to do that. I have been pointing this out for two and a half years now, but nobody ever listens.

It will be set in the cmake itself. My statement was that it is not required to be passed in.

Your PR, as it stands, would change that and require SWIFT_HOST_VARIANT_SDK to always be passed in.

Which is not a problem since linux, Android, and all Darwin platforms like macOS and iOS pass SWIFT_HOST_VARIANT_SDK in. Currently, only Saleem's Windows CI doesn't.

Again, how the build is invoked from one particular script does not define how everything should call it. The truth is in the cmake.

I think a feasible solution would be to pass in LLVM_USE_LINKER for the Android build. Would that work for you?

You're suggesting I add that to build-script instead? That's another possibility, but I'd prefer that it's documented here instead.

I have not heard a single reason why Saleem won't just add this variable to his Windows CI, as he does for dozens of other variables when configuring the CMake build for Windows. If that's difficult for some still unstated reason, the patch above changing the order should work too.

There have been multiple reasons stated, that you chose to ignore and/or call wrong. The reason not to change the windows build is that it invokes the cmake as it is expected. The build works without your changes, it doesn't work with your changes. We have given you multiple options to choose from that would make your build work, without touching or breaking the Windows build.

[finagolfin]

We are going in circles here, let me just address the last bit.

There have been multiple reasons stated, that you chose to ignore and/or call wrong.

Please point me to where a single explanation was given for "why Saleem won't just add this variable to his Windows CI," either on this pull or the previous #39791. Just link to it, since you claim it was given. I can't call the reason wrong if I said it was never given: rather I called many other statements of his wrong, as I did for some of yours.

We have given you multiple options to choose from that would make your build work, without touching or breaking the Windows build.

I don't think Saleem ever bothered to give an option, you gave one. That's not multiple.

I gave 2-3 options, including adding SWIFT_HOST_VARIANT_SDK to the Windows CI or the patch pasted above, neither of which I've heard a substantive argument against.

[drexin]

We are going in circles here, let me just address the last bit.

There have been multiple reasons stated, that you chose to ignore and/or call wrong.

Please point me to where a single explanation was given for "why Saleem won't just add this variable to his Windows CI," either on this pull or the previous #39791. Just link to it, since you claim it was given. I can't call the reason wrong if I said it was never given: rather I called many other statements of his wrong, as I did for some of yours.

Both @compnerd and I have given you reasons why we don't think this change is correct. The same reasons explain why we don't think changing the Windows build to match your expectations of how the cmake should be invoked is appropriate. I'm sorry if that wasn't clear from the context, but I explicitly pointed it out in my previous post.

We have given you multiple options to choose from that would make your build work, without touching or breaking the Windows build.

I don't think Saleem ever bothered to give an option, you gave one. That's not multiple.

I gave 2-3 options, including adding SWIFT_HOST_VARIANT_SDK to the Windows CI or the patch pasted above, neither of which I've heard a substantive argument against.

At least two options were given. Using CMAKE_SYSTEM_NAME, or using LLVM_USE_LINKER. If neither of those work, I would like to hear the specific reasons why they don't work.

[finagolfin]

Both @compnerd and I have given you reasons why we don't think this change is correct.

In that case, I've exhaustively argued the opposite: using CMAKE_SYSTEM_NAME to set the linker is incorrect in this Swift compiler repo. Honestly, when I pointed out that every single build flag for the host tools but one is set by SWIFT_HOST_VARIANT_SDK, that was the mic drop for me. I don't know why the two of you are even arguing this silly point after that.

Using CMAKE_SYSTEM, or using LLVM_USE_LINKER. If neither of those work, I would like to hear the specific reasons why they don't work.

I'll stay away from the silly reasons about being incorrect that you gave and point out that practically, I use the build-script to cross-compile and it only passes in SWIFT_HOST_VARIANT_SDK, either when natively compiling or cross-compiling this repo. So it makes no sense to pass in CMAKE_SYSTEM_NAME on top of that when that is not how the Swift compiler build with build-script works.

As for LLVM_USE_LINKER, I already wrote you above: "I'd prefer that it's documented here instead" with all the other platform linkers.

Now, I don't think you can say I haven't given you reasons not to do those two, even if you disagree with them.

What about my patch reordering the variables above? I have not heard a peep out of you about that option. We are spending far too much time arguing about what is really a minor issue.