cmake: Set minimum CEF version to 95 #436
Conversation
We know that our minimum supported CEF version is 95, and we know that OBS (or obs-browser) will fail to build against anything older. Specify the minimum version required in CMake so this becomes a configure failure instead of a build failure.
|
Wouldn't it make more sense to specify an |
Not to me, no. This is a narrowly scoped PR to simply address the lack of a lower bound at configure time. CEF 95 is the oldest version our code will build against at this time. Furthermore, I did not want to create a barrier to development for those implementing support for newer CEF versions by setting further version requirements. Right now, our code has ifdefs for multiple versions of CEF. Historically, we have found that not all CEF upgrades go perfectly, and we have needed to maintain compatibility with different versions on different platforms such that a single exact version requirement would not work. |
If we treat this as a minimum version, then our build system effectively states
If this is not the case code-wise, then adding a single version guard is incomplete. If we add a version guard, it needs to be correct. So we either state the exact version that we support or a range of versions which we do indeed support building with. If different OS have different version requirements, they also need to be added explicitly (see also FFmpeg, for which we have different requirements on Linux). I just don't see a good reason to add a version check, but then not make full use of it by having it specific enough to prohibit configuring an impossible build. |
I disagree. In my reading, it communicates:
Anything else is an interpretation.
Except this effectively is the case, code-wise. The code is known to fail below CEF 95. It is unknown what happens above CEF 103, other than so far vague reports of "it builds okay" (for versions 104 and up till somewhat recently) or in some cases "requires changes" (see #434). This PR is narrowly scoped to address the known failure, not constrain against unknowns.
They currently do not. I am speaking historically as to why we cannot (or could not) universally all the time require one specific version of CEF. We also do not require exact versions or even version ranges of FFmpeg right now. We have set minimums. This mirrors that. If we want to start providing version ranges or maximum versions via CMake, I consider that outside the scope of this PR. This is specifically addressing the lack of a minimum version requirement at configure time which we know will fail at build time.
As mentioned, this is fixing a specific GitHub Issue that is currently open on obs-studio that is trivial to fix that prevents a known bad configuration (anything under CEF 95). If we don't want this, then we might as well wash our hands of it and set no version requirement and simply say "you use our packaged version or use your own at your own risk". In my opinion, this is better than what we have now, which is no version constraints at configure time at all. If we want to discuss the merits of version maximums or version ranges, I would prefer we do that after fixing a known reported bug that is trivial to solve. |
|
It's not an interpretation, with the change you can configure a project with any CEF version above CEF 95. CMake will not prohibit anyone from configuring the project with - say CEF 115 - and thus still allows one to create a "broken by default" project config just like when providing CEF 94 without the change. If we want to use CMake to enforce/check for incompatibility with a provided CEF version, then we should do it correctly and explicitly specify the upper and lower version ranges that our code supports. The difference to dependencies like FFmpeg is that we keep up with their releases well, but once FFmpeg 7.0 is out (and we don't support building with it), we need to add that as a version constraint as well (similar to how we already constrain the supported Python versions). |
Without this change, you can configure the project with any CEF version at all. That seems worse than this. |
My main point is that if we add version guards, we should add them correctly and not little by little. We know it's broken with versions below 95, we cannot ensure it works with version above 103 (and we don't support it at all), so we have a clear range of supported versions. I don't see a good reason why we should not use that as the version spec to fix the underlying issue instead of just the symptom mentioned in the associated GitHub issue. |
|
Discussed off-thread with @PatTheMav . This is fine as-is, but his points are well taken. If we want to set maximum versions for dependencies in CMake, and I am not opposed to doing so, we can approach that as a separate matter. If and when we do so, we'll simply have to be mindful that we need to update CMake when updating for new versions. |
Description
We know that our minimum supported CEF version is 95, and we know that OBS (or obs-browser) will fail to build against anything older. Specify the minimum version required in CMake so this becomes a configure failure instead of a build failure.
Motivation and Context
Fixes obsproject/obs-studio#7963.
Actually fixing on the obs-studio side will require a submodule update.
How Has This Been Tested?
I configured obs-studio on Windows 11 with CEF 103.
Types of changes
Checklist: