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

Improvements to the build system #1245

Merged
merged 10 commits into from
Nov 27, 2023
Merged

Improvements to the build system #1245

merged 10 commits into from
Nov 27, 2023

Conversation

ibc
Copy link
Member

@ibc ibc commented Nov 27, 2023

Details

  • Don't specify Python Invoke version.
  • build.rs: Idiomaticvs improvements.
  • npm-scripts.mjs: Use spawn instead of exec to run commands, so we can pass command and each argument separately.
  • Honor alphabetical order in "files" in package.json and remove Makefile from it and worker/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:

  1. Python Invoke doesn't specify an API to pass command name and separate argument strings when running external commands. Honestly I don't even know if Python itself supports it. Ticket reported in Invoke project: context.run(): Expose API to pass command and separate args instead of a single string pyinvoke/invoke#977
  2. Node does offer an API for that: child_process.spawn(command[, args][, options]). However it doesn't escape or encode properly individual arguments. For example:
const path = '/tmp/123 foo "bar';

cont result = child_process.spawnSync('ls', [ '-ls', path ]);

result.stderr.toString();
// => 'ls: /tmp/123 foo "bar: No such file or directory\n'

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.

ibc added 3 commits November 27, 2023 14:05
- 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`.
@ibc ibc requested a review from nazar-pc November 27, 2023 15:37
Copy link
Collaborator

@nazar-pc nazar-pc left a 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

worker/Cargo.toml Show resolved Hide resolved
ibc added 4 commits November 27, 2023 17:30
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.
@ibc
Copy link
Member Author

ibc commented Nov 27, 2023

I'm reverting changes in npm-scripts and use execSync() again. Why? Because mediasoup-node CI task fails on Windows due to "tsc ENOENT": https://github.com/versatica/mediasoup/actions/runs/7008109231/job/19063676923?pr=1245

Revert commit here: e39731d

When running 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. Using exec() is way more comfortable than using spawn() and using the latter has proven to not fix ANYTHING.

@ibc ibc merged commit 95390bd into v3 Nov 27, 2023
36 checks passed
@ibc ibc deleted the improvements-to-build-setup branch November 27, 2023 18:11
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.

2 participants