android / ndk

The Android Native Development Kit
1.99k stars 257 forks source link

[BUG] r22 appears to break cmake compiler and OS detection on Windows #1427

Closed grendello closed 3 years ago

grendello commented 3 years ago

Description

Xamarin.Android has been using Android SDK's cmake for quite a while now and until NDK r22 everything has worked as expected. With r22 the Unix (Linux and macOS) builds still work fine, however the Windows build breaks because cmake is not able to properly detect the C/C++ compilers (please see the attached error and output logs). Not only that, it appears that the WIN32 cmake variable is not set at all, instead MINGW is set.

We normally use cmake from the SDK (version 3.10.2), but I also tried with cmake 3.18 shipped with VS2019, to the same effect. Our CMakeLists.txt, PR to switch XA to NDK r22, and a sample invocation line:

C:\Users\AzDevOps\android-toolchain\sdk\cmake\3.10.2.4988404\bin\cmake --debug-output -GNinja -DCMAKE_MAKE_PROGRAM=C:\Users\AzDevOps\android-toolchain\sdk\cmake\3.10.2.4988404\bin\ninja -DANDROID_STL="none" -DANDROID_CPP_FEATURES="no-rtti no-e
xceptions" -DANDROID_TOOLCHAIN=clang -DCMAKE_TOOLCHAIN_FILE=C:\Users\AzDevOps\android-toolchain\ndk\build\cmake\android.toolchain.cmake -DANDROID_NDK=C:\Users\AzDevOps\android-toolchain\ndk -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DJDK_INCLUDE="C:\Users\AzDevOps\android-toolchain\jdk-11\include C:\Users\AzDevOps\android-toolchain\jdk-11\include\win32" -DMONO_PATH=C:\a\2\s\external\mono -DCONFIGURATION=Release -DCMAKE_BUILD_TYPE=Debug -DANDROID_NATIVE_API_LEVEL=16 -DANDROID_PLATFORM=android-16 -DANDROID_ABI=armeabi-v7a -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY=C:\a\2\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\lib\armeabi-v7a -DCMAKE_LIBRARY_OUTPUT_DIRECTORY=C:\a\2\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\lib\armeabi-v7a C:\a\2\s\src\monodroid\

The same CMakeList.txt (and the same cmake version) works fine with r21 on Windows

Environment Details

Not all of these will be relevant to every bug, but please provide as much information as you can.

CMakeError.log.txt CMakeOutput.log.txt

grendello commented 3 years ago

Just a note regarding the command above. I do know that r22 doesn't have support for API16 (at least the runtime libs for it are absent, clang wrapper scripts are still there), but it's not relevant in this case - the error appears to be unrelated at least. However, for API21 build cmake still mis-detects the system as MINGW. Please see the logs attached below - the libraries mentioned in the error log are used only if MINGW is detected.

CMakeError.log.txt CMakeOutput.log.txt

DanAlbert commented 3 years ago

Looks like CMake hasn't set the -target for Clang?

Compiler: C:/Users/AzDevOps/android-toolchain/ndk/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe 
Build flags: -g;-DANDROID;-fdata-sections;-ffunction-sections;-funwind-tables;-fstack-protector-strong;-no-canonical-prefixes;-D_FORTIFY_SOURCE=2;-march=armv7-a;-mthumb;-Wformat;-Werror=format-security;
Id flags:  

The output was:
1
clang: warning: argument unused during compilation: '-mthumb' [-Wunused-command-line-argument]
error: unknown target CPU 'armv7-a'
note: valid target CPU values are: nocona, core2, penryn, bonnell, atom, silvermont, slm, goldmont, goldmont-plus, tremont, nehalem, corei7, westmere, sandybridge, corei7-avx, ivybridge, core-avx-i, haswell, core-avx2, broadwell, skylake, skylake-avx512, skx, cascadelake, cooperlake, cannonlake, icelake-client, icelake-server, tigerlake, knl, knm, k8, athlon64, athlon-fx, opteron, k8-sse3, athlon64-sse3, opteron-sse3, amdfam10, barcelona, btver1, btver2, bdver1, bdver2, bdver3, bdver4, znver1, znver2, x86-64

Our QA will have tested Windows before we shipped though. We probably need a repro case here.

at least the runtime libs for it are absent

$ ls android-ndk-r22/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi/16/
crtbegin_dynamic.o
crtbegin_so.o
crtbegin_static.o
crtend_android.o
crtend_so.o
libandroid.so
libc++.a
libc.a
libcompiler_rt-extras.a
libc++.so
libc.so
libdl.a
libdl.so
libEGL.so
libGLESv1_CM.so
libGLESv2.so
libjnigraphics.so
liblog.so
libm.a
libm.so
libOpenMAXAL.so
libOpenSLES.so
libstdc++.a
libstdc++.so
libz.a
libz.so
DanAlbert commented 3 years ago

Assigning to hhb in case he has any ideas.

grendello commented 3 years ago

Looks like CMake hasn't set the -target for Clang?

It seems as if ARM support wasn't there at all, note this error:

error: unknown target CPU 'armv7-a'

And look at the list of supported CPUs, it contains only x86.

Compiler: C:/Users/AzDevOps/android-toolchain/ndk/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe 
Build flags: -g;-DANDROID;-fdata-sections;-ffunction-sections;-funwind-tables;-fstack-protector-strong;-no-canonical-prefixes;-D_FORTIFY_SOURCE=2;-march=armv7-a;-mthumb;-Wformat;-Werror=format-security;
Id flags:  

The output was:
1
clang: warning: argument unused during compilation: '-mthumb' [-Wunused-command-line-argument]
error: unknown target CPU 'armv7-a'
note: valid target CPU values are: nocona, core2, penryn, bonnell, atom, silvermont, slm, goldmont, goldmont-plus, tremont, nehalem, corei7, westmere, sandybridge, corei7-avx, ivybridge, core-avx-i, haswell, core-avx2, broadwell, skylake, skylake-avx512, skx, cascadelake, cooperlake, cannonlake, icelake-client, icelake-server, tigerlake, knl, knm, k8, athlon64, athlon-fx, opteron, k8-sse3, athlon64-sse3, opteron-sse3, amdfam10, barcelona, btver1, btver2, bdver1, bdver2, bdver3, bdver4, znver1, znver2, x86-64

Our QA will have tested Windows before we shipped though. We probably need a repro case here.

at least the runtime libs for it are absent

My bad, looked in a wrong directory (aarch64) :)

$ ls android-ndk-r22/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi/16/
crtbegin_dynamic.o
crtbegin_so.o
crtbegin_static.o
crtend_android.o
crtend_so.o
libandroid.so
libc++.a
libc.a
libcompiler_rt-extras.a
libc++.so
libc.so
libdl.a
libdl.so
libEGL.so
libGLESv1_CM.so
libGLESv2.so
libjnigraphics.so
liblog.so
libm.a
libm.so
libOpenMAXAL.so
libOpenSLES.so
libstdc++.a
libstdc++.so
libz.a
libz.so
grendello commented 3 years ago

I think the error: unknown target CPU 'armv7-a' error is a red herring and that, ultimately, the problem is the misdetection of the system as MINGW. Looking further at the CMakeOutput.log attached in the OP, it gets the correct compiler options:

Detecting C compiler ABI info compiled with the following output:
Change Dir: C:/Users/Grendel/vc/xamarin-android/src/monodroid/obj/Debug/armeabi-v7a-Debug/CMakeFiles/CMakeTmp

Run Build Command:"C:\Users\Grendel\android-toolchain\sdk\cmake\3.10.2.4988404\bin\ninja" "cmTC_22a78"
[1/2] Building C object CMakeFiles/cmTC_22a78.dir/CMakeCCompilerABI.c.o
[2/2] Linking C executable cmTC_22a78
Android (6875598, based on r399163b) clang version 11.0.5 (https://android.googlesource.com/toolchain/llvm-project 87f1315dfbea7c137aa2e6d362dbb457e388158d)
Target: armv7-none-linux-android16
Thread model: posix
InstalledDir: C:\Users\Grendel\android-toolchain\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin
Found candidate GCC installation: C:/Users/Grendel/android-toolchain/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib/gcc/arm-linux-androideabi\4.9.x
Selected GCC installation: C:/Users/Grendel/android-toolchain/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib/gcc/arm-linux-androideabi/4.9.x
Candidate multilib: thumb;@mthumb
Candidate multilib: armv7-a;@march=armv7-a
Candidate multilib: armv7-a/thumb;@march=armv7-a@mthumb
Candidate multilib: .;
Selected multilib: armv7-a/thumb;@march=armv7-a@mthumb

However, I added the following snippet to our CMakeLists.txt:

if(WIN32)
        message(STATUS "WIN32 set")
endif()

if(MINGW)
        message(STATUS "MINGW set")
endif()

And the output is:

-- MINGW set
   Called from: [1]     src/monodroid/CMakeLists.txt
CMake Error at CMakeLists.txt:93 (message):
  Please set the MINGW_DEPENDENCIES_ROOT_DIR variable on command line
  (-DMINGW_DEPENDENCIES_ROOT_DIR=PATH)
fuchg3 commented 3 years ago

It seems cmake 3.19.2 could detect the right platform. That's my temporary workaround.

hhb commented 3 years ago

It seems cmake 3.19.2 could detect the right platform. That's my temporary workaround.

You probably are using cmake's NDK support instead of NDK toolchain file. In that case CMake >= 3.19 is required for NDK r22.

(I'm investing why MINGW is used there in this issue

hhb commented 3 years ago

Seems it is not trivial to build Xamarin.Android. Do you have a smaller repro case?

Also can you try to copy r21 android.toolchain.cmake to r22 to see how it does? One of the new changes in r22 is aosp/1413029. Maybe it affects compiler detection somehow?

grendello commented 3 years ago

Seems it is not trivial to build Xamarin.Android. Do you have a smaller repro case?

You don't really need to build it all. On Windows (from VS prompt), after cloning and restoring submodules, you need the following steps:

  1. In the top source dir run msbuild /t:Prepare Xamarin.Android.sln
  2. Change to the src/monodroid directory and run msbuild /bl (the /bl will create a detailed binary log, msbuild.binlog, which you can replay either with msbuild /v:diag msbuild.binlog or with https://msbuildlog.com/)
  3. CMake generates its files in subdirectories of src/monodroid/obj/{Debug,Release}, you can rerun cmake in any of those with the options needed

Also can you try to copy r21 android.toolchain.cmake to r22 to see how it does? One of the new changes in r22 is aosp/1413029. Maybe it affects compiler detection somehow?

I don't use Windows locally, but I'll try to test that ASAP.

fuchg3 commented 3 years ago

Seems it is not trivial to build Xamarin.Android. Do you have a smaller repro case?

Also can you try to copy r21 android.toolchain.cmake to r22 to see how it does? One of the new changes in r22 is aosp/1413029. Maybe it affects compiler detection somehow?

The problem seems fixed after replacing android.toolchain.cmake file.

grendello commented 3 years ago

@fuchg3 thanks, you saved me from a few hours of installing software :)

grendello commented 3 years ago

Seems it is not trivial to build Xamarin.Android. Do you have a smaller repro case?

Also can you try to copy r21 android.toolchain.cmake to r22 to see how it does? One of the new changes in r22 is aosp/1413029. Maybe it affects compiler detection somehow?

I've just tested it on CI and it does fix the detection

hhb commented 3 years ago

It seems cmake 3.19.2 could detect the right platform. That's my temporary workaround.

Since you have the environment, can you help test what is the minimum cmake version that this issue is fixed? Seems we need to bump this a little bit.

Or maybe we can conservatively make it 3.19?

fuchg3 commented 3 years ago

Since you have the environment, can you help test what is the minimum cmake version that this issue is fixed? Seems we need to bump this a little bit.

Or maybe we can conservatively make it 3.19?

The minimun version is 3.19.0. I changed that value to 3.19 and it worked fine.

hhb commented 3 years ago

The minimun version is 3.19.0. I changed that value to 3.19 and it worked fine.

Thanks seems what we need is cmake 5192 and cmake 5058? I'll update toolchain file.

hhb commented 3 years ago

(Change merged in https://android-review.googlesource.com/c/platform/ndk/+/1557260)

mirabilos commented 3 years ago

This causes lots of warnings with every build now; cmake 3.10 is the latest the Android SDK/NDK offers.

DanAlbert commented 3 years ago

Did you update? As far as we know this is fixed.

mirabilos commented 3 years ago

Update what?

Update the NDK to 22.1? Yes, which is when the new build warnings begun to show up.

Update cmake? No, because cmake 3.10 is the latest version that is possible to use with the SDK/NDK. Which, then, is the very cause of this warning because the NDK 22.1 really wants cmake 3.19 or newer. You should make it possible to comply with the raised requirements before you raise them ☺

DanAlbert commented 3 years ago

The fix does not require CMake 3.19. The fix was specifically to make this work with versions of CMake earlier than 3.19.

We do not have enough information to investigate further. We need a repro case.

As for getting new versions of CMake into the SDK, you're talking to the wrong people. We only maintain the NDK. File a bug against Studio.

enh-google commented 3 years ago

https://issuetracker.google.com/177245495 seems to be the existing public bug. looks like they have 3.18 in the beta channel.

mirabilos commented 3 years ago

Dan Albert dixit:

The fix does not require CMake 3.19.

Yes, I know. But you seem to not have read my initial comment here, which says: the fix now causes tons of build warnings in every single project.

(Incidentally even in whose where 22.0 worked fine, e.g. on GNU/Linux.)

As for getting new versions of CMake into the SDK, you're talking to the wrong people. We only maintain the NDK.

Doesn’t mean it’s right to release a new version of the NDK that effectively requires something that’s not available yet.

bye, //mirabilos -- Sometimes they [people] care too much: pretty printers [and syntax highligh- ting, d.A.] mechanically produce pretty output that accentuates irrelevant detail in the program, which is as sensible as putting all the prepositions in English text in bold font. -- Rob Pike in "Notes on Programming in C"

DanAlbert commented 3 years ago

Yes, I know. But you seem to not have read my initial comment here, which says: the fix now causes tons of build warnings in every single project.

Oh! You didn't say the fix caused this, you just said "this", which I took to mean "this bug". That makes sense now.

Doesn’t mean it’s right to release a new version of the NDK that effectively requires something that’s not available yet.

Nothing is being required here. It's a warning, not an error. That warning is there for a good reason. We used to have bugs filed often for things that happened as a result of compiler ID bypass. The warning is there to communicate "yeah, nothing the NDK can do about it, you need to use a newer version of CMake."

The change in r22b was to raise the minimum version of CMake that was required for using the built-in compiler ID behavior, because prior to 3.19 there were bugs (that's the original report here). We can't revert this to fix the warnings without breaking builds.

FWIW you don't need to wait for the SDK to get 3.20. You can use any version of CMake you want: https://developer.android.com/studio/projects/install-ndk#vanilla_cmake.

mirabilos commented 3 years ago

Dan Albert dixit:

FWIW you don't need to wait for the SDK to get 3.20. You can use any version of CMake you want: https://developer.android.com/studio/projects/install-ndk#vanilla_cmake.

Ah, good to know. But I cannot require that of everyone who wants to use the software… though, if it’s just warning…

mirabilos commented 3 years ago

Dan Albert dixit:

FWIW you don't need to wait for the SDK to get 3.20. You can use any version of CMake you want: https://developer.android.com/studio/projects/install-ndk#vanilla_cmake.

Heh. Debian also only has 3.18.4 so there is no newer version available in a way acceptable to machine usage policy at work…

bye, //mirabilos -- Infrastrukturexperte • tarent solutions GmbH Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/ Telephon +49 228 54881-393 • Fax: +49 228 54881-235 HRB AG Bonn 5168 • USt-ID (VAT): DE122264941 Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg