Instead, folks can use the equivalent variables provided by CMake
to set those. This is an alternative to D111672.
Details
- Reviewers
phosek compnerd - Group Reviewers
Restricted Project Restricted Project Restricted Project - Commits
- rG3ee0cec88eff: [runtimes] Remove FOO_TARGET_TRIPLE, FOO_SYSROOT and FOO_GCC_TOOLCHAIN
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I have the feeling that this is going to break people, but I'm investigating this patch as requested in D111672.
I think that as long as the builders remain green, this is fine. Yes, this is very likely to break users. We should announce this change on llvm-dev at the very least, and you *could* possibly add a transition helper in the form of:
if(NOT CMAKE_SYSROOT AND LIBCXX_SYSROOT) message(SEND_WARNING "LIBCXX_SYSROOT is deprecated, please use CMAKE_SYSROOT") set(CMAKE_SYSROOT ${LIBCXX_SYSROOT} CACHE STRING "" FORCE) endif()
I think that having a nice transition would be preferable, and we could drop it in the next release.
I went through my notes and the reason I introduced these flags was to support the scenario where you use LLVM_ENABLE_PROJECTS=clang;libcxx;... and you want to build libc++ (and other runtimes) for target other than host, so you cannot use CMAKE_CXX_COMPILER_TARGET or CMAKE_SYSROOT because that would also affect the tools build.
That predates the bootstrapping (née runtimes) build support and is very limited (you can only build runtimes for one target, you can only vary a few flags, you're still using the host compiler so you likely need a two-stage build) which is why we haven't used it ever since bootstrapping build became available.
I don't know if anyone still uses this support, but even if so we should steer them towards the bootstrapping build. I agree with @compnerd that providing a deprecation message for now and removing them in the next release might be preferable.
I'm going to land this now, however I'll use message(WARNING instead of message(FATAL_ERROR to avoid requiring a CMake cache rebuild for everyone.
Remark: Targets need to specify CMAKE_ASM_COMPILER_TARGET or UnwindRegistersRestore|Save.S will be assembled for the default target, not the CMAKE_C|CXX_COMPILER_TARGET.
This caused the libunwind miscompile for the clang-ve-ninja bot: https://lab.llvm.org/buildbot/#/builders/91/builds/4728
Damn, that is tricky. Thanks for the heads up. Do you think we should set CMAKE_ASM_COMPILER_TARGET to CMAKE_CXX_COMPILER_TARGET? Or perhaps we could do something like:
if (NOT CMAKE_ASM_COMPILER_TARGET STREQUAL CMAKE_CXX_COMPILER_TARGET) message(FATAL_ERROR "I'm sure this is an oversight!") endif() if (NOT CMAKE_C_COMPILER_TARGET STREQUAL CMAKE_CXX_COMPILER_TARGET) message(FATAL_ERROR "I'm sure this is an oversight!") endif()
Anything to either avoid the miscompile would be better than the status quo, which is kind of a trap.
Do we really know that the ASM target is always the same as the C|CXX target?
+1 for emitting a warning at least. FATAL_ERROR if we know for sure that these are always the same.