gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

Update CMakeLists for modern builds #290

Merged
PatTheMav merged 1 commit from cmake-overhaul into master 2022-03-17 08:27:17 -05:00
Conversation 11 Commits 1 Files changed 14 +507 -429
PatTheMav commented 2021-04-19 16:20:09 -05:00 (Migrated from github.com)

Description

Updates CMake build script to coincide with changes in https://github.com/obsproject/obs-studio/pull/4560. Header definitions have been changed in source files appropriately.

Motivation and Context

Updates CMake build scripts last updated 7 years ago and improves build situation on macOS.

How Has This Been Tested?

  • Full OBS builds on macOS, Linux, and Windows

Types of changes

  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)

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.
### Description Updates CMake build script to coincide with changes in https://github.com/obsproject/obs-studio/pull/4560. Header definitions have been changed in source files appropriately. ### Motivation and Context Updates CMake build scripts last updated 7 years ago and improves build situation on macOS. ### How Has This Been Tested? * Full OBS builds on macOS, Linux, and Windows ### Types of changes - Code cleanup (non-breaking change which makes code smaller or more readable) - Breaking change (fix or feature that would cause existing functionality to change) ### Checklist: - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [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.
tytan652 (Migrated from github.com) requested changes 2021-04-20 02:52:46 -05:00
tytan652 (Migrated from github.com) left a comment

Same as https://github.com/obsproject/obs-studio/pull/4560, you seem to revert recent CMake changes

Same as https://github.com/obsproject/obs-studio/pull/4560, you seem to revert recent CMake changes
CMakeLists.txt
@ -1,346 +1,343 @@
cmake_minimum_required(VERSION 2.8.12)
project(obs-browser)
tytan652 (Migrated from github.com) commented 2021-04-20 02:51:50 -05:00

Please don't revert #285

Please don't revert #285
tytan652 commented 2021-06-08 11:29:39 -05:00 (Migrated from github.com)

I think you forgot to change the formatcode.sh of this repo

I think you forgot to change the `formatcode.sh` of this repo
PatTheMav commented 2021-06-08 12:05:19 -05:00 (Migrated from github.com)

Good catch - see my comment here: https://github.com/obsproject/obs-vst/pull/85#issuecomment-856917956

Good catch - see my comment here: https://github.com/obsproject/obs-vst/pull/85#issuecomment-856917956
tytan652 (Migrated from github.com) reviewed 2021-06-19 07:11:21 -05:00
CMakeLists.txt
@ -347,0 +88,4 @@
if(ENABLE_BROWSER_LEGACY)
target_compile_definitions(obs-browser PRIVATE ENABLE_BROWSER_LEGACY)
endif()
tytan652 (Migrated from github.com) commented 2021-06-19 07:11:21 -05:00

You should maybe add a fatal error when it's not found rather than letting CMake continue.

You should maybe add a fatal error when it's not found rather than letting CMake continue.
PatTheMav (Migrated from github.com) reviewed 2021-06-19 13:52:28 -05:00
CMakeLists.txt
@ -347,0 +88,4 @@
if(ENABLE_BROWSER_LEGACY)
target_compile_definitions(obs-browser PRIVATE ENABLE_BROWSER_LEGACY)
endif()
PatTheMav (Migrated from github.com) commented 2021-06-19 13:52:28 -05:00

Added.

Added.
RytoEX (Migrated from github.com) requested changes 2021-06-21 01:22:58 -05:00
CMakeLists.txt
@ -1,346 +1,343 @@
cmake_minimum_required(VERSION 2.8.12)
project(obs-browser)
RytoEX (Migrated from github.com) commented 2021-06-19 17:32:20 -05:00

BROWSER_SOURCE_AVAILABLE only seems to be set and never used. I think we can safely remove it.

`BROWSER_SOURCE_AVAILABLE` only seems to be set and never used. I think we can safely remove it.
PatTheMav commented 2021-06-30 13:41:47 -05:00 (Migrated from github.com)

@dodgepong As this is a companion PR to https://github.com/obsproject/obs-studio/pull/4560, should make sense to add it this PR and https://github.com/obsproject/obs-vst/pull/85 to the OBS 28 project as well.

@dodgepong As this is a companion PR to https://github.com/obsproject/obs-studio/pull/4560, should make sense to add it this PR and https://github.com/obsproject/obs-vst/pull/85 to the OBS 28 project as well.
WizardCM (Migrated from github.com) requested changes 2021-09-24 00:44:58 -05:00
CMakeLists.txt
@ -1,346 +1,343 @@
cmake_minimum_required(VERSION 2.8.12)
project(obs-browser)
WizardCM (Migrated from github.com) commented 2021-09-24 00:41:54 -05:00

I disagree with the lack of additional information provided in this message. Something similar to the old line (using the new variable names) would make it clearer to the user.

message(FATAL_ERROR "CEF Not found -- set CEF_ROOT_DIR or set BUILD_BROWSER=OFF.")
I disagree with the lack of additional information provided in this message. Something similar to the old line (using the new variable names) would make it clearer to the user. ``` message(FATAL_ERROR "CEF Not found -- set CEF_ROOT_DIR or set BUILD_BROWSER=OFF.") ```
FindCEF.cmake
@ -1,91 +1,160 @@
include(FindPackageHandleStandardArgs)
WizardCM (Migrated from github.com) commented 2021-09-24 00:39:22 -05:00

I would vote that because we consider obs-browser to be recommended for core building, this shouldn't be marked as Advanced so that users can more clearly see it pre-populated in CMake-GUI.

I would vote that because we consider obs-browser to be recommended for core building, this shouldn't be marked as Advanced so that users can more clearly see it pre-populated in CMake-GUI.
PatTheMav (Migrated from github.com) reviewed 2021-09-24 04:14:31 -05:00
FindCEF.cmake
@ -1,91 +1,160 @@
include(FindPackageHandleStandardArgs)
PatTheMav (Migrated from github.com) commented 2021-09-24 04:14:31 -05:00

Sounds good, changed it.

Sounds good, changed it.
PatTheMav (Migrated from github.com) reviewed 2021-09-24 04:14:46 -05:00
CMakeLists.txt
@ -1,346 +1,343 @@
cmake_minimum_required(VERSION 2.8.12)
project(obs-browser)
PatTheMav (Migrated from github.com) commented 2021-09-24 04:14:46 -05:00

Extended the text accordingly.

Extended the text accordingly.
WizardCM (Migrated from github.com) approved these changes 2021-09-24 04:26:17 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
tytan652
RytoEX
WizardCM
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
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/obs-browser!290
No description provided.