vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.32k stars 212 forks source link

Incorrect usage of std::atomic template in cmake configuration check #471

Closed rhabacker closed 2 years ago

rhabacker commented 2 years ago

Describe the bug Building vsg with mingw gcc version 10 fails with

[   61s] CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
[   61s] Please set them or make sure they are set and tested correctly in the CMake files:
[   61s] CXX_ATOMIC_LIBRARIES
[   61s]     linked by target "vsg" in directory /home/abuild/rpmbuild/BUILD/VulkanSceneGraph-0.2.8+20220628+git.7aa0f39/src/vsg
[   61s] 

To Reproduce

  1. Inspect _log.txt
  2. See error

Expected behavior The cmake run should not fail

Desktop (please complete the following information):

Additional context The compiler check introduced with https://github.com/vsg-dev/VulkanSceneGraph/commit/340cc9fb1eec4a9f876ae77ad9486a9a4c6ab925 seems to be incorrect:

$ cat test.cpp 
#include <atomic>
std::atomic a;
int main() { return a; }

Compiling that code always fail

$ i686-w64-mingw32-g++ test.cpp -o test.exe
test.cpp:2:5: error: invalid use of template-name 'std::atomic' without an argument list
    2 |     std::atomic a;
      |     ^~~
test.cpp:2:5: note: class template argument deduction is only available with '-std=c++17' or '-std=gnu++17'
In file included from /usr/lib64/gcc/i686-w64-mingw32/10.3.0/include/c++/atomic:41,
                 from test.cpp:1:
/usr/lib64/gcc/i686-w64-mingw32/10.3.0/include/c++/bits/atomic_base.h:152:12: note: 'template<class _Tp> struct std::atomic' declared here
  152 |     struct atomic;
      |            ^~~~~~
test.cpp: In function 'int main()':
test.cpp:3:25: error: 'a' was not declared in this scope
    3 |     int main() { return a; }
      |                         ^

also on linux

$ g++ test.cpp  -o test 
test.cpp:2:1: error: invalid use of template-name ‘std::atomic’ without an argument list
 std::atomic a;
 ^~~
test.cpp:2:1: note: class template argument deduction is only available with -std=c++1z or -std=gnu++1z
In file included from /usr/include/c++/7/atomic:41:0,
                 from test.cpp:1:
/usr/include/c++/7/bits/atomic_base.h:126:12: note: ‘template<class _Tp> struct std::atomic’ declared here
     struct atomic;
            ^~~~~~
test.cpp: In function ‘int main()’:
test.cpp:3:21: error: ‘a’ was not declared in this scope
 int main() { return a; }

Following the hint mentioned by the compiler also did not solve the problem:

$ i686-w64-mingw32-g++ test.cpp -o test.exe -std=c++17
test.cpp:2:13: error: class template argument deduction failed:
    2 | std::atomic a;
      |             ^
test.cpp:2:13: error: no matching function for call to 'atomic()'
 g++ test.cpp  -o test -std=c++1z
test.cpp:2:13: error: class template argument deduction failed:
 std::atomic a;
             ^
test.cpp:2:13: error: no matching function for call to ‘atomic()’
In file included from test.cpp:1:0:

According to https://en.cppreference.com/w/cpp/atomic/atomic it should be

$ cat test.cpp 
#include <atomic>
std::atomic<int> a;
int main() { return a; }

Compiling this code works as expected

$ i686-w64-mingw32-g++ test.cpp -o test.exe
<no output>
$ g++ test.cpp  -o test 
<no output>
rhabacker commented 2 years ago

Add additional issue of the mentioned commit is the usage of set(HAVE_CXX_ATOMIC_WITHOUT_LIB FALSE).

According to https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html

The check is only performed once, with the result cached in the variable named by <resultVar>. Every subsequent CMake run will re-use this cached value rather than performing the check again, even if the <code> changes. In order to force the check to be re-evaluated, the variable named by <resultVar> must be manually removed from the cache.

Setting it to FALSE will always return a failed test - it should be unset(HAVE_CXX_ATOMIC_WITHOUT_LIB) to always run the test or removed completely so that the test is only run on the first call, which is better.

robertosfield commented 2 years ago

I merged the submission adding the atomic check but as I didn't see an obvious error though it was fine, you are 100% right, the code is wrong. I have fixed the src/vsg/CMakeLists.txt : 3d0260a804d7ca63aa08e57d76ac29a76f1fceef

Could you try this out?

rhabacker commented 2 years ago

but as I didn't see an obvious error though it was fine

It is not visible on Linux because due to HAVE_CXX_ATOMIC_WITHOUT_LIB is always false, find_library() is called and the mentioned library is present. It is noticed with mingw gcc 10 because this library is not present.

robertosfield commented 2 years ago

Have you tried the fix? 3d0260a804d7ca63aa08e57d76ac29a76f1fceef

You'll need to remove your CMakeCache.text and rebuild from scratch.

rhabacker commented 2 years ago

Have you tried the fix?

build has been started. I will report back when it is ready.

rhabacker commented 2 years ago

build has been started. I will report back when it is ready.

build has been finished - the patch works as expected

[   68s] -- Performing Test HAVE_CXX_ATOMIC_WITHOUT_LIB
[   68s] -- Performing Test HAVE_CXX_ATOMIC_WITHOUT_LIB - Success

BTW: That the first line was not printed is also an indication that there was something wrong.

robertosfield commented 2 years ago

Thanks for the testing.