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

Fix CMake Warning and tweak Linux install_headers #6768

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

tytan652
Copy link
Collaborator

Description

Fix this warning:

CMake Warning (dev) at cmake/Modules/ObsHelpers.cmake:43 (install):
  Target obs-frontend-api has PUBLIC_HEADER files but no PUBLIC_HEADER
  DESTINATION.
Call Stack (most recent call first):
  cmake/Modules/ObsHelpers_Linux.cmake:10 (_setup_binary_target)
  UI/obs-frontend-api/CMakeLists.txt:39 (setup_binary_target)
This warning is for project developers.  Use -Wno-dev to suppress it.

And add EXCLUDE_FROM_ALL to Linux install_headers, because even without it we still need to run cmake --install . --component obs_libraries

Motivation and Context

Fix a warning, and tweak something.

How Has This Been Tested?

Try to build a -git package (with my own PKGBUILD) which use cmake --install . --component obs_libraries, header are there.

And Cmake no longer throw the warning.

Types of changes

  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • 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.

@WizardCM WizardCM added the Code Cleanup Non-breaking change which makes code smaller or more readable label Jul 19, 2022
Even without it, we still need to run the following command for other
files:
`cmake --install . --component obs_libraries`
@PatTheMav
Copy link
Member

What's the impact on Windows? Does it create stray header files in the runDir?

@RytoEX
Copy link
Member

RytoEX commented Jul 19, 2022

What's the impact on Windows? Does it create stray header files in the runDir?

We might have to run it on CI to find out, as I think the issue resolved by #6719 was CI only. Adding Seeking Testers and re-running CI.

@RytoEX RytoEX added the Seeking Testers Build artifacts on CI label Jul 19, 2022
@RytoEX
Copy link
Member

RytoEX commented Jul 19, 2022

What's the impact on Windows? Does it create stray header files in the runDir?

No stray files from CI builds, at least.
image

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Looks good to me. @PatTheMav ?

@PatTheMav
Copy link
Member

I haven't tested the changes locally, but looks fine to me.

@RytoEX RytoEX merged commit 4dd1da8 into obsproject:master Jul 20, 2022
@RytoEX RytoEX added this to PRs Pending Review in OBS Studio 28.0 via automation Jul 20, 2022
@RytoEX RytoEX added this to the OBS Studio 28.0 milestone Jul 20, 2022
@RytoEX RytoEX moved this from PRs Pending Review to Completed in OBS Studio 28.0 Jul 20, 2022
@RytoEX
Copy link
Member

RytoEX commented Jul 20, 2022

Hmm. I must have missed this when previously checking:
image
image

It seems that this creates an "include" directory inside the rundir per config. While harmless, this might need a follow-up.

@PatTheMav
Copy link
Member

Hmm. I must have missed this when previously checking: image image

It seems that this creates an "include" directory inside the rundir per config. While harmless, this might need a follow-up.

That's what I expected and why I asked for review on Windows.. 😂

@RytoEX
Copy link
Member

RytoEX commented Jul 20, 2022

Hmm. I must have missed this when previously checking: image image
It seems that this creates an "include" directory inside the rundir per config. While harmless, this might need a follow-up.

That's what I expected and why I asked for review on Windows.. 😂

I might have just skimmed straight past the top-level rundir and gone to bin since that's what we had previously checked.

PatTheMav added a commit that referenced this pull request Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Seeking Testers Build artifacts on CI
Projects
OBS Studio 28.0
  
Completed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants