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

Use spawn(), not execFile(). #345

Merged

Conversation

LindirQuenya
Copy link
Member

Change execFile(file, args) to spawn(file, args, {'shell':false}).
Most of the justification for switching may be found here.
Because execFile() isn't used anymore, I changed the name of the execFile parameter to noshell, and eliminated the redundant shell parameter (except in some ManagedChildProcess internals where shell makes more sense).

Also, fix createCommand() on Mac and Linux (#333). Previously, it would surround the filename in quotes, regardless of the execFile (now noshell) argument.

LindirQuenya and others added 5 commits May 28, 2022 11:38
refactor: turn a bunch of Promise functions into async's.

Previously, onError was unused.
…able-promises

refactor: change some confusing Promise() functions into clearer async's.
Create a new parameter, `noshell`, which is negated to get the
value of the `shell` parameter given to `spawn()`.

Make spawning use `spawn(file, args, {'shell': false})` instead of
`execFile(file, args)`. `spawn` gives a stdio stream, whereas `execFile`
waits for the subprocess to exit, and then gives all the output.
Also don't escape args for a shell when we don't use a shell.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2471651780

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 56.684%

Totals Coverage Status
Change from base Build 2401499397: 0.0%
Covered Lines: 376
Relevant Lines: 591

💛 - Coveralls

@LindirQuenya
Copy link
Member Author

Come to think of it, maybe hotfix/10.1.3 would have been a better target for this PR. You can change the target before merging, right?

@colin969
Copy link
Member

Wrapping in quotes is a requirement - Behaviour, particularlly in native Flash player executables, differs depending on spacing in URLs.

@LindirQuenya
Copy link
Member Author

Unsure what you mean by that. The change with the quotes only affects the filename, not the arguments.

@colin969 colin969 changed the base branch from develop to hotfix/10.1.3 June 15, 2022 23:40
@colin969 colin969 merged commit 9b384ad into FlashpointProject:hotfix/10.1.3 Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants