-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[mediasoup-worker] Require C++17, Meson >= 1.1.0 and update subprojects #1081
Conversation
Fixes #1080 We could just require C++14 but let's move to C++17.
@nazar-pc which version of Meson should I set in |
Latest Meson release is 1.1.0 and was released weeks ago (we still use pre-1.0), I suggest using |
Ok. Wondering if we could also update ninja version. Remember that we limit it to 1.10.2.4 in # Let's use a specific version of ninja to avoid buggy version 1.11.1:
# https://mediasoup.discourse.group/t/partly-solved-could-not-detect-ninja-v1-8-2-or-newer/
# https://github.com/ninja-build/ninja/issues/2211
# https://github.com/ninja-build/ninja/issues/2212
NINJA_VERSION ?= 1.10.2.4 |
I don't think we need to limit either. Meson requirements are 1.8.2+, so we are good either way, but you can update to newer version if there is one. |
I cannot figure it out. Question made here: scikit-build/ninja-python-distributions#157 (comment) |
@nazar-pc I cannot run
|
Try deleting |
Thanks. But shouldn't some of the |
I don't think we have a separate cleanup method for that. It is cleaned up by default at the end actually, but not when error happens 😕 |
So https://github.com/versatica/mediasoup/actions/runs/4994428077/jobs/8944973803?pr=1081
Note that it's about |
Maybe it succeeds on macOS, but requires something on Linux, I will look into it later.
Why wouldn't it? Debug is the default for testing and similar purposes. We are caching some stuff in CI, but not |
True, ignore it please. |
@jmillan it fails to build on our server (jsut run
So... Amazing :( |
This has been fixed in our server by upgrading to gcc 7 (it was 6), done by @jmillan. We are good. |
Hehehe |
GH mobile sucks. Fixed. |
@nazar-pc "good news" is that I can reproduce the Rust+Linux issue in our Debian 11 server by just running
I don't really understand what is going on. |
Maybe this: https://stackoverflow.com/questions/69866575/why-cant-linker-find-absl-references A guy having a similar problem with abseil-cpp when building his project using C++17. |
Does using C++14 solve the problem for now? I think that would be the easiest to do for now, we don't have to use C++17 right now. |
Protobuf also having similar problems: protocolbuffers/protobuf#12292 |
Trying to build with C++14 right now. For whatever reason it takes forever to build when running |
It doesn't cache anything between builds, so needs cores to build faster. |
Same problem with C++14. I suspect it's not related to C++ version but to the updated version of abseil-cpp. |
And I cannot try with C++11 (as in v3) because updated version of abseil-cpp requires C++14 or greater. |
As mentioned in mesonbuild/wrapdb#932 latest release is somewhat broken. When I was doing upgrades, I was checking the diff of CMakeFiles between releases, something might have happened that was not accounted for. There is WIP mesonbuild/wrapdb#996 that might address it eventually, I have downgraded abseil-cpp to the previous release for now. Still requires C++14 and should be new enough to resolve original issue. Feel free to investigate what happened to abseil-cpp upstream, I'm not in the mood to dig into it right now. |
Are you 100% sure that abseil-cpp-20220623.0 also contains the fix? See #1080 (comment) |
No, not 100%. @estnml would you mind to try this branch and confirm or deny? GitHub Actions doesn't have GCC 13 yet either. |
Just in case, I was replied this (but I have no idea what it means): protocolbuffers/protobuf#12292 (comment) So options are:
|
There is also option 3: fix it ourselves instead of waiting 😉 |
Fix where? |
Wherever the issue is. Likely in Meson wrap, but investigation is probably necessary. |
Should be good enough for now then, thanks for verifying! |
I'm glad I could help, and thanks for this beautiful project. |
Thanks a lot! Merging. Will release tomorrow. |
mediasoup-node 3.11.25 released and website updated (required Python >= 3.7 and C++17). |
Beautiful, thanks guys! |
Fixes #1080
TODO: Once published, update installation requirements in website.