gh-obsproject/libdshowcapture
Watch
1
0
Fork
You've already forked libdshowcapture
 0
Code Issues Pull requests 6 Releases Activity

Make libdshowcapture.dll installable #51

Open
Ybalrid wants to merge 1 commit from Ybalrid/yba/cmake-install into master
pull from: Ybalrid/yba/cmake-install
merge into: gh-obsproject:master
Conversation 5 Commits 1 Files changed 1 +3
Ybalrid commented 2023-03-07 13:26:53 -06:00 (Migrated from github.com)

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.
<!--- Please fill out the following template, which will help other contributors review your Pull Request. --> <!--- Make sure you’ve read the contribution guidelines here: https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst --> ### Description <!--- Describe your changes in detail. --> <!--- If this change includes UI elements, please include screenshots. --> 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 <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open GitHub Issue, or implements feature request --> <!--- from the Ideas page, please link to the issue here. --> This allows building this library in a form that is very easy to consume for 3rd party projects. ### How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment (hardware, OS version, etc.),--> <!--- and the tests you ran, including how it may affect other areas of code. --> - 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 <!--- What types of changes does your PR introduce? Uncomment all that apply --> <!--- - Bug fix (non-breaking change which fixes an issue) --> <!--- - New feature (non-breaking change which adds functionality) --> - Tweak (non-breaking change to improve existing functionality) <!--- - Performance enhancement (non-breaking change which improves efficiency) --> <!--- - Code cleanup (non-breaking change which makes code smaller or more readable) --> <!--- - Breaking change (fix or feature that would cause existing functionality to change) --> <!--- - Documentation (a change to documentation pages) --> ### Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). **No C++ changes!** - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate. - [x] I have included updates to all appropriate documentation.
Ybalrid commented 2023-03-07 13:28:21 -06:00 (Migrated from github.com)

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

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 commented 2023-06-05 09:31:28 -05:00 (Migrated from github.com)

@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

@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
PatTheMav (Migrated from github.com) reviewed 2023-06-05 10:09:08 -05:00
CMakeLists.txt
@ -120,2 +120,4 @@
target_link_libraries(libdshowcapture PRIVATE setupapi strmiids ksuser winmm
wmcodecdspuuid)
set_target_properties(libdshowcapture PROPERTIES PUBLIC_HEADER "${libdshowcapture_HEADERS}")
PatTheMav (Migrated from github.com) commented 2023-06-05 10:09:08 -05:00

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})

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 commented 2023-06-05 10:11:07 -05:00 (Migrated from github.com)

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.

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 commented 2023-06-16 07:58:49 -05:00 (Migrated from github.com)

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

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
This pull request has changes conflicting with the target branch.
  • CMakeLists.txt
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'Ybalrid/yba/cmake-install:Ybalrid/yba/cmake-install'
git switch 'Ybalrid/yba/cmake-install'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'
git merge --no-ff 'Ybalrid/yba/cmake-install'
git switch 'Ybalrid/yba/cmake-install'
git rebase 'master'
git switch 'master'
git merge --ff-only 'Ybalrid/yba/cmake-install'
git switch 'Ybalrid/yba/cmake-install'
git rebase 'master'
git switch 'master'
git merge --no-ff 'Ybalrid/yba/cmake-install'
git switch 'master'
git merge --squash 'Ybalrid/yba/cmake-install'
git switch 'master'
git merge --ff-only 'Ybalrid/yba/cmake-install'
git switch 'master'
git merge 'Ybalrid/yba/cmake-install'
git push origin 'master'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Bug

   
Duplicate

   
Enhancement

   
Help Wanted

   
Invalid

   
Question

   
Wontfix
No labels
Bug
Duplicate
Enhancement
Help Wanted
Invalid
Question
Wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/libdshowcapture!51
No description provided.