From ddd028bf50febb33679e8e28bc93f75f8336142e Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Fri, 21 Oct 2022 14:27:33 -0700 Subject: [PATCH 1/6] Cleanup in `mcap_vendor` package - 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 own source code and we shouldn't run linters on any third party code. Prevent CI failures. Signed-off-by: Michael Orlov --- mcap_vendor/CMakeLists.txt | 42 ++++++++++++++------------------------ mcap_vendor/package.xml | 4 ---- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index 9902303..37a2d27 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -1,6 +1,11 @@ cmake_minimum_required(VERSION 3.5) project(mcap_vendor LANGUAGES C CXX ASM) +## Dependencies +find_package(ament_cmake REQUIRED) +find_package(zstd_vendor REQUIRED) +find_package(zstd REQUIRED) + ## Compile options if(NOT CMAKE_CXX_STANDARD) set(CMAKE_CXX_STANDARD 17) @@ -11,12 +16,6 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") add_compile_options(-Wall -Wextra -Wpedantic -Werror) endif() -## Dependencies -find_package(ament_cmake REQUIRED) -find_package(zstd_vendor REQUIRED) -find_package(zstd REQUIRED) - - ## Define vendor macro macro(build_mcap_vendor) include(FetchContent) @@ -35,48 +34,37 @@ macro(build_mcap_vendor) file(GLOB _lz4_srcs ${lz4_SOURCE_DIR}/lib/*.c) - add_library( - mcap SHARED + add_library(${PROJECT_NAME} SHARED src/main.cpp ${_lz4_srcs} ) set(_mcap_include_dir ${mcap_SOURCE_DIR}/cpp/mcap/include) - target_include_directories(mcap SYSTEM PRIVATE + target_include_directories(${PROJECT_NAME} SYSTEM PRIVATE ${lz4_SOURCE_DIR}/lib ) - target_include_directories(mcap SYSTEM PUBLIC + target_include_directories(${PROJECT_NAME} SYSTEM PUBLIC "$" "$" ) - ament_target_dependencies(mcap zstd) + ament_target_dependencies(${PROJECT_NAME} zstd) install( - DIRECTORY - ${_mcap_include_dir}/mcap - DESTINATION - ${CMAKE_INSTALL_PREFIX}/include + DIRECTORY ${_mcap_include_dir}/mcap + DESTINATION ${CMAKE_INSTALL_PREFIX}/include ) - install(TARGETS mcap EXPORT export_mcap) + install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}) endmacro() ## Call vendor macro build_mcap_vendor() -ament_export_targets(export_mcap HAS_LIBRARY_TARGET) - +ament_export_include_directories(${_mcap_include_dir}) +ament_export_libraries(${PROJECT_NAME}) +ament_export_targets(${PROJECT_NAME} HAS_LIBRARY_TARGET) ament_export_dependencies(zstd_vendor zstd) -## Tests -if(BUILD_TESTING) - find_package(ament_lint_auto REQUIRED) - list(APPEND AMENT_LINT_AUTO_EXCLUDE - ament_cmake_uncrustify - ) - ament_lint_auto_find_test_dependencies() -endif() - ## Package ament_package() diff --git a/mcap_vendor/package.xml b/mcap_vendor/package.xml index 9c97a65..239ebe9 100644 --- a/mcap_vendor/package.xml +++ b/mcap_vendor/package.xml @@ -12,10 +12,6 @@ zstd_vendor - ament_cmake_clang_format - ament_lint_auto - ament_lint_common - ament_cmake From f3606f91ee21e2373b740284384bf70976edd498 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 25 Oct 2022 08:48:05 -0700 Subject: [PATCH 2/6] Install `mcap` headers in `include/${PROJECT_NAME}/mcap` and export them Signed-off-by: Michael Orlov --- mcap_vendor/CMakeLists.txt | 4 ++-- rosbag2_storage_mcap/src/mcap_storage.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index 37a2d27..1e04c1d 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -52,7 +52,7 @@ macro(build_mcap_vendor) install( DIRECTORY ${_mcap_include_dir}/mcap - DESTINATION ${CMAKE_INSTALL_PREFIX}/include + DESTINATION include/${PROJECT_NAME} ) install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}) @@ -61,7 +61,7 @@ endmacro() ## Call vendor macro build_mcap_vendor() -ament_export_include_directories(${_mcap_include_dir}) +ament_export_include_directories(include/${PROJECT_NAME}) ament_export_libraries(${PROJECT_NAME}) ament_export_targets(${PROJECT_NAME} HAS_LIBRARY_TARGET) ament_export_dependencies(zstd_vendor zstd) diff --git a/rosbag2_storage_mcap/src/mcap_storage.cpp b/rosbag2_storage_mcap/src/mcap_storage.cpp index 2306923..9600e4b 100644 --- a/rosbag2_storage_mcap/src/mcap_storage.cpp +++ b/rosbag2_storage_mcap/src/mcap_storage.cpp @@ -36,7 +36,7 @@ #endif #endif -#include +#include #include #include From 916d8f3dc7c93253992d90f10e4af511b8822df6 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Wed, 26 Oct 2022 11:53:14 -0700 Subject: [PATCH 3/6] Revert renaming `mcap` library to `mcap_vendor` and fixed include issue Signed-off-by: Michael Orlov --- mcap_vendor/CMakeLists.txt | 16 ++++++++-------- rosbag2_storage_mcap/src/mcap_storage.cpp | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index 1e04c1d..ea6e8a6 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -34,36 +34,36 @@ macro(build_mcap_vendor) file(GLOB _lz4_srcs ${lz4_SOURCE_DIR}/lib/*.c) - add_library(${PROJECT_NAME} SHARED + add_library(mcap SHARED src/main.cpp ${_lz4_srcs} ) set(_mcap_include_dir ${mcap_SOURCE_DIR}/cpp/mcap/include) - target_include_directories(${PROJECT_NAME} SYSTEM PRIVATE + target_include_directories(mcap PRIVATE ${lz4_SOURCE_DIR}/lib ) - target_include_directories(${PROJECT_NAME} SYSTEM PUBLIC + target_include_directories(mcap PUBLIC "$" - "$" + "$" ) - ament_target_dependencies(${PROJECT_NAME} zstd) + ament_target_dependencies(mcap zstd) install( DIRECTORY ${_mcap_include_dir}/mcap DESTINATION include/${PROJECT_NAME} ) - install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}) + install(TARGETS mcap EXPORT mcap) endmacro() ## Call vendor macro build_mcap_vendor() ament_export_include_directories(include/${PROJECT_NAME}) -ament_export_libraries(${PROJECT_NAME}) -ament_export_targets(${PROJECT_NAME} HAS_LIBRARY_TARGET) +ament_export_libraries(mcap) +ament_export_targets(mcap HAS_LIBRARY_TARGET) ament_export_dependencies(zstd_vendor zstd) ## Package diff --git a/rosbag2_storage_mcap/src/mcap_storage.cpp b/rosbag2_storage_mcap/src/mcap_storage.cpp index 9600e4b..2306923 100644 --- a/rosbag2_storage_mcap/src/mcap_storage.cpp +++ b/rosbag2_storage_mcap/src/mcap_storage.cpp @@ -36,7 +36,7 @@ #endif #endif -#include +#include #include #include From 41c20735f9872e8c76a68b8e213f0acb9918b2ee Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Wed, 26 Oct 2022 13:11:31 -0700 Subject: [PATCH 4/6] Add `LIBRARY DESTINATION lib` for install targets Signed-off-by: Michael Orlov --- mcap_vendor/CMakeLists.txt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index ea6e8a6..3de5148 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -55,7 +55,13 @@ macro(build_mcap_vendor) DESTINATION include/${PROJECT_NAME} ) - install(TARGETS mcap EXPORT mcap) + install( + TARGETS mcap + EXPORT mcap + ARCHIVE DESTINATION lib + LIBRARY DESTINATION lib + RUNTIME DESTINATION bin + ) endmacro() ## Call vendor macro From fc5f8aa5682518c6f65afec2590719643383d1bf Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Wed, 26 Oct 2022 15:07:09 -0700 Subject: [PATCH 5/6] Attempt to fix Windows build error by removing `ament_export_libraries(mcap)` Signed-off-by: Michael Orlov --- mcap_vendor/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index 3de5148..56e8dfc 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -68,7 +68,6 @@ endmacro() build_mcap_vendor() ament_export_include_directories(include/${PROJECT_NAME}) -ament_export_libraries(mcap) ament_export_targets(mcap HAS_LIBRARY_TARGET) ament_export_dependencies(zstd_vendor zstd) From 2bcff24a35fbff4c51a90d0c0bfc57f9a23dbcdf Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Thu, 27 Oct 2022 13:07:13 -0700 Subject: [PATCH 6/6] Nitpick. remove extra space before `HAS_LIBRARY_TARGET` Signed-off-by: Michael Orlov Co-authored-by: Chris Lalancette --- mcap_vendor/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mcap_vendor/CMakeLists.txt b/mcap_vendor/CMakeLists.txt index 56e8dfc..b9bc045 100644 --- a/mcap_vendor/CMakeLists.txt +++ b/mcap_vendor/CMakeLists.txt @@ -68,7 +68,7 @@ endmacro() build_mcap_vendor() ament_export_include_directories(include/${PROJECT_NAME}) -ament_export_targets(mcap HAS_LIBRARY_TARGET) +ament_export_targets(mcap HAS_LIBRARY_TARGET) ament_export_dependencies(zstd_vendor zstd) ## Package