search

ami-iit / meshcat-cpp

Self-contained C++ interface for the MeshCat visualizer
BSD 3-Clause "New" or "Revised" License
25 stars 4 forks source link

Document how to compile the library with conda-forge dependencies and add CI jobs #3

Closed traversaro closed 1 year ago

traversaro commented 1 year ago

Some changes:

traversaro commented 1 year ago

@GiulioRomualdi I can't assign you as reviewer, so I just mention you.

traversaro commented 1 year ago

Actually the job already run:

https://github.com/traversaro/meshcat-cpp/actions/runs/4498726093/jobs/7915699431

The Windows error is:

D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(13,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(19,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(26,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(34,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(42,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]

@GiulioRomualdi should we enable C++20 ?

traversaro commented 1 year ago

Moved deps/CMakeLists.txt to MeshcatCppDeps.cmake, otherwise the targets defined in a nested find_package call will not be visibile from the root

I just realized that this may have side effects as we are loosing the EXCLUDE_FROM_ALL, as FetchContent_MakeAvailable basically at some point calls add_subdirectory perhaps there is a way to pass EXCLUDE_FROM_ALL to FetchContent_MakeAvailable.

traversaro commented 1 year ago

Moved deps/CMakeLists.txt to MeshcatCppDeps.cmake, otherwise the targets defined in a nested find_package call will not be visibile from the root

I just realized that this may have side effects as we are loosing the EXCLUDE_FROM_ALL, as FetchContent_MakeAvailable basically at some point calls add_subdirectory perhaps there is a way to pass EXCLUDE_FROM_ALL to FetchContent_MakeAvailable.

Indeed we could do that, see https://stackoverflow.com/questions/65527126/disable-install-for-fetchcontent .

traversaro commented 1 year ago

Moved deps/CMakeLists.txt to MeshcatCppDeps.cmake, otherwise the targets defined in a nested find_package call will not be visibile from the root

I just realized that this may have side effects as we are loosing the EXCLUDE_FROM_ALL, as FetchContent_MakeAvailable basically at some point calls add_subdirectory perhaps there is a way to pass EXCLUDE_FROM_ALL to FetchContent_MakeAvailable.

Indeed we could do that, see https://stackoverflow.com/questions/65527126/disable-install-for-fetchcontent .

Done in https://github.com/ami-iit/meshcat-cpp/pull/3/commits/85d9df8cc62cb8b158615615e6ec2f5cb26b093a .

traversaro commented 1 year ago

As now msgpack-cxx 6.0.0 is available in conda-forge, I switched to use it, see https://github.com/ami-iit/meshcat-cpp/pull/3/commits/33605158e621cfab5559ee3db9fa607be94ef267 .

GiulioRomualdi commented 1 year ago

@traversaro, is the PR ready to be reviewed?

traversaro commented 1 year ago

Actually the job already run:

https://github.com/traversaro/meshcat-cpp/actions/runs/4498726093/jobs/7915699431

The Windows error is:

D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(13,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(19,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(26,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(34,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]
D:\a\meshcat-cpp\meshcat-cpp\src\Shape.cpp(42,13): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\meshcat-cpp\meshcat-cpp\build\MeshcatCpp.vcxproj]

@GiulioRomualdi should we enable C++20 ?

The CI is not working on Windows, see the quoted comment (https://github.com/ami-iit/meshcat-cpp/pull/3#issuecomment-1480837312). I can look into fixing this or just enabling C++20, let me know what do you prefer.

GiulioRomualdi commented 1 year ago

c++20 should be ok, can you add it in this PR?

traversaro commented 1 year ago

c++20 should be ok, can you add it in this PR?

Done in https://github.com/ami-iit/meshcat-cpp/pull/3/commits/27e783d965f1fc3e568f9d40f74bb148fc211198, I also bumped the CMake version to 3.12 as this is the first version with cxx_std_20 support, see https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#high-level-meta-features-indicating-c-standard-support .

traversaro commented 1 year ago

CI job: https://github.com/traversaro/meshcat-cpp/actions/runs/4542601153 .

traversaro commented 1 year ago

CI job: https://github.com/traversaro/meshcat-cpp/actions/runs/4542601153 .

Switching to C++20 created some compilation problems. I would need a bit more time to look into this.

GiulioRomualdi commented 1 year ago

I think they are related to span. Since it si supported by c++20 we can avoid to use our custom implementation

traversaro commented 1 year ago

I think they are related to span. Since it si supported by c++20 we can avoid to use our custom implementation

It was some strange issue related to stduuid: stduuid vendors gsl::span, and when C++20 was used the span header vendored in it was shadowing the span of the C++ standard library. Anyhow, updating to the latest version of stduuid fixed the issue.

traversaro commented 1 year ago

I added also an apt-based CI job that tests compilation on:

I did used vanilla docker images to avoid regression related to GitHub Runner images that use a CMake version different from apt. However, for some reason the action does not run on my fork, probably we can merge and then fix eventual problem? I can also revert the cxx_std_20 change if you prefer. @GiulioRomualdi

GiulioRomualdi commented 1 year ago

It would be nice if you can revert. Then I can update the code to avoid using c++20 features

traversaro commented 1 year ago

It would be nice if you can revert. Then I can update the code to avoid using c++20 features

Done, I kept the stduuid update has anyone is good to update to the latest release.

traversaro commented 1 year ago

Anyhow, it may be worth to just try to enable cxx_std_20, we did that back in https://github.com/ami-iit/yarp-device-openxrheadset/pull/7/files#diff-3cec70df8109e5a4f0d1fabd0f92cc3b2b2930db2532f0784564f1d849dfcb87R95 and it seems to be working fine on Ubuntu 20.04, if you just use designated initializers.