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

Better CEF integration #1557

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dimitry-ishenko
Copy link
Contributor

@dimitry-ishenko dimitry-ishenko commented Sep 10, 2024

This PR adds a few patches to CEF in order to make it export its configuration targets. This allows cleaner integration of CEF as both a sub-project and a system-provided library.

Notice that I have also eliminated several link_directory() calls and a target_include_directory() call. These are no longer necessary because they are provided by the CEF::CEF target, which is exported by the CEF library. This also eliminated separate paths between MSVC and Linux inside the HTML producer.

This temporarily makes linux-system builds fail until proper libcef117-dev package is accepted into the CasparCG PPA.

@dimitry-ishenko
Copy link
Contributor Author

Waiting for #1560 to be accepted.

@Julusian
Copy link
Member

I am not 100% sold on this currently. I know that it is the proper thing to do, but the duplication of the patches in both here and the ppa isn't nice. Unless the plan is to submit these patches upstream to cef too?

I am also worried about my ability to maintain this patch. I don't know enough about cmake or the conventions for these 'install' scripts to feel at all comfortable making any changes to it.
So I'm not full opposed to it, but I worry that I won't be able to maintain it

@dimitry-ishenko
Copy link
Contributor Author

I am not 100% sold on this currently. I know that it is the proper thing to do, but the duplication of the patches in both here and the ppa isn't nice. Unless the plan is to submit these patches upstream to cef too?

You are right for sure, duplication is not the nicest thing. But, I figured this would be smaller of the two evils (I am also removing lines of code, not just adding them). I will try submitting the patches, but I suspect they are not going to care, at least not for this version of CEF. But, I will give it a shot when I have a chance.

I am also worried about my ability to maintain this patch. I don't know enough about cmake or the conventions for these 'install' scripts to feel at all comfortable making any changes to it. So I'm not full opposed to it, but I worry that I won't be able to maintain it

No worries, I can help maintain the patch for as long as needed. Plus, I've just downloaded the latest version of CEF and the patch still works except for 1 hunk which is simple to fix (they've added a check a before running doxygen). Your current patch also fails in the same exact spot. So, in terms of additional burden, it shouldn't be much more than the current approach.

@dimitry-ishenko
Copy link
Contributor Author

@Julusian I've trimmed the patch down a little bit. It still adds 13 lines of code overall, but in the grand scheme of things, it pushes some of the noise out of the main CMakeLists file into the library.

If we do the same thing with other libraries (such as, FFmpeg, FreeImage, SFML, etc.), which I intend to do after I get our play-out box fully up-and-running, we should be able to eliminate a lot more noise and different code paths between Windows and Linux, as well as local and system builds.

@dimitry-ishenko dimitry-ishenko marked this pull request as ready for review September 17, 2024 22:26
@Julusian
Copy link
Member

One thing to figure out here is should the package be called 'cef' or 'casparcg-cef'?
I called the one in the ppa 'casparcg-cef' to ensure that it wont conflict with anything else, seeing as the casparcg build in the ppa is pinned to an exact version match (I'm not sure if the abi is guaranteed to be stable between patch versions).
I know that is isn't exactly conforming to debian policies for packages, but wanted to play it safe.

Other than that question, (and that I need to publish a new version of cef on the ppa), I have no remaining objections here. Still not 100% convinced, but it does clean up some nastiness I had to do (that BUILD_BYPRODUCTS hack for ninja), so may as well at least give it a try. If I end up getting stuck fighting it I can always revert it at that time.


If we do the same thing with other libraries (such as, FFmpeg, FreeImage, SFML, etc.)

For most of those (except ffmpeg and cef), I think there is a lot less to do, as on linux we only support system versions. But the windows flow could probably be tidied up to move all the conditional link logic to be defined inside of Bootstrap_Windows

For ffmpeg on linux, its already pretty close, using FIND_PACKAGE in both flows, but handling different paths. I don't think its worth trying to use a patch in this repository seeing how simple the differences are, and don't want to make any changes to the source tar.gz, as the current format of that is a simple compile from source of ffmpeg (with some extra so files thrown in).

@dimitry-ishenko
Copy link
Contributor Author

dimitry-ishenko commented Sep 19, 2024

One thing to figure out here is should the package be called 'cef' or 'casparcg-cef'?

I've opted for libcef117 because we are not doing anything caspar-specific and the package is generic enough to be used in other projects. I've also separated other languages into libcef117-data so that people who are tight on space (eg, embedded) can skip it.

You can see my source package here: https://github.com/dimitry-ishenko-casparcg/chromium-embedded-117
And my PPA here: https://launchpad.net/~ppa-verse/+archive/ubuntu/casparcg/+packages

I don't think its worth trying to use a patch in this repository seeing how simple the differences are, and don't want to make any changes to the source tar.gz, as the current format of that is a simple compile from source of ffmpeg (with some extra so files thrown in).

It shouldn't affect the source. Or if it does, I will abort. I was more thinking along the lines of grabbing updated .cmake files for FFmpeg and such and using that to hide some of the complexity/diverging paths.

@Julusian
Copy link
Member

I'm still not sure whether I agree that libcef being reusable is good, or whether I prefer to have it isolated/namespaced..

@dimitry-ishenko
Copy link
Contributor Author

I'm still not sure whether I agree that libcef being reusable is good, or whether I prefer to have it isolated/namespaced.

@Julusian you are right to be concerned -- you are the boss. 😅 If there are any specific issues you want me to address, please holler.

In the meantime, I did create .deb builds for CEF 128 and verified that (a) they can be installed side-by-side with CEF 117, and (b) they are rejected by find_package(CEF 117 REQUIRED). The builds are now also in ppa-verse/casparcg and the source package along with instructions is in chromium-embedded-128.

So, if you decide to switch to CEF 128 some time in the future, it will be ready for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants