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

cpp: support linking statically on Windows #1229

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

Conversation

wrightsg
Copy link

@wrightsg wrightsg commented Sep 2, 2024

Changelog

Allows building and linking the mcap library statically on Windows.

Docs

None

Description

I'm using mcap_builder to use the mcap library in my project.

Even though mcap is a header-only library, mcap_builder creates a library for the client application to link against.
mcap_builder currently always creates a shared library. I have a PR open to support creating a static library: olympus-robotics/mcap_builder#4

Creating and linking against a static library on Windows causes linking issues because visibility.hpp implicitly assumes dynamic linking.

This PR adds a define that needs to be set when building the library statically. The PR is backwards compatible, i.e. the behavior remains the same as before if the define is not set. The define is named based on CMake convention, see https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html

@defunctzombie
Copy link
Contributor

A few thoughts after a quick review:

  1. I don't like the undocumented and un-commented MCAP_STATIC_DEFINE
  2. This seems like an issue created by mcap_builder and not specific to the mcap library here.

What is an alternative approach that does not mean introducing CMAKE specific un-documented defines to this library?

@wrightsg
Copy link
Author

wrightsg commented Sep 6, 2024

I guess this could be solved in mcap_builder by not compiling the mcap library into a static or dynamic library but to consume it as header-only library (through CMake interface library).

However, mcap already provides the MCAP_PUBLIC #define which is only required when compiling the mcap library into a static or dynamic library. What is missing, is the support for the specific combination of compiling it as a static library on Windows. The other combinations (dynamic/Windows, static/Linux, dynamic/Linux) are already supported. And not supporting one specific combination is inconsistent.

Regarding documentation, I'm happy to add this to the PR if you point me to the location where it should go (comment in the visibility.hpp header or in some README?).

@james-rms
Copy link
Collaborator

I think this is fine so long as this additional flag is explained in cpp/README.md under the "Including in your Project" heading.

Copy link
Collaborator

@james-rms james-rms left a comment

Choose a reason for hiding this comment

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

Will merge after readme is updated.

@defunctzombie
Copy link
Contributor

@wrightsg ping on updating the readme and then we can merge

@wrightsg wrightsg requested a review from jtbandes as a code owner November 25, 2024 10:03
@wrightsg
Copy link
Author

@wrightsg ping on updating the readme and then we can merge

The README is now updated, apologies for the delay.

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.

3 participants