Umpire icon indicating copy to clipboard operation
Umpire copied to clipboard

Set CMP0067 to use language standard in try_compile

Open davidbeckingsale opened this issue 2 years ago • 3 comments

Fixes #771

davidbeckingsale avatar Aug 30 '22 14:08 davidbeckingsale

It looks reasonable, but actually this doesn't fix the problem, possibly due to a bug in CMake?

I tried two spack build-envs, one with [email protected] and one with [email protected]. With 3.17.5 setting BLT_CXX_STD correctly detects std::filesystem even without this patch. 3.23.2 never detects it correctly, even if I brute force a set(CMAKE_CXX_STANDARD XX) right in front of the check_cxx_source_compiles call. https://gitlab.kitware.com/cmake/cmake/-/issues/22766 and https://gitlab.kitware.com/cmake/cmake/-/issues/22784 seem related but I can't say for sure if this is the same issue. If I add a set(CMAKE_REQUIRED_FLAGS -std=c++XX) in front of the check_cxx_source_compiles call it does correctly detect std::filesystem again, but that's not a very nice solution, especially if you try to support windows.

It clearly looks like a CMake bug, but I don't have any good ideas on how to best work around it.

msimberg avatar Aug 30 '22 15:08 msimberg

That's frustrating, let me do some more digging. I think I will go ahead and merge this anyway.

As a workaround, you should be able to manually set UMPIRE_ENABLE_FILESYSTEM to the desired value.

davidbeckingsale avatar Aug 30 '22 15:08 davidbeckingsale

I hit this problem, too, and decided to dig into it. The solution is pretty simple:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 995761ea..06240ec6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -14,6 +14,7 @@ cmake_policy(SET CMP0067 NEW)
 
 include(CMakeDependentOption)
 include(CMakePackageConfigHelpers)
+include(CheckCXXSourceCompiles)
 
 project(Umpire
   LANGUAGES CXX C

What happens is that when function is first defined (check_cxx_source_compiles() in particular), the function records the policies that were active at the time. I didn't actually track down where exactly it happens, but apparently this function gets first defined when CMP0067 is temporarily set to OLD somewhere, and then it sticks, even though that's not what one wants :(

First defining it early when the policies are what one wants them to be fixes the issue.

germasch avatar Sep 12 '22 19:09 germasch

Hi @germasch - we looked into this as well and confirmed that it works in 3.14, but broke again with anythin newer than 3.18.0. We have a different fix for this in the works here: https://github.com/LLNL/blt/pull/600. Once this PR has been merged in to BLT, we will then pull in that later sub-module for Umpire.

mcfadden8 avatar Oct 03 '22 21:10 mcfadden8

Fixed by #784

davidbeckingsale avatar Oct 10 '22 18:10 davidbeckingsale