-
-
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
Improvements to the build system #1245
Conversation
- Don't specify Python Invoke version. - Idiomaticvs improvements in `build.rs`. - Honor alphabetical order in "files" in `package.json` and remove `Makefile` from it and `worker/Cargo.toml`.
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.
I like less shell scripts, just one nit in build.rs
Why? Because mediasoup-node CI task fail son Windows due to "tsc ENOENT": https://github.com/versatica/mediasoup/actions/runs/7008109231/job/19063676923?pr=1245 When runnin any npm script, npm adds `node_modules/.bin` to the `PATH`. Otherwise calling `tsc` here wouldn't work at all. But for whatever reason it's failing when using `spawn()` despite the created process inherits `process.env` as documented. I don't want to waste time investigating this.
I'm reverting changes in npm-scripts and use Revert commit here: e39731d When running any npm script, npm adds |
Details
build.rs
: Idiomaticvs improvements.npm-scripts.mjs
: Use spawn instead of exec to run commands, so we can pass command and each argument separately.package.json
and removeMakefile
from it andworker/Cargo.toml
.Does this PR fix installation under paths with a double quote in a directly name?
No. I'm afraid there is no easy way to do that. For many reasons:
child_process.spawn(command[, args][, options])
. However it doesn't escape or encode properly individual arguments. For example:At least installation in a path with whitespaces keep working (it was already since previos PR). I just give up here and honestly nobody should expect thst any library work properly when installing it under a too fancy path. BTW this is not the purpose of this PR anyway.