-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Question: Bumping C++ version to c++17 #1478
Comments
Is there a particular reason to move to c++17? I'm not opposed to requiring it, there simply hasn't been a reason to |
@Julusian Not necessarily. Was just digging into the ffmpeg producer again and couldn't use a structured bindings when looking throught the code. Seems like the only blocker is a bunch of We should likely be changing those auto_ptr regardless since they are only in the single place in the codebase and are concidered obsolete... If I have a compelling reason I'll post an actual issue to bump it. Another question, if it were to be bumped, would the approach there to be set Would we also be disabling compiler extensions? Not sure if any of the current code makes use of them, it builds with them disabled but with the amount of UB in C++ I don't think that is necessarily a glowing recommendation. |
I'm not seeing any usages of
I don't know enough about cmake to have a preference either way. Probably best to go with their recommendations, but that depends on the version of cmake the new version requires.
What compile extensions is that? |
C++17 has a few nice features that Caspar can benefit from:
While there may not be an immediate reason to switch, this can help with code update/maintenance in the future. Just my 2 cents. |
yeah some of those things look like they would be nice. |
You have a very strong argument there. 😃 On the other hand, I don't think there's been any ABI changes since C++11, so bumping standard version to C++17 (or even C++20, if you ask me) shouldn't break any existing code... theoretically... 😵💫 |
Very well may have been in boost however I did get build errors... Boost version 1.66 claims it's tested on C++17 so it shouldn't have any build errors. I'll double check
This was my primary reason for only actually upgrading when a feature is needed. As I said I put in structured bindings to clarify while I was reading through the code however they aren't needed in the codebase. If there is an actual feature that uses them it would be possible to bump the version on only the project in cmake, like just bumping the ffmpeg module to C++17 while keeping the rest C++14 as an interim measure. |
From boost 1.67.0 so seems like this was fixed in version 1.67 of boost. |
Was referring to this property in cmake https://cmake.org/cmake/help/latest/prop_tgt/CXX_EXTENSIONS.html#prop_tgt:CXX_EXTENSIONS. I'm not sure any of the extensions are currently being used, it's a large code base though and there are a large amount of extensions depending on the compiler. Everything builds with it set to off but as I mentioned earlier I don't feel very confident in that. |
It does look like at least in the linux build that are manually set to off in a way note even old cmake recommended...
|
As long as it still works with the compiler on Ubuntu 20.04 I don't have a problem with this.
I think it would be better to have everything using the same, unless we have a compelling reason to keep one lower. Otherwise I fear it will discourage use of newer features as we don't want to risk breaking something by changing it
Haha, yeah I don't think we've had a cmake expert here for many years (if ever), so the cmake files are likely full of all kinds of smells. Just that linux and windows have entirely separate flows is pretty suspicious. |
I'm by no means an expert. Just know that manually adding that flag when there are two different ways to do it using cmake features definitely is an issue. |
Regarding C++20 as far as I know support is still kind of spotty and as far as I know 20.04 does not support it, at least some of the debian builds using unstable debian doesn't support C++20 in the system compilers. |
FWIW gcc-10 is available on Ubuntu 20.04: https://packages.ubuntu.com/focal/devel/gcc-10 and it supports most of the core C++20: https://en.cppreference.com/w/cpp/compiler_support#cpp20 Plus, hopefully people start upgrading to 22.04 or even 24.04, which is just around the corner 😉 |
Years ago I've done some CMake clean-ups but I don't think they've ever been merged into the main branch. I have given up on Caspar since then, but if there is interest I might put in some work on it when we start upgrading our playout machine (in the next few months hopefully). |
2.4.0 hasnt been released yet, so we could easily drop support for 20.04 if that would be beneficial. I'm not attached to keeping it, and am unlikely to ever attempt to run it on there myself. On an unrelated note, but maybe kinda related to cmake things, I am considering whether the caspar linux builds should be distributed as a deb file, and properly rely on system libraries. The current linux builds did not run when I last tried one on 20.04, and the so file copying has been such a pain. |
I am in favor of both.
I've been preaching that for years and had a fork of the server that built against system libs on Linux. But, none of my PRs ever got merged and have rotten into oblivion. 😅 |
It almost all is these days. Just cef, ffmpeg and boost are not taken from the system.
|
oscpack, GLEW, TBB and OpenAL are the other ones.
I like that. I already have one here: https://launchpad.net/~ppa-verse/+archive/ubuntu/cling where I host a few things. We can start with that for testing purposes and then make an official one for CasparCG. There are already .deb packages for GLEW, TBB, OpenAL and Boost. Not sure if they are the right versions. We can separately package up CEF and FFmeg of whatever versions we need. I am familiar with Debian workflow and can put in some time on that in about a month or so, as long as my patches don't end up bit-rotting again. 😸 |
oh yes, oscpack is true. the rest are taken from the system, the files in https://github.com/CasparCG/server/tree/master/src/packages are windows only
I'm not sure if we want/need a ppa/apt repository. We don't do releases particularly often, and even when we do, users wont want to be surprised by a new version being potentially auto-installed. I'm not decided either way yet though.
I'm not entirely sure if it makes more sense to have CEF as part of the main deb. I think it will mostly depend on if we do go the ppa route. If not it would be easier for users to not have to worry about a second deb file, which will have quite strict version rules.
That would be great. I've had a bit of a play and do feel rather lost. I have cpack generating a deb file with just the executable in it, and Im wondering if theres a way to do these |
I have a branch in my github where glew is replaced with glad, since glew is currenty not recommended and Caspar isn't running the latest version as is published on github vs what is on the sourceforge page. It may need some work on the linux build side though since they are setting manual flags on all projects in cmake where glad is threoretically a C lib and that is breaking its build on linux... Could possibly just use glad as header only there... |
Versioning is not a problem. We can add version number into the server's package name. Like Ubuntu does with eg gcc. There is generic gcc package, which depends on gcc-11 (on 22.04 at least) and then there are gcc-9, gcc-10, gcc-11 and gcc-12 packages if you need a specific version. The beauty of having own PPA is you have full control of the release cycle and dependencies. Also, people who are afraid of breaking things, should just pin their packages so they doesn't update.
Monolithic .deb is not a good solution IMHO, as it goes against the Unix philosophy. Say there is a bug or security fix in CEF or FFmpeg. Now you have to recompile the server package and put out a new .deb. To do that you have to bump its version. And then people may think there was a change in the server, when there wasn't. And then how do you tell people that they should grab new version? |
True, We could just make that part of the readme/installation guide to recommend pinning it.
For ffmpeg I think that separate makes sense, as it can be updated without issue. Many users will be happy enough with the distros default 4.4. Others will want a much newer version. But for CEF, we are highly unlikely to want to publish a new version of it separately. In large because the CEF branches are only maintained for a few months. If we want any security patches beyond that we would need to backport them and compile CEF ourselves (doable, it just takes a couple of hours). I have not thought about the scanner at all here.. Thats something else that should be done, and can definitely be its own package |
I don't have an opinion on glew vs glad vs something else. As long as it works on everything, and doesnt require committing any linux binary dependencies to the repository (ideally it should come from the os package manager). |
I just built with glad to test whether GLEW wasn't causing #1450, it wasn't hence not pull request. I don't think GLEW needs to be replaced however glad can be a header only library in the STB fashion so it would end up not being a dep other than as a loader exactly as glew is not. It would however end up being built into the binary. Glad is also public domain... https://github.com/Dav1dde/glad I don't see a reason to change though, except for possibly switching to Vulcan but I'm not going to be the one writing all that code. Something we might want to look into is bumping to glew 2.2.0 for windows, in linux it is the default from 22.04 but 20.04 is still using 2.1.0, not sure what changed since they didn't seem to do release notes for the version bump. https://github.com/nigels-com/glew |
There is a changelog hidden away there, but it appears they havent published the website https://github.com/nigels-com/glew/blob/master/doc/log.html#L101-L278 Yeah, might as well. It would be nice to get it out of the repository too, but finding maintained packages on nuget is a pain, I could create our own package for it instead though. It looks like we are currently on 2.1.0 based on when it was last updated here. I did prototype using vcpkg instead of nuget for dependencies on windows a few months back, as packages there are much better maintained, but having to build ffmpeg from scratch makes it painful. I dont remember if I tried setting up binary caching or not. Or was it that the ffmpeg it produced was statically linked into the casparcg.exe that I didnt like? |
Yeah, I don't think switching to vulkan or something will be trivial, and could be painful for some users as it will likely drop support for older gpus. If anything were to be changed on that front, I would be very tempted by OpenCL instead, as we don't do much with geometry, and it would be simpler to do 10bit (or higher) colour and make non-rgb colorspaces easier |
I would probably argue if there were to be a change on the GPU side to switch to an agnostic API, WebGPU or whatever that one google made. From what I know OpenGL development has stopped now that Vulkan has been released and I wouldn't be suprised if WebGPU makes some decent strides concidering creating cross platform code in it is pretty trivial. For things I will probably look at in the near future it would probably be cleaning up some of the cmake stuff. From what I can tell if Ubuntu 20.04 is the minimum then we can bump cmake to 3.16 which will also give us access to their PCH api and we can get rid of the custom stuff that currently exists. Likewise getting rid of the ton of duplication between windows and linux builds would be great, possibly getting rid of all of it. As mentioned in my other FFmpeg issue having some development documentation would be great. Architecture docs could be really useful. I haven't dug through the codebase a ton but what the actual code path from AMCP to where createproducer/consumer actually get's called seems to be behind more than one or two layers of abstraction, which is a necessary evil but an overview of that might help get new devs onboard. Same as the still currently outstanding bug with ffmpeg where it had crashes when an incomplete or broken video get's played. That bug is entirely because there are 3 threads and 2 executors in there and it's all being syncronised with "fences" of sorts but exceptions are allowed to leak and are not getting caught. If anything I would want to push C++20 just to get coroutine support in the future since it may actually make some of that code easier to abstract... |
did you notice my hack for pch on linux? server/src/CMakeModules/Bootstrap_Linux.cmake Lines 116 to 118 in b0ecbee
Is it beneficial to be using them these days? having them disabled on linux hasn't bothered me at all.
sounds good to me :)
gcc do still call their support experimental, so I'm not sure if we should go that far https://gcc.gnu.org/projects/cxx-status.html#cxx20 |
Apparently PCH speeds up build times? Maybe it's working since loading debug symbols the first time takes longer than building for me in VS... Will try with them removed sometime. |
Was more tongue in cheek. I've found a lot of the C++20+ features that would be really nice have lackluster support |
IMHO we should still bump C++ standard version to 20. MSCV has excellent support for it. And gcc is not too far off. By the time some gets around to using some of the more advanced C++20 features that are currently not implemented, they will most likely be implemented. But at least we won't have to talk about bumping from 17 to 20 in a few years from now. |
Plus, if we set minimum Ubuntu version to 22.04 it's got gcc-11 as the default compiler and gcc-12 is available as well. |
I think we should first prioritise fixing cleaning up the build system. Currently the manual compiler flags getting added actually cause some issues when trying to implement stuff. Likewise would need to bump the cmake version up to at least 3.12 before we can even discuss C++20 since the current version, 3.10, doesn't have support for C++20. |
We can do both. 😺 I will take a stab at it when I get a chance, but it won't be for a few weeks. We are scheduling to upgrade our play-out box, and as part of it I will be able to spend some time on Caspar. |
After lookng into updating CEF, that requires c++17, so it is highly likely this will be a requirement soon |
I do believe the last time I tried that CasparCG does currently compile with C++17 but I could be mistaken. |
@jpc0 yes it compiles with it fine, and now the cmake files specify |
Description
Just a general question regarding whether there is any current blockers to bumping the language version to C++17. It is currently at C++14.
Solution suggestion
Move to C++17
The text was updated successfully, but these errors were encountered: