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

Meson build system 💪 #622

Merged
merged 14 commits into from Oct 10, 2021
Merged

Meson build system 💪 #622

merged 14 commits into from Oct 10, 2021

Conversation

nazar-pc
Copy link
Collaborator

@nazar-pc nazar-pc commented Jul 25, 2021

This PR replaces GYP build system with fully-functional Meson build system

🤪 How to review this PR

First of all, please read this message start to finish.

Please review this PR commit by commit, I have intentionally separated changes into pieces such that they make sense, every commit compiles without errors on Linux/macOS (Windows support is added in later commits) and large changes (like moving OpenSSL from one directory to another) are isolated in separate trivial-to-review commits for your convenience.

😎 Compatibility

Make and NPM commands are left are exactly the same as before, so no one should notice any difference.

Makefile has a few internal tasks added and clean/clean-all have slightly different meaning, but behavior is in line with what it was before, no one should be hurt.

⚠️ Changes in system requirements

Previously Python 2 or 3 were required on Linux/macOS, now strictly Python 3 with PIP is required.

On Windows Python 2 was required, now Python 3 with PIP is required just like on Linux/macOS + there is a new requirement to have Make (from MinWG).

In both cases changes are relatively minor, but they are breaking changes nevertheless!

⚙️ Meson configs and dependencies

The way things are implemented is the following:

🪄 How it works

Makefile is modified to first install PIP (with bundled installer) into mediasoup-specific directory, then Meson and Ninja are installed using that PIP installation. This way we only need Python 3 installed and nothing else.

Xcode generator is replaced with Meson, just like compile_commands_template.json is also generated by Meson and doesn't need to be included and re-generated manually with GYP+Bear combo. I didn't test either, but I expect them to work just fine (except different location of Xcode project).

After Meson and Ninja are installed, they are used to setup and build existing Makefile targets. There are environment variables to configure Meson arguments, so it is possible to impact optimization level, ASAN builds, LTO and other options from the outside.

🎉 CI and Windows

All existing CI targets still work, but on top of those Windows CI targets are added:

  • MSVC target (what was officially supported already) compiles and runs all tests, environment variable GYP_MSVS_VERSION is no longer needed to be specified, Meson will take care of everything
  • GCC and Clang compile everything, but fail to run with strange error; they were not supported before anyway, so it is not like that matters, but if someone cares they can fix it

♾️ Next steps

  • ✔️ first of all, I need to continue work with upstream to land required changes in various places so that we no longer depend on my forks and I don't have to maintain them (and PRs are already submitted, just waiting for them to be reviewed)
  • there will be one more PR that will remove unused bundled dependencies after this one is merged, I rebased this PR ~20 times to clean it up and simplify review process (there was ~30 commits initially)
  • we can replace Makefile with something else (ideally a python script, so Node.js is not a requirement for Rust version to install) to remove Make dependency requirement on Windows (not sure if we should do that before this PR lands to reduce chance of breakage for users or we can relax dependencies later)
  • ✔️ we can install PIP even if it is not present on the system already, relaxing system requirements further, but that would require to bundle get-pip.py script (decided it is not worth it)
  • ✔️ there is a minor inconvenience that only maintainers will notice when doing changes to meson.build file or dependencies caused by Ninja doesn't forward environment variables to meson on regeneration ninja-build/ninja#1997
    • can be worked around with make clean-all, but would be nice to get it fixed, thankfully as soon as the fix lands, we'll be able to use it since we do not depend on Ninja being installed on the system
  • we can add an option to use system libraries (IIRC Vittorio Palmisano was maintaining his fork with support for system OpenSSL, this will no longer be necessary and can improve installation times by skipping a bunch of compilation work)

P.S. I spent way too much time on this, especially fighting with Windows while waiting for CI to finish over and over again, hopefully it gets through 🤞

Fixes #359, closes #602

@nazar-pc nazar-pc changed the title Meson build system 💪 Meson build system 💪 Jul 25, 2021
@nazar-pc nazar-pc force-pushed the meson branch 6 times, most recently from 788ec54 to f2998ac Compare July 28, 2021 05:36
@ibc
Copy link
Member

ibc commented Jul 29, 2021

This is awesome. Please let me some days to review because I'm from here to there.

@nazar-pc nazar-pc mentioned this pull request Jul 29, 2021
@cretz
Copy link

cretz commented Jul 29, 2021

For what it's worth, I use this on Windows and really appreciate the effort there. I am curious if there are performance penalties on Windows with no-ASM OpenSSL. I am hoping with this build system it'll be easy to have the Rust lib support the MSVC target too.

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Jul 29, 2021

GYP version also used non-ASM version of OpenSSL, so nothing changes about that yet.

On Rust side file descriptor communication is the primary and basically only thing that is not implemented (I don't use Windows, so unless someone contributes it is not gonna happen), but it is out of scope of this PR, please create a topic on the forum if you want to work on that and I'll try to help you with that.

@nazar-pc nazar-pc force-pushed the meson branch 2 times, most recently from 5432aa7 to 2abffbc Compare August 3, 2021 21:29
@eli-schwartz
Copy link

I don't see the point of bundling a 25k lines copy of get-pip.py, can't you just run python -m ensurepip? https://docs.python.org/3/library/ensurepip.html

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Aug 4, 2021

@eli-schwartz it would have been easier indeed, but if you do that on Ubuntu 20.04 for instance, you'll get this:

~> docker run --rm -it ubuntu:20.04 bash
root@2f119fbd9efa:/# apt-get update
root@2f119fbd9efa:/# apt-get install -y python3
root@2f119fbd9efa:/# python3 -m ensurepip
/usr/bin/python3: No module named ensurepip

That is why I decided to bundle get-pip.py even though I acknowledge how ugly it is.

@eli-schwartz
Copy link

You need to install python3-venv on Debian (or, like the rest of the world, stop using Debian-built python for anything -- theirs is the only distro where the package is actively problematic to use by users instead of distro developers).

Alternatively you could apt install -y python3-pip, or clone the meson repo and run /path/to/meson.py.

IMHO anything is better than checking 25k lines into git.

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Aug 4, 2021

python3 package on Ubuntu/Debian actually depends on python3-venv, but it doesn't bring PIP with it.

Requiring python3-pip package is an option, but the goal here was to not add extra dependencies (initial version of PR did have this requirement).

Cloning Meson adds tools other than Python3 (like git) to dependencies too and still requires us to have PIP or other way of getting Ninja installed on the OS (or adding Ninja to growing list of dependencies).

I'm fine either way (we already bundle the whole OpenSSL here, which far bigger), if Inaki and Jose think adding PIP to requirements is fine, I'll update PR accordingly.

@eli-schwartz
Copy link

https://packages.ubuntu.com/focal/amd64/python3.8-venv/filelist

This package has the ensurepip module in it. python3-venv depends on python3-{minor_version}-venv.

...

Personally I do not and never have seen the problem with installing packages from a distro... Ubuntu 20.04 has meson 0.53 and ninja 1.10 so if you apt install -y meson you'll get all three (including python). You can use pip on other OSes.

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Aug 4, 2021

The challenge here is that mediasoup users interact with NPM and Cargo. They don't care too much about how underlying modules are build and will be surprised when after minor update it suddenly breaks somewhere internally.
I agree it is trivial to install on Linux, but it is less so on Windows for instance.

And I'm not against what you're suggesting BTW, just trying to explain the rationale here.

@eli-schwartz
Copy link

Yes, but on windows you generally don't need to install pip since it's installed by default.

Anyway, I get the rationale, and am merely cautioning that it could be done without a specific disadvantage.

For CI, installing python3-venv or python3-pip should be enough. For end users, I think running python3 -m ensurepip || echo "ensurepip failed, if you are on debian please install the python3-venv package" should be quite sufficient to ensure their lack of confusion.

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Aug 9, 2021

Finally last fork of mine was upstreamed, this PR relies on upstream releases/git revisions/wraps.

@ibc
Copy link
Member

ibc commented Aug 10, 2021

Oh great!!

BTW Sorry for the delay, Jose is off this week

@ibc
Copy link
Member

ibc commented Aug 13, 2021

@nazar-pc please take a look to libsrtp 2.4.0 latest version, it comes with Meson support!

@nazar-pc
Copy link
Collaborator Author

@ibc this PR used pre-release version of 2.4.0 all along, happy to see official release, will update accordingly now.

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@piranna
Copy link
Contributor

piranna commented Aug 18, 2021

A bit offtopic:

Would this be in a 4.0.0 version, or a major version change is not planned? If so, where can we see the upcoming changes? I have some to propose...

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Oct 8, 2021

I do not understand why Windows tests are failing, it doesn't make any sense to me:

/d/a/mediasoup/mediasoup/worker/out/Release/mediasoup-worker-test.exe --invisibles --use-colour=yes 
===============================================================================
All tests passed (1606 assertions in 39 test cases)

mingw32-make: *** [Makefile:130: test] Error 127
mingw32-make: Leaving directory 'D:/a/mediasoup/mediasoup/worker'
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! mediasoup@3.8.4 test:worker: `node npm-scripts.js test:worker`
npm ERR! Exit status 1

Sometimes it succeeds, sometimes it crashes. Unless anyone cares enough we might have to remove it from CI until further investigation.

@ibc
Copy link
Member

ibc commented Oct 8, 2021

In tests.cpp can you enable mediasoup worker ERROR logs? I think there is a env that can be set in command line (it should be obvious by taking a look to tests.cpp)

@ibc
Copy link
Member

ibc commented Oct 8, 2021

Oh but actually all tests passed...

@ibc
Copy link
Member

ibc commented Oct 8, 2021

Maybe it's just a wrong non zero return value so npm task assumes it went wrong?

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Oct 8, 2021

IDK, it sometimes passes CI, for instance commit "Faster worker CI by skipping rebuilding due to installation cleanup" finished successfully here: https://github.com/nazar-pc/mediasoup/runs/3836364875?check_suite_focus=true

@ibc
Copy link
Member

ibc commented Oct 8, 2021

Running this branch locally in macOS BigSur Intel. Some things:

# Updated pip and setuptools are needed for meson
# `--system` is not present everywhere and is only needed as workaround for Debian-specific issue (copied from
# https://github.com/gluster/gstatus/pull/33), fallback to command without `--system` if the first one fails.
/usr/local/bin/python3 -m pip install --system --target=/Users/ibc/src/v3-mediasoup/worker/out/pip pip setuptools || \
		/usr/local/bin/python3 -m pip install --target=/Users/ibc/src/v3-mediasoup/worker/out/pip pip setuptools || \
		echo "Installation failed, likely because PIP is unavailable, if you are on Debian/Ubuntu or derivative please install the python3-pip package"

Usage:
  /usr/local/opt/python/bin/python3.7 -m pip install [options] <requirement specifier> [package-index-options] ...
  /usr/local/opt/python/bin/python3.7 -m pip install [options] -r <requirements file> [package-index-options] ...
  /usr/local/opt/python/bin/python3.7 -m pip install [options] [-e] <vcs project url> ...
  /usr/local/opt/python/bin/python3.7 -m pip install [options] [-e] <local project path> ...
  /usr/local/opt/python/bin/python3.7 -m pip install [options] <archive url/path> ...

no such option: --system

Is no such option: --system expected?


The Meson build system
Version: 0.59.2
Source dir: /Users/ibc/src/v3-mediasoup/worker
Build dir: /Users/ibc/src/v3-mediasoup/worker/out/Release
Build type: native build
Project name: mediasoup-worker
: undefined

Should we fill Project version?


WARNING: Project targeting '>= 0.58' but tried to use feature deprecated since '0.56.0': meson.source_root. use meson.project_source_root() or meson.global_source_root() instead.
Build targets in project: 22
WARNING: Deprecated features used:
 * 0.56.0: {'meson.source_root'}

[559/991] Compiling C object subprojects/libuv-v1.41.0/libuv.a.p/src_threadpool.c.o
../../subprojects/libuv-v1.41.0/src/threadpool.c:49:44: warning: unused parameter 'w' [-Wunused-parameter]
static void uv__cancelled(struct uv__work* w) {
                                           ^
../../subprojects/libuv-v1.41.0/src/threadpool.c:269:55: warning: unused parameter 'req' [-Wunused-parameter]
static int uv__work_cancel(uv_loop_t* loop, uv_req_t* req, struct uv__work* w) {
                                                      ^
2 warnings generated.
[560/991] Compiling C object subprojects/libuv-v1.41.0/libuv.a.p/src_unix_async.c.o
../../subprojects/libuv-v1.41.0/src/unix/async.c:122:69: warning: unused parameter 'events' [-Wunused-parameter]
static void uv__async_io(uv_loop_t* loop, uv__io_t* w, unsigned int events) {

Too many compilation warnings in libuv. Can we avoid them?

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Oct 8, 2021

Is no such option: --system expected?

Yes, that is because Debian/Ubuntu have special version of Python, so we try 2 versions of the command expecting at least one to succeed. In your case it is second one, so it should still work fine.

Should we fill Project version?

We might if you want to maintain yet another place for version change. I don't think it matters too much in this case.

Too many compilation warnings in libuv. Can we avoid them?

It should have not been there, we already set warning_level=0 for all dependencies, I'll ask in Meson chat.

@ibc
Copy link
Member

ibc commented Oct 10, 2021

This PR should remove worker/deps/gyp right?

@nazar-pc
Copy link
Collaborator Author

It should remove most of worker/deps, but I will submit a separate PR with that, the diff is too big as it is.

@ibc
Copy link
Member

ibc commented Oct 10, 2021

BTW I'm creating a PR on top of yours right now. Will announce it in less than 1-2 hours if everything goes fine.

@ibc
Copy link
Member

ibc commented Oct 10, 2021

Here we are: nazar-pc#2

@nazar-pc
Copy link
Collaborator Author

Do you really want to merge that PR into this branch or maybe we can merge it separately? This PR is really big already and nazar-pc#2 is rather distinct in what it contains.

@ibc
Copy link
Member

ibc commented Oct 10, 2021

Do you really want to merge that PR into this branch or maybe we can merge it separately? This PR is really big already and nazar-pc#2 is rather distinct in what it contains.

If I make my PR target current v3 branch, then lot of conflicts would happen later in yours. I've tested my PR (everything I could) and there are no issues. If we wait and any .ts or .js file is changed in the meantime, this is gonna be hell XD

So yes, my intention is to merge this into your PR so we an move forward easily. I promise no regressions will happen due to my PR 😅

@nazar-pc
Copy link
Collaborator Author

Can we just merge this first, create another PR and merge that one? At least there will be 2 distinct merges happening and not one huge update with everything at once.

BTW, do you want me to comment-out Windows CI job for now?

@ibc
Copy link
Member

ibc commented Oct 10, 2021

Can we just merge this first, create another PR and merge that one? At least there will be 2 distinct merges happening and not one huge update with everything at once.

As far as you promise that you are not gonna modify any .ts or .js file... ok :)

BTW, do you want me to comment-out Windows CI job for now?

Yes, please. BTW I would also remove worker/deps/gyp right now, not sure why we should wait for a new PR :-|

@nazar-pc
Copy link
Collaborator Author

As far as you promise that you are not gonna modify any .ts or .js file... ok :)

I will not.

why we should wait for a new PR

I'm a huge fan of meaningful distinct PRs and commits, this is why I spent so much time before submitting this PR such that someone can click on each commit and get something that makes sense on its own and compiles at every step of the way. So you can use tools like git bisect if need be and you can merge it without squashing to master. This is just my point of view of course.

@ibc
Copy link
Member

ibc commented Oct 10, 2021

So.... let's wait for CI and merge!!!!

@ibc
Copy link
Member

ibc commented Oct 10, 2021

please merge!

@nazar-pc nazar-pc merged commit df06636 into versatica:v3 Oct 10, 2021
@eli-schwartz
Copy link

Congrats on the successful conversion!

819 additions and 5,504 deletions. 

Whew, and that's even without deleting deps that meson can acquire for you. :D

@nazar-pc
Copy link
Collaborator Author

Those dependencies are just 4,700,569 lines of code, no big deal 🙃

@ibc
Copy link
Member

ibc commented Oct 10, 2021

I'll add a reminder tomorrow to make a new mediasoup release, announce it in the forum and update the website.

THANKS

@samarjit
Copy link

What are the steps to setup the compiler for windows?
I have msbuildtools 2019 and using make from mingw32-make, meson 0.59.2. I did not install visual studio community edition.
I am not able to setup the library paths. I tried LD_LIBRARY_PATH and LIBRARY_PATH but those did not work.

mediasoup>mingw32-make -C worker
mediasoup>cat meson-log.txt
Build started at 2021-10-21T03:16:14.667303
Main binary: c:\users\xxx\envs\p3\scripts\python3.exe
Build Options: -Db_ndebug=true -Db_pie=true -Db_staticpic=true -Dbuildtype=release
Python system: Windows
The Meson build system
Version: 0.59.2
Source dir: C:\mediasoup\worker
Build dir: C:\mediasoup\worker\out\Release
Build type: native build
Project name: mediasoup-worker
Project version: undefined
Sanity testing C compiler: cl
Is cross compiler: False.
Sanity check compiler command line: cl sanitycheckc.c /Fesanitycheckc.exe /MD /nologo /showIncludes /link
Sanity check compile stdout:
sanitycheckc.c
LINK : fatal error LNK1104: cannot open file 'MSVCRT.lib'

@nazar-pc
Copy link
Collaborator Author

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.

Drop gyp, move to CMake
7 participants