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 options for building and installing shared, static libraries #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sizeofvoid
Copy link

This PR adds cmake options for building and installing shared and/or static libraries. All combinations were tested on a UNIX-like system. Therefore a test under Windows is missing.

Copy link
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

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

After fixing Matroska-Org/libebml#70 and installing the build. The DLL version of libmatroska builds correctly.

The static version doesn't build because it cannot find libebml headers (which the DLL version finds fine via my CMAKE_INSTALL_PREFIX).

@robUx4
Copy link
Contributor

robUx4 commented Jan 3, 2021

It's missing target_link_libraries(matroska-static PUBLIC EBML::ebml)

@robUx4
Copy link
Contributor

robUx4 commented Jan 3, 2021

This PR fixes #17

@@ -4,6 +4,8 @@ project(matroska VERSION 1.6.2)

option(DISABLE_PKGCONFIG "Disable PkgConfig module generation" OFF)
option(DISABLE_CMAKE_CONFIG "Disable CMake package config module generation" OFF)
option(DISABLE_SHARED_LIBS "Disable build and install shared libraries" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

The preferred name for this option is BUILD_SHARED_LIBS. I've seen it in a lot of libraries.

Copy link
Contributor

Choose a reason for hiding this comment

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

And both libEBML & libMatroska have gained support for BUILD_SHARED_LIBS in the meantime. Maybe this PR is simply outdated & superfluous now? I'd just close it.

Copy link
Contributor

Choose a reason for hiding this comment

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

As link the suggests, it can be turned into an option to make it more visible:

This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries.

It works without the option (we force it in VLC).

Copy link
Contributor

Choose a reason for hiding this comment

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

The option has been added in #79

@robUx4 robUx4 mentioned this pull request Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants