-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CI: Remove deprecated ubuntu-20.04 host and add windows-2025 and ubuntu-20/22.04-arm #1500
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there are also aarch64 versions of Ubuntu runners (yes, the naming is bad):
- ubuntu-22.04-arm
- ubuntu-24.04-arm
Might be worth testing on those too
Will do next. |
@nazar-pc I've also added ubuntu-22/24.04-arm and it's failing A LOT XD |
Why? Because there is no arm support for clang-format: https://github.com/versatica/mediasoup/actions/runs/13720571403/job/38374956016?pr=1500 |
That is some really comprehensive set of tests. I'm wondering if all of those permutations on the Node.js side are needed if Rust/C++ worker tests are passing. Also formatting can be skipped since it is not platform-specific. |
I don't understand. Node tests are a thing, Worker tests are another thing and Rust tests are another thing.
Pushed now. Worker formating check disabled in Windows and Ubuntu ARM. |
What I'm saying is that Node.js is cross-platform, whatever you're writing in TS/JS will work exactly the same way on x86-64 and aarch64, which is not necessarily the case in Rust/C++ with various pointers and other low-level stuff. So testing all versions of distributions and all versions of Node.js in all possible combinations is probably unnecessary. It should be sufficient to have one for Linux and one for Windows in case of Node.js and focus on testing lower-level stuff on different platforms instead. There are 78 jobs right now, that'll take a lot of time to run and hardly provide a lot of useful insights. |
I also see macOS 13/14 can probably be removed to make testing faster. They are only used for development anyway. |
I don't agree because Node may behave different depending on OS. For example, remember that we had to split the UnixSocket channel into two (one for sending from Node to Worker and one for receiving in Node from Worker) due to a bug of Node (some old version) in Windows. In |
We are including them in the worker prebuild CI, we need to test them: # Worker prebuild for Linux with kernel version 6 Ubuntu (22.04).
# Let's use Ubuntu 22.04 instead of 24.04 so that it builds the
# mediasoup-worker binary using an old version of GLib that will work
# on Linux hosts running more modern GLib versions.
# See https://github.com/versatica/mediasoup/issues/1089.
- os: ubuntu-22.04
cc: gcc
cxx: g++
- os: ubuntu-22.04-arm
cc: gcc
cxx: g++
- os: macos-13
cc: clang
cxx: clang++
- os: macos-14
cc: clang
cxx: clang++
- os: macos-15
cc: clang
cxx: clang++
- os: windows-2022
cc: cl
cxx: cl
- os: windows-2025
cc: cl
cxx: cl |
I'm happy that |
Sure, run one on Linux, one on macOS and one on Windows. But I don't see a lot of sense running more than one of each. You can run a few versions of Node.js on Linux too to make sure Node.js versions that are supported are covered too. But I don't think all permutations are really necessary. |
Imagine that the Node bug in Windows I told above only happens in certain version of Windows, then what? |
Then there will be a bug, but as I said, the probability of that in Node.js specifically is quite low and it is likely that there would be nothing we could do about that other than recommend users to upgrade/downgrade Node.js anyway |
Ok, let me please merge this PR first and then I will work on removing entries from CI on next week. |
Worried about these Node tests failing in ubuntu-24.04-arm: https://github.com/versatica/mediasoup/actions/runs/13721389481/job/38377476129?pr=1500 |
Ok, this action never ends: https://github.com/versatica/mediasoup/actions/runs/13721389481/job/38377476129?pr=1500 |
Hell, this is systematically failing: https://github.com/versatica/mediasoup/actions/runs/13721389481/job/38383101706?pr=1500 |
Let's disable it for now then and debug later |
Issue created here: #1502 I will comment that job for this PR to pass and be merged. |
Details