Update macOS minimum version to 10.13 #233

Merged

RytoEX merged 1 commit from update-macos-min into master 2021-03-22 14:54:11 -05:00

Conversation 14 Commits 1 Files changed 1 +5 -1

RytoEX commented 2020-07-22 23:21:57 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Description

Update the minimum macOS version targeted/supported to macOS 10.13.

Motivation and Context

In line with obsproject/obs-deps#16, since we no longer actively support macOS older than 10.13, let's update the minimum macOS version targeted in our code and build scripts.

How Has This Been Tested?

I have not tested this. Someone with able to build and test on macOS 10.13+ will need to build and test this.

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.

### Description <!--- Describe your changes in detail. --> <!--- If this change includes UI elements, please include screenshots. --> Update the minimum macOS version targeted/supported to macOS 10.13. ### Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open Mantis issue, or implements feature request --> <!--- from the Ideas page, please link to the issue here. --> In line with obsproject/obs-deps#16, since we no longer actively support macOS older than 10.13, let's update the minimum macOS version targeted in our code and build scripts. ### 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. --> I have not tested this. Someone with able to build and test on macOS 10.13+ will need to build and test this. ### 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) --> <!--- - 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). - [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. - [ ] 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.

WizardCM (Migrated from github.com) reviewed 2020-07-22 23:21:57 -05:00

RytoEX commented 2020-07-23 01:10:08 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

It seems that trying to build the CEF wrapper (libcef_dll_wrapper) with 10.13 as a target causes some build failures. These failures showed up in my obs-studio fork when trying to build on CI. The failures appear to be due to CEF using functions deprecated in 10.12. The failures may be able to be suppressed by omitting -Werror and/or -Wdeprecated-declarations, but I'm not sure if that's okay here. On the other hand, it's third-party code that we don't modify, so we aren't responsible for errors and deprecation warnings there.

I can try removing those warning flags specifically for building libcef_dll_wrapper, but I'd appreciate some feedback on that action.

It seems that trying to build the CEF wrapper (libcef_dll_wrapper) with 10.13 as a target causes some build failures. These failures showed up in my obs-studio fork when trying to build on CI. The failures appear to be due to CEF using functions deprecated in 10.12. The failures may be able to be suppressed by omitting -Werror and/or -Wdeprecated-declarations, but I'm not sure if that's okay here. On the other hand, it's third-party code that we don't modify, so we aren't responsible for errors and deprecation warnings there. I can try removing those warning flags specifically for building libcef_dll_wrapper, but I'd appreciate some feedback on that action.

WizardCM commented 2020-07-23 01:33:48 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

The main reason we have a 10.13 minimum requirement is due to a Qt update, rather than outdated code on our side. I think it's fine if the browser panels are built with an "outdated" minimum version as it won't affect the user experience.

When we next update CEF, we can make sure to update the minimum macOS version to match. We could create & pin an Issue for this so that we don't forget.

The main reason we have a 10.13 minimum requirement is due to a Qt update, rather than outdated code on our side. I think it's fine if the browser panels are built with an "outdated" minimum version as it won't affect the user experience. When we next update CEF, we can make sure to update the minimum macOS version to match. We could create & pin an Issue for this so that we don't forget.

RytoEX commented 2020-07-23 01:41:26 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Unfortunately, I think the deprecation errors are going to be a problem in the future. The same code that CI is failing on is currently in CEF master, so a CEF update won't fix this.

The hope behind submitting updates for obs-deps, obs-browser, and obs-studio is that by having everything targeting the same minimum macOS we can gain some benefits by not having the build tools and compiler think we need support for 10.8.

It does seem I can explicitly enable -Wno-error=deprecated-declarations just for the libcef_dll_wrapper build, so this is probably okay to do.

Unfortunately, I think the deprecation errors are going to be a problem in the future. The same code that CI is failing on is currently in CEF master, so a CEF update won't fix this. The hope behind submitting updates for obs-deps, obs-browser, and obs-studio is that by having everything targeting the same minimum macOS we can gain some benefits by not having the build tools and compiler think we need support for 10.8. It does seem I can explicitly enable `-Wno-error=deprecated-declarations` _just_ for the libcef_dll_wrapper build, so this is _probably_ okay to do.

👍 1

WizardCM commented 2020-07-23 01:42:48 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

In that case, would it be worth checking the CEF Issues list, and if it's not documented, to raise it ourselves?

In that case, would it be worth checking the CEF Issues list, and if it's not documented, to raise it ourselves?

RytoEX commented 2020-07-23 03:05:36 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

In that case, would it be worth checking the CEF Issues list, and if it's not documented, to raise it ourselves?

It's probably not a bad idea. The closest I could find in their PR/issues lists is this: https://bitbucket.org/chromiumembedded/cef/pull-requests/320/update-mac-target-sdk-to-1010/diff

Not sure what the best approach is for bringing this up there. File a new issue? Any insight @pkviet ?

> In that case, would it be worth checking the CEF Issues list, and if it's not documented, to raise it ourselves? It's probably not a bad idea. The closest I could find in their PR/issues lists is this: https://bitbucket.org/chromiumembedded/cef/pull-requests/320/update-mac-target-sdk-to-1010/diff Not sure what the best approach is for bringing this up there. File a new issue? Any insight @pkviet ?

RytoEX commented 2020-07-24 00:00:58 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

It looks like CEF itself has an open PR where Marshall suggested adding -Wno-deprecated-declarations to their cmake/cef_variables.cmake file to avoid build errors. I've seen other projects use -Wno-deprecated-declarations and/or -Wno-error=deprecated-declarations to prevent deprecation errors/warnings from dependencies, so I think this is relatively safe to do for our purposes here.

As far as I know, -Wno-error=deprecated-declarations turns errors for deprecated function calls into warnings, so it just prevents the build from failing, but the warnings may still show up in build logs. If we wanted to also suppress the warnings, we could probably also use -Wno-deprecated-declarations just for the libcef_dll_wrapper build.

It looks like CEF itself has [an open PR](https://bitbucket.org/chromiumembedded/cef/pull-requests/303/cefclient-port-to-gtk3/diff) where Marshall suggested adding `-Wno-deprecated-declarations` to their `cmake/cef_variables.cmake` file to avoid build errors. I've seen other projects use `-Wno-deprecated-declarations` and/or `-Wno-error=deprecated-declarations` to prevent deprecation errors/warnings from dependencies, so I think this is relatively safe to do for our purposes here. As far as I know, `-Wno-error=deprecated-declarations` turns errors for deprecated function calls into warnings, so it just prevents the build from failing, but the warnings may still show up in build logs. If we wanted to also suppress the warnings, we could probably also use `-Wno-deprecated-declarations` just for the libcef_dll_wrapper build.

PatTheMav commented 2020-10-16 13:59:22 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

@RytoEX yeah that's exactly what I just added to CI for macOS 10.13 "compatibility".

@RytoEX yeah that's exactly what I just added to CI for macOS 10.13 "compatibility".

RytoEX commented 2020-10-16 15:34:23 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

@RytoEX yeah that's exactly what I just added to CI for macOS 10.13 "compatibility".

@PatTheMav
Hmm. I guess I never submitted my branch as a PR for some reason. I probably just forgot.
https://github.com/RytoEX/obs-studio/tree/update-macos-min
github.com/RytoEX/obs-studio@d829a87374

> > > @RytoEX yeah that's exactly what I just added to CI for macOS 10.13 "compatibility". @PatTheMav Hmm. I guess I never submitted my branch as a PR for some reason. I probably just forgot. https://github.com/RytoEX/obs-studio/tree/update-macos-min https://github.com/RytoEX/obs-studio/commit/d829a87374c186137cb023deb37ff14b533290cd

WizardCM commented 2020-11-26 17:15:58 -06:00 (Migrated from github.com)

• Copy link
• Copy Markdown

So what's the status on this one? Likely best to save it for when we update CEF?

So what's the status on this one? Likely best to save it for when we update CEF?

RytoEX commented 2020-12-11 14:58:23 -06:00 (Migrated from github.com)

• Copy link
• Copy Markdown

When we next update CEF, we can make sure to update the minimum macOS version to match. We could create & pin an Issue for this so that we don't forget.

So what's the status on this one? Likely best to save it for when we update CEF?

I somehow didn't see the notification for this. As we are updating CEF, should we merge this?

> When we next update CEF, we can make sure to update the minimum macOS version to match. We could create & pin an Issue for this so that we don't forget. > So what's the status on this one? Likely best to save it for when we update CEF? I somehow didn't see the notification for this. As we are updating CEF, should we merge this?

PatTheMav commented 2020-12-22 10:26:02 -06:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Theoretically we should be able to merge this already, as the dll wrapper is built for macOS 10.13 anyway.

Theoretically we should be able to merge this already, as the dll wrapper is built for macOS 10.13 anyway.

WizardCM commented 2021-03-12 05:52:09 -06:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Please rebase this branch, and then I'll merge this.

Please rebase this branch, and then I'll merge this.

RytoEX commented 2021-03-12 14:55:38 -06:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Rebased and targeted 10.13 as the minimum in both places it was specified. It was previously only specified once, but @PatTheMav has made some additions since this was originally PR'd. Might need to double check that I've targeted the right project in the right place, since some stuff has shifted around.

Rebased and targeted 10.13 as the minimum in both places it was specified. It was previously only specified once, but @PatTheMav has made some additions since this was originally PR'd. Might need to double check that I've targeted the right project in the right place, since some stuff has shifted around.

Sign in to join this conversation.

Reviewers

No reviewers

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

OBS Studio 27.0

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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!233

No description provided.