feat: automate and publish prebuilt binaries #1083

barlock · 2023-05-18T14:06:38Z

This PR automates and builds prebuilt binaries that get downloaded as a post-install step instead of building.

Typically builds on CI take me 6 minutes which over time adds up pretty significantly (especially when you consider that adding up to about 4 years of total community compute time per week of installs of this package 😱). This also cuts the build requirements for windows machines.

I've been using this branch in a fork to create prebuilds for myself since Dec and it's been working great.

The only change to typical process that you would need to make after this change would be that instead of creating just tags, you would need to create a release with the same name as the tag. You can see examples in my fork.

jmillan

This is really nice!

npm-scripts.js

jmillan · 2023-05-19T08:38:20Z

npm-scripts.js

+function extractRemoteTar(tarUrl, cwd)
+{
+	const tar = require('tar');
+	const axios = require('axios');


Can we avoid this dependency just for making a single HTTP request?

I'm unaware of a simple, dependency free way to make http requests. fetch in node is experimental in 17.5 and still hasn't entered stability in 19.

Do you know of a way? (Simple being the key, I can use node:http but it would add a lot of code)

node-fetch is 107KB while axios is 1.74MB.

I think the former is more than enough for the needed usage.

Done! Swapped for node-fetch. Note that node-fetch >= 3 requires ESM, so I used node-fetch@2

ibc · 2023-05-19T08:46:14Z

Actually we already rejected a similar PR in the past and I would like to re-check the reasons why we did it. Let's please put this PR on hold for a while until we check that.

barlock · 2023-05-19T13:09:58Z

Previously you'd rejected issues I'd created b/c you didn't want to maintain building the binaries and thought letting someone else create the binaries for you would be easier, which it obviously is. I think this introduces security risks though (Eg, how does an installer know that the binaries in the npm package aren't malicious if they don't come from the authors of the project?).

Since the previous recommendation was "Put it in a separate page" I did that, as it's been working for a while, I figured I'd at least make the PR and see if your mind had changed, otherwise I'll keep maintaining my fork with this branch while I'm using this project.

Co-authored-by: José Luis Millán <jmillan@aliax.net>

ibc · 2023-05-19T13:26:24Z

I see, thanks. Changes make sense and look great. Let's please have some days before we fully review and merge this PR.

ibc · 2023-05-19T13:45:00Z

Easy question for now: how to prevent "npm ci" in my mediasoup local repo from downloading the binary instead of locally building it? If you call "make clean" in worker folder in your local fork and then run "npm run release:check", does it build the worker or does it fetch it from GH? It must do the former.

barlock · 2023-05-19T14:47:51Z

npm run release:check will build the worker.

npm ci without any flags will download it, however npm run release:check specifically adds --ignore-scripts so it can explicitly build it without using postinstall

ibc · 2023-05-23T12:43:24Z

.gitignore


 ## Worker.
-/worker/out
+out/


Why this change? Why was worker/out worse? INMHO it makes sense that worker binary is located in worker/ folder. Do I miss something?

I needed a place to download the prebuilds to, I wanted them to be different than worker/out so we wouldn't accidentally start taring tarballs, so I put them in repo-root/out. the binary is still in worker/out.

repo-root/out is fairly arbitrary, I was guessing what you'd prefer based on patterns in the repo, do you have thoughts on a different location for that?

I would love worker/prebuilds/ and of course gitignore it.

ibc · 2023-05-23T12:44:47Z

npm-scripts.js

 		{
-			buildWorker();
-
-			if (!process.env.MEDIASOUP_LOCAL_DEV)


We should not remove this condition. If MEDIASOUP_LOCAL_DEV env is given we should neither download binary from GH nor building it locally. Here in this PR we are downloading the binary from GH anyway.

I'm going to update the PR to do what you described in this comment, as the logic is simpler and the result should be faster. (If MEDIASOUP_LOCAL_DEV clean worker, else, attempt to download, if that fails, build it.

However, I think the current implementation if MEDIASOUP_LOCAL_DEV is set, builds the worker then removes it? Is that intentional?

Let me know if I misinterpreted your feedback.

If MEDIASOUP_LOCAL_DEV is given then it should not download the prebuilt binary, it should build it locally and should NOT delete built artifacts after it. It's basically for local development for not having to build everything when running make in worker folder.

However my comment was wrong (the above is correct). I meant another variable that is... I don't remember but you can see it in Worker.ts. If given, install or postinstall script should neither download binary nor try to build local one. This is for people that want to run mediasoup with a binary compiled by themselves in some path (given in that env). Hope it's clear now.

Ok, I meant MEDIASOUP_WORKER_BIN which is used in lines below.

ibc · 2023-05-23T12:46:50Z

npm-scripts.js

+	}
+}
+
+function createTar(files, dest)


Shouldn't this function be async? Shouldn't the caller awaits for it to complete? Same concern for functions below.

Most are, just not explicitly. I went through and explicitly called them out as async or converted them to return promises if they didn't already.

ibc · 2023-05-23T16:04:18Z

npm-scripts.js

 			{
-				cleanWorker();
-			}
+				console.error('Failed to download prebuilt binary, building instead', error.message);


Those logs don't conform to the syntax of other console logs in same file. Also, why are you using then and catch instead of try/catch with await in the try block? We strongly promote that.

Old habits I guess. This is the first instance of async/await in this file and top level await was added unflagged in node 14.8. Your package.json says you support >=16 so that change should be fine. I'll make the change.

Note that it will explicitly break earlier versions of node, which I just confirmed are out of maintenance, so probably fine.

Yup :)

BTW I've pulled your branch and I'm some cosmetic changes so I'll ask you later to cherry-pick them in your branch. So let me please convert those to await syntax.

Hmm. Today I learned something new about top level await (honestly never attempted it before). In order to convert the syntax to async/await I'd need to convert the whole file into ESM instead of CJS.

So, renamed to npm-scripts.mjs and swap the require for import statements.

I'm happy to do so, but I wanted to check with you before I rewrite a lot.

Actually I'm making many changes into the file, wondering if you'd mind to wait for me to finish

👍 Sorry, pausing for a few hours, I'll circle back around in the afternoon after meetings and your changes are done.

Thanks a lot. I'll ping you here when I'm done.

ibc · 2023-05-23T16:55:36Z

Correct me if I'm wrong but we don't really need those 'prebuild:download' and 'prebuild:unpackage' tasks at all, right? I'm trying to simplify things.

barlock · 2023-05-23T16:58:59Z

We don't, I added them for development of the script itself, so they could be removed.

ibc · 2023-05-23T17:32:39Z

Commit done, but I had to fork your fork: c1f08d8

So if you like, can you git cherry-pick it?

NOTE: I'll be back tomorrow.

ibc · 2023-05-24T08:40:45Z

@barlock I am working in a forked branch/PR here: #1087

There are two concerns I have:

When running "npm install mediasoup" and the script fetches the prebuilt binary (and doesn't build local one) we are NOT using the binary later ¯_(ツ)_/¯. This is because Worker.ts still expects to spawn the binary at worker/out/Release/mediasoup-worker. So I'll modify Worker.ts in a way that it tries first to load the binary from worker/prebuild/Release.
For local development, make command must delete worker/prebuild/* so later it will use the locally build one.
Both bullets above must also apply to Rust.
When running CI tasks, npm ci (which will trigger "postinstall" script as usual) should NOT fetch the prebuilt binary but should always build it locally. I'll add an enc for this.

I'll try to do these things later in my branch.

barlock · 2023-05-24T13:01:11Z

we are NOT using the binary later ¯_(ツ)_/¯.

This PR untar's the downloaded worker to worker/out/Release/mediasoup-worker*. I agree it's probably cleaner code wise to separate those too and allow Worker.tsto pull the binary from aprebuild` directory, though it does run into some complications as you mention.

I am working in a forked branch/PR here: #1087

Great! How would you like me to help with those changes? Would you prefer me to make PRs against that PR or cherry-pick into this one? I believe I also gave you should have the ability to make changes on this branch as a maintainer?

While pulling in changes I'm testing the end-to-end CD flow in my fork that I've been using. GHA are failing in your changes due to require statements moving to the top of npm-scripts. Your current CI logic is to use npm-scripts to perform a specialized install, so it can only use native requires. tsc, tar and node-fetch requires will fail, so I'd moved them into the function/case that used them to avoid that issue.

Alternatively we could just add a npm ci at the top of your steps, but I'm guessing you chose not to do that for a reason.

ibc · 2023-05-24T14:18:34Z

If you don't mind I'm doing too many changes already in my branch. Not sure why I couldn't push to yours. I use gh pr checkout NUMBER command which, sometimes, doesn't create a fork of the PR but this time it did.

Would you mind waiting for me to finish my ongoing stuff and then we discuss pending things?

Regarding use of require instead of import (note: I already fixed those issues), problem is that some deps in npm-scripts files MUST NOT be available when users install mediasoup in production mode (npm install mediasoup --production) since those deps are in devDependencies so they are not installed. So we cannot unconditionally import them in npm-scripts. However, we can use require() dynamically in the file so IMHO it's better if we keep using CommonJS instead of switching to ESM. Definitely we don't want that users have to install all deps in devDependencies when they install mediasoup.

ibc · 2023-05-24T15:59:15Z

NOTE: All credits will be for you in CHANGELOG and announcement and so on.

barlock · 2023-05-24T19:38:58Z

👍 I'll keep checking out your branch and ill wait for you to let me know how and when I can pitch in. Thanks for the credit, 😁 I'm more interested in seeing the change go through and I'm happy it's getting added.

ibc · 2023-05-25T13:18:52Z

I'm almost done in #1087. Let me please close this one.

ibc · 2023-05-25T17:51:17Z

PR is done and all pending stuff done: #1087

feat: publish prebuilt binaries

bedc6ad

jmillan reviewed May 19, 2023

View reviewed changes

Apply suggestions from code review

ba41b5e

Co-authored-by: José Luis Millán <jmillan@aliax.net>

barlock added 2 commits May 22, 2023 12:50

swap axios for node-fetch

3c49fb9

fix linting issues

e11a03d

ibc reviewed May 23, 2023

View reviewed changes

fix postinstall logic, explicitly make functions async

67dad4d

ibc reviewed May 23, 2023

View reviewed changes

barlock added 3 commits May 23, 2023 12:34

fix log format and change prebuild dir

75f8b20

fix log format and change prebuild dir

8d0a089

fix postinstall logic

5bddd85

ibc mentioned this pull request May 24, 2023

Automate and publish prebuilt mediasoup-worker binaries #1087

Merged

7 tasks

ibc closed this May 25, 2023

Uh oh!

feat: automate and publish prebuilt binaries #1083

feat: automate and publish prebuilt binaries #1083

Uh oh!

Conversation

barlock commented May 18, 2023

Uh oh!

jmillan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmillan May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmillan May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibc commented May 19, 2023

Uh oh!

barlock commented May 19, 2023

Uh oh!

ibc commented May 19, 2023

Uh oh!

ibc commented May 19, 2023

Uh oh!

barlock commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibc commented May 23, 2023

Uh oh!

barlock commented May 23, 2023

Uh oh!

ibc commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibc commented May 24, 2023

Uh oh!

barlock commented May 24, 2023

Uh oh!

ibc commented May 24, 2023

jmillan May 19, 2023 •

edited

Loading

jmillan May 22, 2023 •

edited

Loading

barlock commented May 19, 2023 •

edited

Loading

ibc commented May 23, 2023 •

edited

Loading