search

ros-tooling / rosbag2_storage_mcap

rosbag2 storage implementation for MCAP file format
https://mcap.dev/
Apache License 2.0
32 stars 5 forks source link

Windows CI not succeeding #67

Closed MichaelOrlov closed 1 year ago

MichaelOrlov commented 1 year ago

Description

Latest main does not pass CI on Windows

Expected Behavior

Windows CI shall pass green without errors and warnings

Additional context

Build fails with error message:

CMake Error at C:/ci/ws/install/share/mcap_vendor/cmake/mcapExport.cmake:85 (message):
16:13:36 CMake Error at C:/ci/ws/install/share/mcap_vendor/cmake/mcapExport.cmake:85 (message):
16:13:36   The imported target "mcap_vendor::mcap" references the file
16:13:36      "C:/ci/ws/install/lib/mcap.lib"
16:13:36   but this file does not exist.  Possible reasons include:
16:13:36   * The file was deleted, renamed, or moved to another location.
16:13:36   * An install or uninstall procedure did not complete successfully.
16:13:36   * The installation package was faulty and contained
16:13:36      "C:/ci/ws/install/share/mcap_vendor/cmake/mcapExport.cmake"
16:13:36   but not all the files it references.
16:13:36 Call Stack (most recent call first):
16:13:36   C:/ci/ws/install/share/mcap_vendor/cmake/ament_cmake_export_targets-extras.cmake:9 (include)
16:13:36   C:/ci/ws/install/share/mcap_vendor/cmake/mcap_vendorConfig.cmake:41 (include)
16:13:36   CMakeLists.txt:23 (find_package)

From the log file It looks like mcap.lib hasn't been built during the build process for some unknown reasons. Also from logs it looks like it was built mcap.dll but not mcap.lib.

MichaelOrlov commented 1 year ago

cc: @amacneil amd @clalancette I have spent 2-3 days trying to make mcap_vendor package to be successfully built on Windows but have no more ideas why it fails.
Unfortunately I don't have yet a local setup of Windows with installed ROS2 environment to be able to debug this issue in more details locally. I am sorry but I am also don't have more time resources to handle this issue. At least in next 1-2 weeks.

MichaelOrlov commented 1 year ago

@amacneil We also have bunch of warnings on Windows build in mcap sources which we need to address.

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
christophebedard commented 1 year ago

@MichaelOrlov I think I've run into this issue before with ros2_tracing. It might be due to no symbols being exported on Windows. See here: https://gitlab.com/ros-tracing/ros2_tracing/-/issues/37#note_198689672. The solution would be to add visibility macros, e.g., ROSBAG2_STORAGE_..._PUBLIC. You can probably refer to these changes: https://gitlab.com/ros-tracing/ros2_tracing/-/merge_requests/74/diffs

MichaelOrlov commented 1 year ago

@christophebedard The problem is that this is a vendor package and we are not own the source code in this package to be able to add visibility macros.

jhurliman commented 1 year ago

@MichaelOrlov if you submit a PR to upstream mcap we can get that added.

MichaelOrlov commented 1 year ago

@jhurliman I know. The problem is that to submit PR need to be assure that it will help. i.e. need to try to fix it locally with Windows build.

emersonknapp commented 1 year ago

I'm going to attempt a windows environment setup tomorrow to try reproducing and debugging the issue. I have a machine with windows installed now, but it's not set up for ros dev yet. Wish me luck haha.

emersonknapp commented 1 year ago

Progress update: I do have a windows environment working and am reproducing this error locally, now digging into the CMake to find out what is the cause of the error. I don't think that the issue is a lack of exported symbols (yet), because the immediate problem is a mismatch between whether the consumer expects mcap.dll - which is created - or mcap.lib which is not being created, and I think is supposed to just be for static libraries?

MichaelOrlov commented 1 year ago

@emersonknapp It was a hypothesis that mcap.lib not created because there are no any exported elements.

emersonknapp commented 1 year ago

Yes, you're right, I've just confirmed that if there are no exported symbols at all, then the .lib is not created, but as soon as any dllexport is present then it is. I am adding visibility macros similar to the ones used in other ros2 packages, and testing to see which exports are strictly required.