Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Cleanup in mcap_vendor package #62

Merged
merged 6 commits into from
Oct 27, 2022
Merged

Cleanup in mcap_vendor package #62

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ddd028b
Cleanup in `mcap_vendor` package
MichaelOrlov Oct 21, 2022
f3606f9
Install `mcap` headers in `include/${PROJECT_NAME}/mcap` and export them
MichaelOrlov Oct 25, 2022
916d8f3
Revert renaming `mcap` library to `mcap_vendor` and fixed include issue
MichaelOrlov Oct 26, 2022
41c2073
Add `LIBRARY DESTINATION lib` for install targets
MichaelOrlov Oct 26, 2022
fc5f8aa
Attempt to fix Windows build error by removing `ament_export_librarie…
MichaelOrlov Oct 26, 2022
2bcff24
Nitpick. remove extra space before `HAS_LIBRARY_TARGET`
MichaelOrlov Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 15 additions & 27 deletions mcap_vendor/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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)
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I really don't think you want to change the name of the library like this.

That is, the idea behind vendor packages is that they are temporary (for some definition of temporary) stand-ins for the package in question. In some day in the future, we should be able to remove the vendor package and just get the library from the underlying OS.

Thus, all of our vendor packages actually build and export the actual library name, not a "vendored" version of it. Someday if libmcap is available in Ubuntu, then we can make this package just skip building on Ubuntu.

For those reasons, I do think you should switch back to the mcap library and target name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette Ok, fair point about libmcap. I have reverted renames.

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
"$<BUILD_INTERFACE:${_mcap_include_dir}>"
"$<INSTALL_INTERFACE:include>"
)
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 include/${PROJECT_NAME}
)

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(include/${PROJECT_NAME})
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()
4 changes: 0 additions & 4 deletions mcap_vendor/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@

<depend>zstd_vendor</depend>

<test_depend>ament_cmake_clang_format</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

<export>
<build_type>ament_cmake</build_type>
</export>
Expand Down
2 changes: 1 addition & 1 deletion rosbag2_storage_mcap/src/mcap_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
#endif
#endif

#include <mcap/mcap.hpp>
#include <mcap_vendor/mcap/mcap.hpp>
MichaelOrlov marked this conversation as resolved.
Show resolved Hide resolved

#include <algorithm>
#include <filesystem>
Expand Down