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

CMake: Add RERUN_INSTALL_RERUN_C option to disable installation of rerun_c library #5374

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Mar 2, 2024

When building the C++ SDK with the BUILD_SHARED_LIBS option set to ON, it is not compulsory to also install the rerun_c static library. This PR adds a CMake option RERUN_INSTALL_RERUN_C (enabled by default), that can be disabled when rerun_sdk is not compiled as static library.

This is useful when packaging the rerun C++ SDK if you want to avoid to also install the rerun_c static library (related to #4579).

What

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@traversaro
Copy link
Contributor Author

Apparently one CI check is failing due to a missing label, but I can't add labels.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thank you! I figured there's no harm in always installing rerun_c, but sure no reason to not make it configurable.

CI for non-maintainer contributors has some issues right now, we'll look after it

@Wumpf Wumpf merged commit 1b4aa3d into rerun-io:main Mar 4, 2024
31 of 36 checks passed
@traversaro
Copy link
Contributor Author

traversaro commented Mar 4, 2024

I figured there's no harm in always installing rerun_c, but sure no reason to not make it configurable.

Thanks!

Just to provide a bit more of context related of how this is related to #4579 . For various reasons (that may be a bit long to discuss here, for the curious I recently discussed this in PickNikRobotics/RSL#116 (comment)) many Linux distributions and/or C++/multi language package managers prefer to avoid to distribute static libraries, in particular conda-forge (see https://github.com/conda-forge/cfep/blob/main/cfep-18.md) prefers to avoid static libraries, mainly because as soon as you have a static library with C or C++ linkage, you do not have all the great mangling that Rust does to ensure to avoid collisions between incompatible symbols (see conda-forge/staged-recipes#23725 (comment)). One could think of just shipping rerun_c and let users ignore it, but if you start shipping it, by Hyrum's Law eventually users will depend on it, so as it is harmless not to include it in the packages, we prefer to do that.

@emilk emilk changed the title Add RERUN_INSTALL_RERUN_C CMake option to disable installation of rerun_c library CMake: Add RERUN_INSTALL_RERUN_C option to disable installation of rerun_c library Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants