Skip to content
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

Write CMake variables set in CMake toolchain files to the CMake cache #1369

Closed
esweet431 opened this issue Nov 5, 2020 · 13 comments
Closed
Assignees
Labels

Comments

@esweet431
Copy link

Hi, I work on Microsoft's C++ Team. We recently changed the way we handle CMake toolchains to better support Android projects. We use the value of the following CMake variables in the CMake cache to automatically configure IntelliSense for Android projects configured with a CMake toolchain file:

CMAKE_C_COMPILER_TARGET
CMAKE_CXX_COMPILER_TARGET
CMAKE_SYSROOT
CMAKE_C_COMPILER
CMAKE_CXX_COMPILER

However, the CMake toolchain files provided by the Android ndk do not write the value of these 5 CMake variables to the cache. Are you able to add "CACHE <type> <docstring>" (ref) to the setting of these 5 variables in your CMake toolchain files, so that the variables are written to the CMake cache? This will provide Android developers with IntelliSense when they are using Visual Studio's CMake support.

@esweet431 esweet431 added the bug label Nov 5, 2020
@DanAlbert DanAlbert added this to Triaged in r22 via automation Nov 5, 2020
@DanAlbert DanAlbert added this to Triaged in r23 via automation Nov 5, 2020
@DanAlbert
Copy link
Member

I suspect #463 fixes this. Either way, we should make sure those get set in the legacy toolchain file so this works for current versions of CMake too.

This will provide Android developers with IntelliSense when they are using Visual Studio's CMake support.

We've already shipped r22 beta 1 and so far it's not looking like we'll need a respin, but I've added this to our triage list so we can try to pick it up if that changes. Otherwise we can get this into r23.

@hhb
Copy link
Collaborator

hhb commented Nov 11, 2020

#463 won't fix this. Because upstream cmake does not add these variables to cache either... I'll send a patch upstream to see what they say.

Also what do you need from CMAKE_SYSROOT? We may not set it in the future.

@esweet431
Copy link
Author

esweet431 commented Nov 11, 2020

If a toolchain defines CMAKE_SYSROOT and writes it to the CMake cache, then Visual Studio will pass the value (--sysroot=<value>) when determining the compilers defines and headers to use for IntelliSense. If this variable is not found in the cache, then VS next checks if the “sysroot” environment variable is defined and uses it instead.

@hhb
Copy link
Collaborator

hhb commented Nov 11, 2020

Sounds like it should be fine if CMAKE_SYSROOT is not in the cache. NDK compiler does not depend on --sysroot now.

https://android-review.googlesource.com/c/platform/ndk/+/1495340
https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5491

@DanAlbert
Copy link
Member

Does intellisense need to know where the sysroot is, or does it just need the compiler to work? Can you use the compilation database that CMake generates?

@bradking
Copy link

Nothing besides CMake should be reading information from CMakeCache.txt. IDEs should use the file api to gain access to information about the buildsystem. See CMake Issue 19514 about extending the file api with information about the compilers.

@DanAlbert
Copy link
Member

Gotcha, thanks for the advice :)

I know that the sysroot appears in the file API because we use it in AGP, but that's kind of a bug. If it is actually needed by intellisense we'll keep it, so curious about the answers to my questions above.

@esweet431
Copy link
Author

IntelliSense needs the set of include paths searched. We have had complaints from Android (r21) developers that the includes added to the search path when the sysroot is used on the command line were not being searched. But if the NDK compiler no longer depends on sysroot then maybe it is no longer an issue. We only need to use sysroot for IntelliSense in the cases when the build is going to use it. What do you mean by the “compilation database”?

We are happy to use the file api instead of CMakeCache.txt. When is CMake going to include the toolchain defined CMake variables in the file api? I see in CMake Issue 19514 that a solution was spec’ed a year ago, but no indication that it was implemented.

@bradking
Copy link

No one in CMake upstream is working on CMake issue 19514. Contributors welcome.

@DanAlbert
Copy link
Member

NDK builds still use the sysroot, clang just doesn't need to be told where it is. It's installed in a fixed location relative to the clang binary and clang understands its layout.

The only reliable way to determine what include directories are used by the compiler is to run a test compile with all the same flags and adding -v. That'll print (among other things):

$ android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -v -target aarch64-linux-android21 foo.cpp -c
...
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/google/home/danalbert/src/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1
 /usr/local/google/home/danalbert/src/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/local/include
 /work/src/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/9.0.8/include
 /usr/local/google/home/danalbert/src/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/aarch64-linux-android
 /usr/local/google/home/danalbert/src/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include
End of search list.

If that's not feasible, https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#Sysroot describes the layout. It's not as robust, but certainly easier.

Having given this a bit more thought I'd guess it wouldn't actually be very helpful to guarantee that this value remains in the cmake output. The behavior is documented, and if it ever changes it's likely not the only change that will be needed to adapt.

@esweet431
Copy link
Author

Thanks for the info. We do run the compiler with -v, but we need a way to append sysroot.

When 19514 is addressed we will use file api to read CMake compiler variables instead of reading from CMakeCache.txt directly. For now, we will still require users to write the value of CMAKE_SYSROOT to the cache for automatic IntelliSense configuration. If possible, we suggest that you do the same and write CMake compiler variables to the cache to keep us working.

@DanAlbert
Copy link
Member

We do run the compiler with -v, but we need a way to append sysroot.

It should be implicit. I didn't pass --sysroot for the output above but the sysroot directories are printed because Clang knows about them anyway.

@DanAlbert DanAlbert removed this from Triaged in r22 Jan 21, 2021
@DanAlbert
Copy link
Member

If I've understood correctly, there's nothing for us to do here, and the changes required are on the IntelliSense side? @esweet431 lmk if I've misunderstood and will reopen.

r23 automation moved this from Triaged to Merged Feb 25, 2021
@DanAlbert DanAlbert removed this from Merged in r23 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants