Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMake support to cpp module #1065

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

filippobrizzi
Copy link

Public-Facing Changes

None

Description

Add CMake support for the top-level cpp module.

This allows to use mcap in projects that do not support/use conan.

The CMake file generates the install config files that allow running find_package(mcap) in external projects.

NOTES:

  • It is possible to update examples/ and test/ to be part of the global CMake and stop using conan to build them.
    • If this is needed, I can do it in this PR.
  • I haven't updated the README, as soon as I get greenlight for this PR I will do it.

@CLAassistant
Copy link

CLAassistant commented Feb 21, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ filippobrizzi
❌ Filippo Brizzi


Filippo Brizzi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@filippobrizzi
Copy link
Author

PTAL @jtbandes @wkalt @james-rms

@indirectlylit
Copy link
Contributor

Hi @filippobrizzi, thank you for the contribution! Improved support for our CMake users is important to us. It may be a little while before we get to this, and we appreciate your patience in the meantime.

One thing that will be necessary eventually is a CI step.

At a high level, we'd like to provide CMake scaffolding for folks to build off of without maintaining something for all architectures and environments. It's possible that we could even migrate from conan to cmake internally if we can do it in a way that doesn't impact existing conan users.

Any feedback would be appreciated!

@@ -71,7 +71,7 @@ test-examples-host:

.PHONY: ci-image
ci-image: hdoc-build
docker build -t mcap_cpp_ci -f ci.Dockerfile --build-arg IMAGE=ubuntu:focal --build-arg CC=clang-13 --build-arg CXX=clang++-13 .
docker build -t mcap_cpp_ci -f ci.Dockerfile --build-arg IMAGE=ubuntu:jammy --build-arg CC=clang-13 --build-arg CXX=clang++-13 .
Copy link
Author

Choose a reason for hiding this comment

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

using latest ubuntu to get latest cmake version needed to create an interface library

@filippobrizzi
Copy link
Author

Hi @indirectlylit thanks for the reply.
I have done the following as a demonstration of a possible solution:

  • mcap/cpp is an header only library so there is nothing much to test here as nothing compile
  • to test that the CMake is correct I updated test to be compiled via CMake and fix CI to work with the new paths.
  • examples still work with conan, in this way in CI we make sure that conan still works and as a demonstration on how to use it.
  • overall we have conan and cmake working side-by-side without any issue.

The only real design choice that I made here for the CMake solution is that lz4 and zstd are imported and built only if tests are enabled. The reason is that I think it should be left to the user the decision whether to use and how to import them. And the test CMakeLists.txt shows how that could be done.
Please take a look and let me know what you think. Happy to provide extra feedback and explanation

@indirectlylit
Copy link
Contributor

indirectlylit commented Feb 27, 2024

Thank you @filippobrizzi - iterating on Github CI code can be frustrating and I appreciate the effort!

I've created internal tracking issue FG-6815 to make sure this stays on our radar and we'll review asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants