Revert "[android] Update to NDK 23"

[compnerd]

Reverts #39045

This broke the Windows CI due to incorrect testing.

[compnerd]

@swift-ci please smoke test

[finagolfin]

How did this affect Windows? Everything in this patch is scoped to Android.

[etcwilde]

https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/6846/

21:24:35 CMake Error at CMakeLists.txt:183 (elseif):
21:24:35   given arguments:
21:24:35 
21:24:35     "STREQUAL" "ANDROID"

[compnerd]

Given that the dispatch changes still need some alteration (I agree that dispatch should not be including bits/stdatomic.h), I think its better to revert than to fix.

[finagolfin]

OK, but that line passed fine on both the Linux and Mac CIs, and I guess the Windows CI was not run by default for the pull this reverts. What version of CMake is that that doesn't support that common usage?

[etcwilde]

Given that it's just showing the two parameters to the elseif, is the variable SWIFT_HOST_VARIANT_SDK set on Windows?

[finagolfin]

is the variable SWIFT_HOST_VARIANT_SDK set on Windows?

Oh yeah, that must be it. I assumed SWIFT_HOST_VARIANT_SDK would be set by build-script, which the Windows CI is not using here. Instead, it must be depending on the automatic recognition later in this CMake file.

A simple way to fix this would be for you to set SWIFT_HOST_VARIANT_SDK explicitly to Windows on your CI instead. This goes back to the issue I've been raising for years now, that this repo inconsistently mixes CMAKE_SYSTEM_NAME and SWIFT_HOST_VARIANT_SDK to signify the host to build for. I tried to use the latter correctly here, instead of the incorrect usage surrounding it, and while that works for build-script, I guess it doesn't for such manual usage that doesn't conform to how build-script does it.

[finagolfin]

Given that the dispatch changes still need some alteration (I agree that dispatch should not be including bits/stdatomic.h)

What do you suggest instead, given that I noted that is a workaround for src/internal.h not being compliant with the C++ standard?

[etcwilde]

Have you tried...

#if defined(__cplusplus)
#include
#else
#include <stdatomic.h>
#endif
One should not access the bits/ headers outside the standard headers. They are private details of the implementation.

As was commented on in the lib dispatch PR , using things in bits is not good. Using the atomic header is the correct approach.

[compnerd]

Use of <atomic> in C++ seems reasonable to me, does that not work?

[finagolfin]

As was commented on in the lib dispatch PR , using things in bits is not good. Using the atomic header is the correct approach.

As I responded there in August, that didn't work either, because it doesn't change the fundamental issue that you're invoking C++ declarations inside an extern C block.

The real problem is in src/internal.h not following the C++ standard, but I'm not sure how that's best fixed.

[compnerd]

There's a horrible thing that comes to mind - and I'd prefer to avoid it if at all possible - we could wrap the header via include_next and convert things around either with macros or other things. If we can work out another workaround, I think that would be preferable. But I think that we have some more potent tools available if absolutely required.

[compnerd]

@swift-ci please test Windows platform

[finagolfin]

we could wrap the header via include_next and convert things around either with macros or other things.

I don't think that would work either, took the discussion there.

[finagolfin]

Getting back to this pull, what about the fix I mentioned earlier: "A simple way to fix this would be for you to set SWIFT_HOST_VARIANT_SDK explicitly to Windows on your CI instead."

[compnerd]

That doesn't work for Windows - the split is important for the split build that Windows does. See the above point about fixing this would be possible but given that the dispatch changes aren't ready yet, it seems better to just revert for the time being.

[finagolfin]

That doesn't work for Windows - the split is important for the split build that Windows does.

Not sure what you mean, SWIFT_HOST_VARIANT_SDK is always set by default later on in that file. build-script simply passes it in earlier instead, so you could do the same, just as you're doing for a couple dozen other variables when you invoke CMake manually on the Windows CI.

given that the dispatch changes aren't ready yet

Why would that be relevant? Merging that would fix the Android CI, but reverting this would leave the Android CI broken anyway, because of the recent LLVM rebranch.

[compnerd]

I agree with you that the current state is less than desirable. There is a fairly simple mapping:
CMAKE_SYSTEM_NAME: all host tools
CMAKE_HOST_SYSTEM_NAME: all build tools
SWIFT_HOST_VARIANT_SDK: everything in stdlib.

Given that the CI has been broken for >12 hours now, I'd like to merge this for the time being. We can revert the revert and work on repairing the android builds subsequently. This is currently preventing further tests, including those related to some of the deferred rebranch issues.

build-script doesn't work for windows - it assumes bash, which is not reasonable. The equivalent is build-windows-toolchain.bat which only configures paths outside of the experimental features (the rest is via cmake caches).

[finagolfin]

There is a fairly simple mapping:
CMAKE_SYSTEM_NAME: all host tools
CMAKE_HOST_SYSTEM_NAME: all build tools
SWIFT_HOST_VARIANT_SDK: everything in stdlib.

I don't think that is the way it works now- for example, when the host tools are cross-compiled to Apple Silicon, build-script only passes in SWIFT_HOST_VARIANT_SDK not CMAKE_SYSTEM_NAME- maybe that changes eventually once @gottesmm moves the build to using CMake toolchain files.

Given that the CI has been broken for >12 hours now, I'd like to merge this for the time being. We can revert the revert and work on repairing the android builds subsequently.

Alright, understood, please go ahead and revert.

build-script doesn't work for windows - it assumes bash, which is not reasonable.

That's not what I suggested. You are currently passing a lot of variables on the Windows CI:

cmake    -B "T:\swift"    -G Ninja    -DCMAKE_BUILD_TYPE=Release    -DCMAKE_C_COMPILER=cl    -DCMAKE_CXX_COMPILER=cl    -DCMAKE_MT=mt    -DCMAKE_INSTALL_PREFIX:PATH=T:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr    -DClang_DIR:PATH=T:\llvm\lib\cmake\clang    -DSWIFT_PATH_TO_CMARK_BUILD:PATH=T:\cmark    -DSWIFT_PATH_TO_CMARK_SOURCE:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\cmark    -DSWIFT_PATH_TO_LIBDISPATCH_SOURCE:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\swift-corelibs-libdispatch    -DLLVM_DIR:PATH=T:\llvm\lib\cmake\llvm    -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=ON    -DSWIFT_INCLUDE_DOCS:BOOL=NO    -DSWIFT_WINDOWS_x86_64_ICU_UC_INCLUDE:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\icu-64_2\include\unicode    -DSWIFT_WINDOWS_x86_64_ICU_UC:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\icu-64_2\lib64\icuuc.lib    -DSWIFT_WINDOWS_x86_64_ICU_I18N_INCLUDE:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\icu-64_2\include    -DSWIFT_WINDOWS_x86_64_ICU_I18N:PATH=C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\icu-64_2\lib64\icuin.lib    -DSWIFT_BUILD_DYNAMIC_STDLIB:BOOL=YES    -DSWIFT_BUILD_DYNAMIC_SDK_OVERLAY:BOOL=YES    -DSWIFT_BUILD_STATIC_STDLIB:BOOL=NO    -DSWIFT_BUILD_STATIC_SDK_OVERLAY:BOOL=NO    -DLLVM_INSTALL_TOOLCHAIN_ONLY:BOOL=YES    -DSWIFT_BUILD_SOURCEKIT:BOOL=YES    -DSWIFT_ENABLE_SOURCEKIT_TESTS:BOOL=YES    -DSWIFT_ENABLE_EXPERIMENTAL_CONCURRENCY=YES    -DSWIFT_ENABLE_EXPERIMENTAL_DISTRIBUTED=YES    -DSWIFT_ENABLE_EXPERIMENTAL_DIFFERENTIABLE_PROGRAMMING=YES    -DSWIFT_INSTALL_COMPONENTS="autolink-driver;compiler;clang-resource-dir-symlink;stdlib;sdk-overlay;editor-integration;tools;sourcekit-inproc;swift-remote-mirror;swift-remote-mirror-headers"    -DSWIFT_PARALLEL_LINK_JOBS=8    -DPYTHON_EXECUTABLE:PATH="C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python37_64"\python.exe    -DCMAKE_CXX_FLAGS:STRING="/GS- /Oy"    -DCMAKE_EXE_LINKER_FLAGS:STRING=/INCREMENTAL:NO    -DCMAKE_SHARED_LINKER_FLAGS:STRING=/INCREMENTAL:NO    -DSWIFT_LIT_ARGS="--time-tests"    -S "C:\Users\swift-ci\jenkins\workspace\oss-swift-windows-x86_64-vs2019\swift"   || (exit /b )

I'm simply suggesting you add this one to that list.

Anyway, please go ahead and revert, we can hash this out later.

[drodriguez]

@compnerd: sorry for the breakage. It seems that "test and merge" didn't kick off the Windows tests (not even as non-blocking). I would try not to use it in the future.