Replace make + Makefile file with Python Invoke library + tasks.py file #1239

ibc · 2023-11-23T13:49:59Z

Fixes #701

Details

Use Python Invoke package to replace make.
Invoke is automatically installed in a local path during mediasoup installation (in Node and Rust).
Keep worker/Makefile as a proxy to Invoke tasks defined in worker/tasks.py.
Everything works as before, nothing has changed from the user or developer perspective.

Bonus Tracks

Allow mediasoup installation under path with whitespaces (fixes Installation fails under path with whitespaces #753): c7720f2

Minor Details

Get rid of getmake.py and cpu_cores.sh scripts.
Update Building.md.

Fixes #701 WIP

nazar-pc

Finally a brave soul decided to tackle it!

package.json

ibc · 2023-11-23T15:51:10Z

@nazar-pc you know what this is doing exactly?

ifeq ($(OS),Windows_NT)
ifeq ($(MESON_ARGS),"")
	MESON_ARGS = $(subst $\",,"--vsenv")
endif
endif

As per my understanding it's basically doing this:

"If this is Windows and MESON_ARGS env is not set, set "--vsenv" value to it".

But I don't understand that "subst" thing at all...

nazar-pc · 2023-11-23T22:07:07Z

But I don't understand that "subst" thing at all...

I vaguely remembered that simple assigning was adding quotes around the value, so it wasn't setting MESON_ARGS="--vsenv", it was doing MESON_ARGS="\"--vsenv\"". Do not ask me why, it makes no sense to me either and I wasted way too much time trying to make sense of it, but I found that snippet somewhere that I can't interpret even after reading docs, that makes it work the way it should. I don't think you need to worry about it in Python though.

worker/tasks.py

…y since `--upgrafe` and `--target` are not compatible because Python (and hence) PIP sucks a lot

ibc · 2023-11-24T10:44:04Z

@nazar-pc I've done this: bb3ced0#diff-b2aa0be81dfc10bb28cd1f039c301d4436e336e9eaef746e7d2522788faf0883L74

nazar-pc · 2023-11-24T10:46:40Z

Why not installing all dependencies at once rather than separately if you know you'll need them? Then yo should not hit that issue and shouldn't need to have multiple directories.

ibc · 2023-11-24T10:54:20Z

Why not installing all dependencies at once rather than separately if you know you'll need them? Then yo should not hit that issue and shouldn't need to have multiple directories.

Because I prefer to not having to install pylint and all its subdependencies (as a requirement) in production environment. It would be sad that we solve that in npm ("dependencies" vs "devDependencies") and not in Python. And also for another reason:

As a solution for TODO 1 (see updated PR description) I'm about to suggest that npm prepare task installs invoke into a custom PIP folder so user doesn't need to do it. Now, if we use a custom pip folder and later try to reuse it for meson and ninja... same problem again. So I think that having separate pip setups for different needs it's ok. I mean, it's PIP fault.

ibc · 2023-11-24T10:59:05Z

OEEEE OEEEE OEEEE all CI passing!!!

@jmillan @nazar-pc Let's please discuss the TODO 1 in the PR description.

…eep Makefile as a proxy to invoke (WIP)

ibc · 2023-11-24T12:37:35Z

CI is gonna fail now due to this (on purpose): 36a5e90

…able (example in Alpine Linux)

ibc · 2023-11-24T13:41:01Z

To play with CI I've created a separate branch https://github.com/versatica/mediasoup/tree/replace-make-with-pip-invoke-ci-playground

ibc · 2023-11-24T16:47:47Z

Interesting. The "ModuleNotFoundError: No module named 'imp'" in macOS 12 CI tasks happens with these versions in the CI host:

Python 3.12.0
pip 23.3.1 from /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/pip (python 3.12)

And it works in my macOS:

Python 3.11.5
pip 23.3.1 from /usr/local/lib/python3.11/site-packages/pip (python 3.11)

Same pip version, which is the latest one: https://pypi.org/project/pip/
:(

UPDATE:

The problem is that Python 3.12 doesn't have imp module anymore. And invoke didn't properly migrated to importlib!!!

ibc · 2023-11-24T17:21:04Z

So the only remaining thing is installing invoke in worker/pip_invoke in build.rs before calling python -m invoke xxxx (obvious).

I need to replicate in Rust this code:

const PYTHON = getPython();

function getPython()
{
	let python = process.env.PYTHON;

	if (!python)
	{
		try
		{
			execSync('python3 --version', { stdio: [ 'ignore', 'ignore', 'ignore' ] });
			python = 'python3';
		}
		catch (error)
		{
			python = 'python';
		}
	}

	return python;
}

if (fs.existsSync(PIP_INVOKE_DIR))
{
	return;
}

// Install pip invoke into custom location, so we don't depend on system-wide
// installation.
executeCmd(
	`${PYTHON} -m pip install --upgrade --target=${PIP_INVOKE_DIR} invoke==${INVOKE_VERSION}`, /* exitOnError */ true
);

Any help please? Specially getting the python3 executable (unless it's ok if we assume env PYTHON || 'python3').

ibc · 2023-11-24T18:09:40Z

Rust thing almost done: 00df131 and 61664e5

This reverts commit 7b1f82c.

CI is green.

ibc · 2023-11-25T10:34:09Z

PR completely ready for review.

…) directly installing Invoke using pip3

- Plus cosmetic changes in CI files

# Conflicts: # npm-scripts.mjs

npm-scripts.mjs

jmillan

💯

…rker.yaml CI directly use Invoke commands instead of npm commands

nazar-pc

Looked through most things briefly.

I think that needs to change is Makefile. I think we should remove it or mention it in Building.md The fact that we still have it (and you, @ibc, plan to use it) while we're documenting that users need to install and use invoke instead (whose version doesn't guarantee to match actually) is odd. It feels inconsistent right now.

On a similar note I'd say we should remove numerous scripts from package.json too and make users call invoke/make instead there too for consistency. Otherwise right now we have 3 ways to format worker, which makes little sense to me to have.

nazar-pc · 2023-11-27T12:21:51Z

worker/build.rs

+    let mut pythonpath = if env::var("PYTHONPATH").is_ok() {
+        let original_pythonpath = env::var("PYTHONPATH").unwrap();


This is not idiomatic, you should do this instead:

Suggested change

let mut pythonpath = if env::var("PYTHONPATH").is_ok() {

let original_pythonpath = env::var("PYTHONPATH").unwrap();

let mut pythonpath = if let Ok(original_pythonpath) = env::var("PYTHONPATH") {

nazar-pc · 2023-11-27T12:22:49Z

worker/build.rs

-        );
+    // Install Python invoke package in custom folder
+    let pip_invoke_dir = format!("{out_dir}/pip_invoke");
+    let invoke_version = "2.2.0";


Is there a good reason why we need this specific version? By not specifying a version we'll get support for future Python versions and bugfixes for free.

If we do need it, this should be a constant at the top of the file instead.

I don't know yet how Invoke library behaves regarding compatibility or breaking changes. I think it's ok to force version 2.2.0 which we know it works in all archs. This doesn't mean we should never upgrade it. But I'm not confident enough to say ·any future version will work without changes anywhere".

I think checking their changelog is a pretty good indicator

nazar-pc · 2023-11-27T12:24:51Z

worker/build.rs

+        .arg("pip")
+        .arg("install")
+        .arg("--upgrade")
+        .arg(format!("--target={pip_invoke_dir}"))


Shouldn't this be two separate arguments instead?:

Suggested change

.arg(format!("--target={pip_invoke_dir}"))

.arg("--target")

.arg(pip_invoke_dir)

nazar-pc · 2023-11-27T12:27:30Z

package.json

-    "worker/test/include",
-    "worker/test/src",
+    "worker/deps/libwebrtc",
+    "worker/tasks.py",


I'm not sure why most of the files here were changed, they were in alphabetical order before and now they are in random order instead (worker/src is now before worker/include, etc.).

Also Makefile should be removed from this list and similar changes need to be done in worker/Cargo.toml too.

ibc · 2023-11-27T13:05:16Z

I think that needs to change is Makefile. I think we should remove it or mention it in Building.md The fact that we still have it (and you, @ibc, plan to use it) while we're documenting that users need to install and use invoke instead (whose version doesn't guarantee to match actually) is odd. It feels inconsistent right now.

I'm honoring all comments in a separate branch, but I don't want to git rid of Makefile. It's a simple additional file. It's just a convenient thing in case people don't want to deal with Invoke yet.

Replace make with pip invoke

f292141

Fixes #701 WIP

ibc requested review from jmillan and nazar-pc November 23, 2023 13:49

nazar-pc reviewed Nov 23, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

Reorder "files" in package.json (folders first)

5c3ba87

ibc added 3 commits November 23, 2023 18:43

WIP

76f7195

Let's see

6a31bdd

fixes

640edb5

ibc commented Nov 23, 2023

View reviewed changes

worker/tasks.py Outdated Show resolved Hide resolved

ibc added 9 commits November 24, 2023 00:53

Don't need cpu_cores.sh anymore

1a3914b

improve tasks.py by manually running pylint on it

26c4faf

make 'lint' task also lint tasts.py file itself

12ba1a3

test

d44d81c

goto hell pylint

37aeb56

let's see if this fixes the "PTY not supported in Windows"

a78e61e

fix some issues

4b1a7a1

cosmetic

4394329

Use different pip folders for groups of packagesd installed separatel…

bb3ced0

…y since `--upgrafe` and `--target` are not compatible because Python (and hence) PIP sucks a lot

ibc added 4 commits November 24, 2023 12:05

Remove TODO_INVOKE.md

f4c46a1

fix typo

af9f836

TMP: Install invoke in Dockerfiles

8ccfb27

Install pip invoke in a custom location during npm installation and k…

36a5e90

…eep Makefile as a proxy to invoke (WIP)

fix super error

f104140

tasks.py: use sh instead of bash in *unix/macOS since it's more avail…

485e240

…able (example in Alpine Linux)

ibc changed the title ~~Replace make with pip invoke~~ Replace make & Makefile file with Python Invoke library & tasks.py file Nov 25, 2023

ibc added 3 commits November 25, 2023 11:24

Replace make with pip invoke ci playground (#1240)

7b1f82c

Revert "Replace make with pip invoke ci playground (#1240)" (#1241)

5db1860

This reverts commit 7b1f82c.

Replace make with pip invoke ci playground (#1242)

7abd48e

CI is green.

ibc marked this pull request as ready for review November 25, 2023 10:32

Update CHANGELOG.md

d56acc2

ibc requested a review from nazar-pc November 25, 2023 10:33

ibc changed the title ~~Replace make & Makefile file with Python Invoke library & tasks.py file~~ Replace make + Makefile file with Python Invoke library + tasks.py file Nov 25, 2023

ibc added 4 commits November 25, 2023 12:03

CI codeql.yaml: try to fix CI issue (Cannot find package 'node-fetch'…

e2e6304

…) directly installing Invoke using pip3

npm-scripts.mjs: remove old msys stuff

90b8e33

- Plus cosmetic changes in CI files

cosmetic

d5254a4

Allow mediasoup installation under path with whitespaces (fixes #753)

c7720f2

# Conflicts: # npm-scripts.mjs

ibc mentioned this pull request Nov 26, 2023

Installation fails under path with whitespaces #753

Closed

Revert change in build.rs since Rust doesn't like \" in format()

2913eb0

ibc commented Nov 26, 2023

View reviewed changes

npm-scripts.mjs Outdated Show resolved Hide resolved

Update CHANGELOG

8968300

jmillan approved these changes Nov 27, 2023

View reviewed changes

ibc added 3 commits November 27, 2023 11:59

Remove useless npm scripts in main package.json and make mediasoup-wo…

42415ca

…rker.yaml CI directly use Invoke commands instead of npm commands

Forgot to remove useless npm ci in mediasoup-worker.yaml

482562c

Fix PYTHONPATH usage

7992108

ibc merged commit 3f0594d into v3 Nov 27, 2023
36 checks passed

ibc deleted the replace-make-with-pip-invoke branch November 27, 2023 12:27

nazar-pc reviewed Nov 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace make + Makefile file with Python Invoke library + tasks.py file #1239

Replace make + Makefile file with Python Invoke library + tasks.py file #1239

ibc commented Nov 23, 2023 •

edited

Loading

nazar-pc left a comment

ibc commented Nov 23, 2023

nazar-pc commented Nov 23, 2023

ibc commented Nov 24, 2023

nazar-pc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023 •

edited

Loading

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023 •

edited

Loading

ibc commented Nov 25, 2023

jmillan left a comment

nazar-pc left a comment

nazar-pc Nov 27, 2023

nazar-pc Nov 27, 2023

ibc Nov 27, 2023

nazar-pc Nov 27, 2023

nazar-pc Nov 27, 2023 •

edited

Loading

nazar-pc Nov 27, 2023 •

edited

Loading

ibc commented Nov 27, 2023

		let mut pythonpath = if env::var("PYTHONPATH").is_ok() {
		let original_pythonpath = env::var("PYTHONPATH").unwrap();

	let mut pythonpath = if env::var("PYTHONPATH").is_ok() {
	let original_pythonpath = env::var("PYTHONPATH").unwrap();
	let mut pythonpath = if let Ok(original_pythonpath) = env::var("PYTHONPATH") {

	.arg(format!("--target={pip_invoke_dir}"))
	.arg("--target")
	.arg(pip_invoke_dir)

Replace make + Makefile file with Python Invoke library + tasks.py file #1239

Replace make + Makefile file with Python Invoke library + tasks.py file #1239

Conversation

ibc commented Nov 23, 2023 • edited Loading

Details

Bonus Tracks

Minor Details

nazar-pc left a comment

Choose a reason for hiding this comment

ibc commented Nov 23, 2023

nazar-pc commented Nov 23, 2023

ibc commented Nov 24, 2023

nazar-pc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023 • edited Loading

ibc commented Nov 24, 2023

ibc commented Nov 24, 2023 • edited Loading

ibc commented Nov 25, 2023

jmillan left a comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Nov 27, 2023

Choose a reason for hiding this comment

nazar-pc Nov 27, 2023

Choose a reason for hiding this comment

ibc Nov 27, 2023

Choose a reason for hiding this comment

nazar-pc Nov 27, 2023

Choose a reason for hiding this comment

nazar-pc Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

nazar-pc Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

ibc commented Nov 27, 2023

ibc commented Nov 23, 2023 •

edited

Loading

ibc commented Nov 24, 2023 •

edited

Loading

ibc commented Nov 24, 2023 •

edited

Loading

nazar-pc Nov 27, 2023 •

edited

Loading

nazar-pc Nov 27, 2023 •

edited

Loading