-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Avoid build commands when MEDIASOUP_WORKER_BIN is set #695
Conversation
This also prevents needing to even install build tools when MEDIASOUP_WORKER_BIN env var is set.
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.
Looks good to me. Tested?
Tests were done manually for this feature. Here are some commands that show the complete test: First, a Stage 1 (Docker image: node:16 which is Debian with Node and extra tools preinstalled) is prepared with all build deps and used to build the docker run -ti --rm --name stage1 \
--entrypoint /bin/bash \
-v /path/to/mediasoup:/host/mediasoup \
node:16
apt-get update && apt-get install --no-install-recommends --yes \
build-essential \
python3 python3-pip
su node
cd /host/mediasoup/
npm install Now, a clean Stage 2 is used to test installing with and without this PR's change. Before the change docker run -ti --rm --name stage2 \
--entrypoint /bin/bash \
-v /path/to/mediasoup:/host/mediasoup \
node:16-slim
export NODE_ENV="production"
export MEDIASOUP_WORKER_BIN="/host/mediasoup/worker/out/Release/mediasoup-worker"
su node
cd /host/mediasoup/
rm -rf node_modules/
npm install This renders the following error:
And installing Make would then complain about missing Python: exit # Exit `su node`
apt-get update && apt-get install --no-install-recommends --yes \
make
su node
cd /host/mediasoup/
rm -rf node_modules/
npm install
After the change Stage 2 runs without errors: docker run -ti --rm --name stage2 \
--entrypoint /bin/bash \
-v /path/to/mediasoup:/host/mediasoup \
node:16-slim
export NODE_ENV="production"
export MEDIASOUP_WORKER_BIN="/host/mediasoup/worker/out/Release/mediasoup-worker"
su node
cd /host/mediasoup/
rm -rf node_modules/
npm install
# OK Is this enough? I thought how to easily make an automated test from this but it's a bit contrived thing to make because it requires modifying what is installed or not in the system (hence why I used Docker for clean baseline on each stage) |
Honestly I don't know if that's enough, probably it's but it's too Docker dependent. Something easier:
|
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.
This is correct, thanks!
When I was refactoring it I moved check into worker:build
, but then when I added cleanup into postinstall
it became incorrect.
All done and yes, without the change Note that my test was actually better by using Docker, and it's not really dependent on Docker itself at all, it's just the tool that I happen to use to ensure a clean (and repeatable) system, and the errors are more obvious because there is nothing installed. In your step 4,
running without Docker does actually work in my system, because I have lots of customization, tools and packages installed, included Meson. So for a true, isolated test, I would need to install a clean Debian system or maybe make a virtual machine... but at that point, a clean Docker container is already equivalent to those (and faster) |
Thanks. Merging. |
Small fix that prevents needing to even install build tools when
MEDIASOUP_WORKER_BIN
env var is set.Fixes #694