search

robotology / whole-body-estimators

YARP devices that implement estimators for humanoid robots.
26 stars 12 forks source link

Add Github Actions to this repository #6

Closed prashanthr05 closed 4 years ago

traversaro commented 4 years ago

Time has passed, and I would go for GitHub Actions now.

prashanthr05 commented 4 years ago

@traversaro Could you look at this log and point to me why the compilation would fail in Windows?

https://github.com/prashanthr05/whole-body-estimators/runs/506436067#step:12:638

traversaro commented 4 years ago

@traversaro Could you look at this log and point to me why the compilation would fail in Windows?

https://github.com/prashanthr05/whole-body-estimators/runs/506436067#step:12:638

In this cases, where at the end of the log you cannot find a meaningful error, I suggest to download the raw log (see https://pipelines.actions.githubusercontent.com/YOZBtIxyFZC86xEU2CiDMdyRfOcLH42OneCPmug013AruDqGoK/_apis/pipelines/1/runs/6/signedlogcontent/4?urlExpires=2020-03-14T10%3A46%3A39.4417111Z&urlSigningMethod=HMACV1&urlSignature=1IhxIHyUE4wMlKu3ixg5g87ksd4As%2Bl9PU7nmKuxyec%3D in this case) and search for the "error" string for actual errors. There are 18 instances of the "error" string there, so it is feasible to search for all of them. By looking for them, I found the error: 

2020-03-13T19:16:35.9373979Z D:\a\whole-body-estimators\whole-body-estimators\devices\baseEstimatorV1\include\baseEstimatorV1.h(46,23): error C2065: 'M_PI': undeclared identifier [D:\a\whole-body-estimators\whole-body-estimators\build\devices\baseEstimatorV1\baseEstimatorV1.vcxproj]
2020-03-13T19:16:35.9378246Z D:\a\whole-body-estimators\whole-body-estimators\devices\baseEstimatorV1\include\baseEstimatorV1.h(51,29): error C2065: 'M_PI': undeclared identifier [D:\a\whole-body-estimators\whole-body-estimators\build\devices\baseEstimatorV1\baseEstimatorV1.vcxproj]

That is the usual error of use of M_PI that is not actually part of the C++ standard, you can either define our own M_PI, or just pass _USE_MATH_DEFINES to use them also in MSVC, see https://github.com/robotology/QA/issues/81 .

Note that eventually we will be able to use std::numbers::pi that is a constant defined in C++20, see https://en.cppreference.com/w/cpp/numeric/constants .

traversaro commented 4 years ago

Furthermore, I checked a bit a code, and I noted a regression w.r.t. to codyco-modules. The CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS CMake variable that is used to have on Windows/MSVC the same behaviour of "exporting all symbols" that you have on Unix systems is not set, and this will probably create problems when using the internal libraries with the plugins. See https://github.com/robotology/how-to-export-cpp-library/blob/master/CMakeLists.txt#L48 for more details.

In codyco-modules on Windows/MSVC by default BUILD_SHARED_LIBS was set to OFF, so we did not noticed this lack.

prashanthr05 commented 4 years ago

So if I'm not wrong, adding the following lines to the CMakeLists.txt should help avoid some of the errors in Windows machine?

if(MSVC)
   add_definitions(-D_USE_MATH_DEFINES)
   option(BUILD_SHARED_LIBS "Build libraries as shared as opposed to static" OFF)
endif()

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)

Or maybe no need to disable BUILD_SHARED_LIBS at all, since we export all windows symbols?

traversaro commented 4 years ago

Or maybe no need to disable BUILD_SHARED_LIBS at all, since we export all windows symbols?

Exactly.

prashanthr05 commented 4 years ago

This commit https://github.com/prashanthr05/whole-body-estimators/commit/7d94cf3f2beff433aafadab697644c7bcfffa826 fixed the Windows compilation.

prashanthr05 commented 4 years ago

Thank you @traversaro @HosameldinMohamed. Closing this issue.