Cleanup in `mcap_vendor` package

MichaelOrlov commented 1 year ago

Use ${PROJECT_NAME} instead of confusing mcap for library name and exporting target, to be consistent with all other packages in ROS2.
Add export for path to the mcap includes and export for mcap_vendor library. It was an issue in downstream packages that mcap/mcap.hpp was not visible since we were not exporting path to it explicitly.
Removed test section with linters for mcap_vendor package. Since package itself doesn't contain any tests and its own code and we shouldn't run linters on any third party code. Prevent CI failures.

Signed-off-by: Michael Orlov michael.orlov@apex.ai

MichaelOrlov commented 1 year ago

Gist: https://gist.githubusercontent.com/MichaelOrlov/8dd65556eb06031d211447a0b0193f65/raw/86c621126e24e948807e3c86fd2a43fd1f5d1b35/ros2.repos BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap TEST args: --packages-above rosbag2_storage_mcap ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11024

Linux
Linux-aarch64
Windows

MichaelOrlov commented 1 year ago

re-run CI after fixing compilation issue.

Linux
Linux-aarch64
Windows

MichaelOrlov commented 1 year ago

@clalancette I've addressed your comments. Could you please reiterate on review?

MichaelOrlov commented 1 year ago

re-run Windows build after rebasing on top of rename test_fixture_interfaces package to testdata (#64)

Windows

MichaelOrlov commented 1 year ago

re-run Windows build after renames and fixing include issue.

Windows

MichaelOrlov commented 1 year ago

@clalancette I see. I don't have to much ideas why library can't be found on Windows. Trying re-run Windows build after adding LIBRARY DESTINATION lib to install targets

Windows

BTW could be that

 install(TARGETS mcap EXPORT mcap)

calling before

ament_export_libraries(mcap)
ament_export_targets(mcap  HAS_LIBRARY_TARGET)

??

clalancette commented 1 year ago

BTW could be that

 install(TARGETS mcap EXPORT mcap)

calling before

ament_export_libraries(mcap)
ament_export_targets(mcap  HAS_LIBRARY_TARGET)

I don't think so; that seems to be the typical way we do things. As a random example that I know works on Windows, see https://github.com/ros2/geometry2/blob/rolling/tf2/CMakeLists.txt .

MichaelOrlov commented 1 year ago

@clalancette It seems the problem is that mcap.dll installing in install/bin folder

mcap.vcxproj -> C:\ci\ws\build\mcap_vendor\Release\mcap.dll

  Building Custom Rule C:/ci/ws/src/ros-tooling/rosbag2_storage_mcap/mcap_vendor/CMakeLists.txt

-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/crc32.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/errors.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/internal.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/intervaltree.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/mcap.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/reader.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/reader.inl
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/read_job_queue.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/types.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/types.inl
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/writer.hpp
-- Installing: C:/ci/ws/install/include/mcap_vendor/mcap/writer.inl
-- Installing: C:/ci/ws/install/bin/mcap.dll
-- Installing: C:/ci/ws/install/share/ament_index/resource_index/package_run_dependencies/mcap_vendor
-- Installing: C:/ci/ws/install/share/ament_index/resource_index/parent_prefix_path/mcap_vendor
-- Installing: C:/ci/ws/install/share/mcap_vendor/environment/ament_prefix_path.bat
-- Installing: C:/ci/ws/install/share/mcap_vendor/environment/ament_prefix_path.dsv
-- Installing: C:/ci/ws/install/share/mcap_vendor/environment/path.bat
-- Installing: C:/ci/ws/install/share/mcap_vendor/environment/path.dsv
-- Installing: C:/ci/ws/install/share/mcap_vendor/local_setup.bat
-- Installing: C:/ci/ws/install/share/mcap_vendor/local_setup.dsv
-- Installing: C:/ci/ws/install/share/mcap_vendor/package.dsv
-- Installing: C:/ci/ws/install/share/ament_index/resource_index/packages/mcap_vendor
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/mcapExport.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/mcapExport-release.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/ament_cmake_export_include_directories-extras.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/ament_cmake_export_libraries-extras.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/ament_cmake_export_targets-extras.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/ament_cmake_export_dependencies-extras.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/mcap_vendorConfig.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/cmake/mcap_vendorConfig-version.cmake
-- Installing: C:/ci/ws/install/share/mcap_vendor/package.xml
---
Finished <<< mcap_vendor [48.7s]

Although ament_cmake_export_libraries-extra.cmake expecting to find it inside install/lib subfolder

     find_library(
        _lib NAMES "${_library}"
        PATHS "${mcap_vendor_DIR}/../../../lib"
        NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH
      )

@clalancette Do you have any ideas what's wrong in CMake file?

MichaelOrlov commented 1 year ago

Trying re-run Windows build after removing ament_export_libraries(mcap) from CMake file

Windows

MichaelOrlov commented 1 year ago

@clalancette I have a suspicious that mcap.lib which is reported to be missed on linker stage hasn't been really built due to the warnings possibly treated as errors

Checking Build System
  Building Custom Rule C:/ci/ws/src/ros-tooling/rosbag2_storage_mcap/mcap_vendor/CMakeLists.txt
  main.cpp
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\internal.hpp(54,46): warning C4267: '+=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(67,23): warning C4244: 'argument': conversion from 'uint64_t' to 'long', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(324,16): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(548,43): warning C4267: '=': conversion from 'size_t' to 'uint16_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(549,45): warning C4267: '=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(550,57): warning C4267: '=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(551,53): warning C4267: '=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(552,47): warning C4267: '=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(1013,40): warning C4267: '+=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(1020,47): warning C4267: '+=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(1034,33): warning C4244: '+=': conversion from 'uint64_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\reader.inl(1068,45): warning C4267: '+=': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\crc32.hpp(86,44): warning C4267: 'initializing': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(45,16): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(268,93): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(268,93): warning C4267: 'initializing': conversion from 'size_t' to 'const int', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(907,57): warning C4267: 'initializing': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(907,57): warning C4267: 'initializing': conversion from 'size_t' to 'const uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(924,81): warning C4267: 'initializing': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(924,81): warning C4267: 'initializing': conversion from 'size_t' to 'const uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(993,83): warning C4267: 'initializing': conversion from 'size_t' to 'uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
C:\ci\ws\build\mcap_vendor\_deps\mcap-src\cpp\mcap\include\mcap\writer.inl(993,83): warning C4267: 'initializing': conversion from 'size_t' to 'const uint32_t', possible loss of data [C:\ci\ws\build\mcap_vendor\mcap.vcxproj]
  lz4.c
  lz4frame.c
  lz4hc.c
  xxhash.c
  mcap.vcxproj -> C:\ci\ws\build\mcap_vendor\Release\mcap.dll

Can we somehow verify that mcap.lib has been successfully built or somehow suppress those warnings? I am sorry I don't have a local machine with Windows setup.

MichaelOrlov commented 1 year ago

Trying re-run Windows build after suppressing warnings with \w option

Windows

clalancette commented 1 year ago

In short, I don't know. Windows is always a pain, and we don't have good ways to go into the CI containers and see what is in there. To move forward here, someone with a Windows machine is going to have to debug it. I do have a Windows machine locally, but I probably don't have time to look into it this week. Ideas:

Find someone else who has a Windows machine handy to help debug.
Go ahead with this PR as-is, since it fixes a couple of other issues, and then fix Windows later (though we'll obviously need to do that before merging this into rosbag2).
Wait for next week when I might be able to spend some time on it.

MichaelOrlov commented 1 year ago

@clalancette I would suggest to move forward with merging this PR and I will create a follow up issue about Windows build failure. I've reverted my last commit with disabling warnings on Windows build, since it doesn't solve a problem.

Wouldn't you mind to approve this PR?

MichaelOrlov commented 1 year ago

Re-run CI

Linux
Linux-aarch64

ros-tooling / rosbag2_storage_mcap

Cleanup in `mcap_vendor` package #62