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

CI: Remove deprecated ubuntu-20.04 host and add windows-2025 and ubuntu-20/22.04-arm #1500

Merged
merged 7 commits into from
Mar 7, 2025

Conversation

@ibc ibc requested review from nazar-pc and jmillan March 7, 2025 11:53
Copy link
Collaborator

@nazar-pc nazar-pc left a 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

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

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.

@ibc ibc changed the title CI: Remove deprecated ubuntu-20.04 host and add windows-2025 CI: Remove deprecated ubuntu-20.04 host and add windows-2025 and ubuntu-20/22.04-arm Mar 7, 2025
@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

@nazar-pc I've also added ubuntu-22/24.04-arm and it's failing A LOT XD

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

@nazar-pc I've also added ubuntu-20/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

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

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.

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

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.

I don't understand. Node tests are a thing, Worker tests are another thing and Rust tests are another thing.

Also formatting can be skipped since it is not platform-specific.

Pushed now. Worker formating check disabled in Windows and Ubuntu ARM.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

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.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

I also see macOS 13/14 can probably be removed to make testing faster. They are only used for development anyway.

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

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.

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 mediasoup-node.yaml we are not just testing the TS/JS layer of mediasoup but also the worker since TS tests rely on the Worker.

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

I also see macOS 13/14 can probably be removed to make testing faster. They are only used for development anyway.

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

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

I'm happy that mediasoup-worker-fuzzer CI passes in Ubuntu ARM machines.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

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.

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.

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

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.

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?

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

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

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

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.

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

Worried about these Node tests failing in ubuntu-24.04-arm: https://github.com/versatica/mediasoup/actions/runs/13721389481/job/38377476129?pr=1500

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

@nazar-pc
Copy link
Collaborator

nazar-pc commented Mar 7, 2025

Let's disable it for now then and debug later

@ibc
Copy link
Member Author

ibc commented Mar 7, 2025

Issue created here: #1502

I will comment that job for this PR to pass and be merged.

@ibc ibc merged commit 22affaf into v3 Mar 7, 2025
77 checks passed
@ibc ibc deleted the ci-remove-ubuntu-20.04-and-add-windows-2025 branch March 7, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants