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

Make libdshowcapture.dll installable #51

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

Conversation

Ybalrid
Copy link

@Ybalrid Ybalrid commented Mar 7, 2023

Description

This changes add an install target to the main CMakeLists.txt. It will install the .lib, .dll and necessary header files to the location set by CMAKE_INSTALL_PREFIX (standard practice for CMake projects)

Motivation and Context

This allows building this library in a form that is very easy to consume for 3rd party projects.

How Has This Been Tested?

  • clone this
  • run configuration with cmake or cmake-gui
  • optionally set CMAKE_INSTALL_PREFIX to a non default directory (if not, it put it in C:\Program Files\<project name>
  • optionally set CMAKE_DEBUG_POSTFIX to allow installing both Release and Debug symbols
  • Build the project, e.g.: with Visual Studio
  • Build the INSTALL sub project

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format. No C++ changes!
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@gxalpha gxalpha requested a review from PatTheMav March 7, 2023 19:27
@Ybalrid
Copy link
Author

Ybalrid commented Mar 7, 2023

There is no documentation to update about building this library, so I am not sure what to do about "all appropriate documentation" In the contribution checklist

@Ybalrid
Copy link
Author

Ybalrid commented Jun 5, 2023

@PatTheMav @gxalpha sorry for the ping, just wanted to bring your attention about this. This is just a minimal change to the CMake files ot allow outputting the DLL easier

@@ -119,3 +119,6 @@ target_compile_definitions(libdshowcapture PRIVATE _UP_WINDOWS=1)

target_link_libraries(libdshowcapture PRIVATE setupapi strmiids ksuser winmm
wmcodecdspuuid)

set_target_properties(libdshowcapture PROPERTIES PUBLIC_HEADER "${libdshowcapture_HEADERS}")
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but given that it's a single property (that has the potential to hold existing values), I'd personally prefer to use:

set_property(TARGET libdshowcapture APPEND PROPERTY PUBLIC_HEADER ${libdshowcapture_HEADERS})

@PatTheMav
Copy link
Member

I don't think this is the correct approach:

The headers are contained in subdirectories, but PUBLIC_HEADER does not support directory structures - it will take all the files specified in the list and copy them into the CMAKE_INCLUDE_DIR in a flattened fashion. The subdirectories are lost.

This will only work if the library (and its consumers) include the headers without any expectation of subdirectories, i.e. #include <IVideoCaptureFilter.h>.

If the headers in subdirectories are not actually public headers, they in turn should not be added as such.

@Ybalrid
Copy link
Author

Ybalrid commented Jun 16, 2023

I'll take a closer look at how these headers are actually installed here, I have been using the output of a cmake install from that script in a project without issues so I probably have missed the flattening of the directories

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

Successfully merging this pull request may close these issues.

2 participants