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

cmake: Fix CEF minimum version #458

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Oct 12, 2024

Description

Apparently, the find_package signature is incorrect. The [version] component must come before the [REQUIRED] component. I mistakenly placed them in the reverse order when originally adding the minimum version.

https://cmake.org/cmake/help/v3.28/command/find_package.html#basic-signature

Motivation and Context

Noticed this while reviewing another PR. Want to fix my past mistakes.

How Has This Been Tested?

Tested locally on Windows 11.
Before:

-- Found CEF: C:/dev/obsproject/obs-studio/.deps/cef_binary_6533_windows_x64/Release/libcef.dll (found version "127.3.4")

After:

-- Found CEF: C:/dev/obsproject/obs-studio/.deps/cef_binary_6533_windows_x64/Release/libcef.dll (found suitable version "127.3.4", minimum required is "95")

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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.

Apparently, the find_package signature is incorrect. The [version]
component must come before the [REQUIRED] component. I mistakenly placed
them in the reverse order when originally adding the minimum version.

https://cmake.org/cmake/help/v3.28/command/find_package.html#basic-signature
@RytoEX RytoEX added Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable labels Oct 12, 2024
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Does the module even compile with a Chromium 95 build?

Because this instruction says it does, but if that wasn't the case then it's not just the order of arguments that is wrong.

@RytoEX
Copy link
Member Author

RytoEX commented Oct 12, 2024

I have no reason to believe it wouldn't build, since we still have the ifdefs in place. I haven't tried yet though.

@PatTheMav
Copy link
Member

I have no reason to believe it wouldn't build, since we still have the ifdefs in place. I haven't tried yet though.

I wouldn't have expected it to either, but if we use a version guard, it should also use the correct version and not allow a solution/project be generated when a severely outdated CEF version is provided.

@RytoEX
Copy link
Member Author

RytoEX commented Oct 12, 2024

I have no reason to believe it wouldn't build, since we still have the ifdefs in place. I haven't tried yet though.

I wouldn't have expected it to either, but if we use a version guard, it should also use the correct version and not allow a solution/project be generated when a severely outdated CEF version is provided.

I used a double negative. To clarify, I would expect it to build, because we still have ifdefs in place down to that version.

@WizardCM
Copy link
Member

Yes, currently OBS is supposed to still build with Chromium 95 if the user chooses to provide the necessary CEF build.

If we successfully release with Chromium 127, the intent is to drop Chromium 95 and below in favour of 103 & 127.

I have verified current master does build with 95.

@WizardCM WizardCM merged commit 4dafce8 into obsproject:master Oct 14, 2024
1 check passed
@RytoEX RytoEX deleted the fix-cef-minimum-version branch October 17, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants