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

Fetch dependencies using FetchContent, remove conan build dependency #27

Closed
wants to merge 9 commits into from

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Apr 6, 2022

Alternative to #26

Instead of using conan to fetch mcap & dependencies, or getting them from rosdep (which doesn't work when we need some static-only APIs and want to control the precise dependency version), we fetch the dependency sources using FetchContent.

cc @nuclearsandwich @wep21 @emersonknapp @jhurliman, please review! Thanks 🙂

jtbandes added 3 commits April 6, 2022 10:41
Signed-off-by: Jacob Bandes-Storch <[email protected]>
Signed-off-by: Jacob Bandes-Storch <[email protected]>
Signed-off-by: Jacob Bandes-Storch <[email protected]>
@jtbandes jtbandes changed the title Build dependencies from submodules Build dependencies from submodules, remove conan build dependency Apr 6, 2022
jtbandes added 2 commits April 6, 2022 11:23
Signed-off-by: Jacob Bandes-Storch <[email protected]>
@jtbandes jtbandes mentioned this pull request Apr 6, 2022
@jtbandes jtbandes requested review from wep21 and jhurliman April 6, 2022 19:08
@jtbandes jtbandes marked this pull request as ready for review April 8, 2022 01:32
@jhurliman
Copy link
Contributor

Do the usage instructions in the README need to be updated?

Signed-off-by: Jacob Bandes-Storch <[email protected]>
@wep21
Copy link
Contributor

wep21 commented Apr 8, 2022

@nuclearsandwich Any thoughts on using submodules? I feel creating vendor packages is better.

@jhurliman
Copy link
Contributor

@wep21 whatever approach we take, a specific version of zstd must be statically linked in. Using the existing ros2 package for the dynamically linked zstd won’t work.

@wep21
Copy link
Contributor

wep21 commented Apr 8, 2022

@jhurliman Which function won't work with dynamically linked zstd? The basic functions(pkay, record, info) seem to work fine with vendor package implementation in my environment.

@jhurliman
Copy link
Contributor

@wep21

https://github.com/foxglove/mcap/blob/main/cpp/mcap/include/mcap/writer.inl#L6

https://github.com/foxglove/mcap/blob/main/cpp/mcap/include/mcap/writer.inl#L258-L259

We declare ZSTD_STATIC_LINKING_ONLY and use the experimental API ZSTD_compress2(), so even if it happens to compile with a dynamically linked zstd it's not in a stable state and is liable to break with any upgrade.

We're on the ROS 2 Tooling WG call right now and an idea that was thrown out is to combine these two approaches, providing a mcap vendor package in the ROS2 ecosystem but building it using submodules or CMake FetchContent to tightly control the version of zstd we pull in and statically link against.

Signed-off-by: Jacob Bandes-Storch <[email protected]>
@jtbandes jtbandes changed the title Build dependencies from submodules, remove conan build dependency Fetch dependencies using FetchContent, remove conan build dependency Apr 8, 2022
@jtbandes
Copy link
Member Author

jtbandes commented Apr 8, 2022

Updated to replace submodules with FetchContent. Another approach with a mcap_vendor package is at #28.

Signed-off-by: Jacob Bandes-Storch <[email protected]>
@jtbandes jtbandes closed this in #28 Apr 20, 2022
jtbandes added a commit that referenced this pull request Apr 20, 2022
Addressing feedback from ros/rosdistro#32534 (comment)

Make mcap available as a shared library in the mcap_vendor package. It does not depend on lz4/zstd shared libraries but pulls them in directly using FetchContent.

mcap itself is also fetched directly from git instead of using conan to pull in the dependency.

Closes #26, closes #27

Signed-off-by: Jacob Bandes-Storch <[email protected]>
@jtbandes jtbandes deleted the jacob/submodules branch April 29, 2022 02:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants