search

robotology / yarp

YARP - Yet Another Robot Platform
http://www.yarp.it
Other
524 stars 195 forks source link

Compilation error of `Log.cpp` on Windows #3031

Closed pattacini closed 11 months ago

pattacini commented 1 year ago

Describe the bug Got compilation error con icub-main CI of yarp-3.8 at:

https://github.com/robotology/yarp/blob/323e27a602d5dc610feaacd4bf9742282d081bab/src/libYARP_os/src/yarp/os/Log.cpp#L751

Here's the error:

2023-10-19T08:04:46.9034651Z D:\a\icub-main\icub-main\yarp\src\libYARP_os\src\yarp\os\Log.cpp(751,27): error C2589: '(': illegal token on right side of '::' [D:\a\icub-main\icub-main\yarp\build\src\libYARP_os\src\YARP_os.vcxproj]
2023-10-19T08:04:46.9120915Z D:\a\icub-main\icub-main\yarp\src\libYARP_os\src\yarp\os\Log.cpp(751,27): error C2062: type 'unknown-type' unexpected [D:\a\icub-main\icub-main\yarp\build\src\libYARP_os\src\YARP_os.vcxproj]
2023-10-19T08:04:46.9123747Z D:\a\icub-main\icub-main\yarp\src\libYARP_os\src\yarp\os\Log.cpp(751,27): error C2059: syntax error: ')' [D:\a\icub-main\icub-main\yarp\build\src\libYARP_os\src\YARP_os.vcxproj]
2023-10-19T08:04:46.9324216Z D:\a\icub-main\icub-main\yarp\src\libYARP_os\src\yarp\os\Log.cpp(752): error C3536: 'p': cannot be used before it is initialized [D:\a\icub-main\icub-main\yarp\build\src\libYARP_os\src\YARP_os.vcxproj]

To Reproduce Steps to reproduce the behavior: See:

Expected behavior We should fix yarp-3.8 in this respect.

Screenshots If applicable, add screenshots to help explain your problem.

Configuration (please complete the following information):

The build environment can be figured out from the GH CI.

Additional context Add any other context about the problem here.

cc @randaz81

traversaro commented 1 year ago

I got the same problem in the robotology-superbuild: https://github.com/robotology/robotology-superbuild/issues/1505 . I guess it is a regression due to an update in Visual Studio version in GitHub Actions. I suspect some header started including windows.h, and so we have a problem with the min macro definition. Probably we can define somewhere (even at CMake level) NOMINMAX (see https://stackoverflow.com/questions/13416418/define-nominmax-using-stdmin-max). Another workaround is to write std::min as (std::min), see https://stackoverflow.com/questions/13416418/define-nominmax-using-stdmin-max .

traversaro commented 1 year ago

Actually windows.h is explicitly included, see https://github.com/robotology/yarp/blob/a0d408b7c4215bae89c9784090fb942ead103893/src/libYARP_os/src/yarp/os/Log.cpp#L45 . So probably something changed that made YARP_HAS_WIN_VT_SUPPORT defined, see https://github.com/robotology/yarp/blob/a0d408b7c4215bae89c9784090fb942ead103893/cmake/YarpSystemCheck.cmake#L175 . CMAKE_SYSTEM_VERSION is a tricky variable and can change based on the Windows SDK installed in the machine or the CMake version. Anyhow, I think regardless of what happened this needs to be fixed.

traversaro commented 1 year ago

Interesting, NOMINMAX should be already defined, see https://github.com/robotology/yarp/blob/a0d408b7c4215bae89c9784090fb942ead103893/src/libYARP_os/src/CMakeLists.txt#L438 . Then I do not know what is going on. A possible hotfix would be to just disable the YARP_HAS_WIN_VT_SUPPORT part of the code.

pattacini commented 1 year ago

Hi @randaz81 @elandini84

Any news about this? It's somewhat critical as it breaks our main CI workflows in robotology-superbuild, icub-main...

traversaro commented 11 months ago

Thanks @randaz81 !